InseeFrLab / onyxia-api

REST API of Onyxia
https://onyxia.sh
MIT License
23 stars 28 forks source link

Issue with ingress annotations in regions #440

Closed athomaj closed 4 months ago

athomaj commented 4 months ago

Hi,

I'm deploying Onyxia on a kube cluster using the helm chart. We have an issue with the ingress annotations as we want to add the cluster issuer as shown in the doc. I was able to find in some of the doc that this was added to the region/expose json file, but it doesnt seem to have been actually implemented in the api code. Here is what my code looks like:

expose: { domain: "onyxia.xxxx.xx", annotations: { "cert-manager.io/cluster-issuer": "letsencrypt" }, ingressClassName: "nginx", ingress: true, "certManager": { "useCertManager": true, "certManagerClusterIssuer": "letsencrypt" } },

I tried to update the other fields and i see that the modifications are well reflected to the ingress values, but not for the annotations, here are the values: ingress: certManagerClusterIssuer: letsencrypt enabled: true hostname: user-julien2-632482-0.onyxia.xxxx.xx ingressClassName: nginx useCertManager: true userHostname: user-julien2-632482-user.onyxia.xxxx.xx

thanks,

olevitt commented 4 months ago

Hi !

Ingress annotations is (at least from what I see) properly implemented in Onyxia-API, see here : https://github.com/InseeFrLab/onyxia-api/blob/4e7aaf00aa464fea794481c9fc6fdefa7d2b5be6/onyxia-model/src/main/java/fr/insee/onyxia/model/region/Region.java#L1149 Your issue may be that to be injected onto your service it has to also be defined in the actual values.schema.json file of the package (helm chart) you install.
What catalog / service are you using ? I just checked our main catalog (helm-charts-interactive-services) with the popular vscode-python service and it appears you are right in that the annotations injection is not defined in the values.schema.json, see https://github.com/InseeFrLab/helm-charts-interactive-services/blob/main/charts/vscode-python/values.schema.json#L532
But my understanding is that it's a per-catalog issue and not an API issue

fcomte commented 4 months ago

To make cert manager works , there is no need to inject annotations. I will check in our test environnement but it's supposed to work with your values

ihiverlet commented 4 months ago

Hi, this configuration makes cert manager work with our helm charts .

ingress:
  enabled: true
  hostname: user-inesh-998250-0.user.domain
  ingressClassName: onyxia
  useCertManager: true
  certManagerClusterIssuer: "my-cluster-issuer"
  userHostname: user-inesh-998250-user.user.domain

What are you trying to achieve ?

athomaj commented 4 months ago

Hi,

I'm using a cluster issuer with letsencrypt, and nginx as the default ingressclass. Usually I need to add the annotations to my ingress for letsencrypt to generate the certificate. otherwise it wont pick it up and generate the certificate, and I cant access the ingress as it's insecure. That's why i tried the annotations and saw that it was not reflected on the two first i tried, which are jupiter and rstudio. I'm not a Java guy, but i saw that it was not reflected in this file: InseeFrLab/onyxia-api/onyxia-api/src/main/resources/regions.json

this might be completely off, just something i noticed.

Thanks

fcomte commented 4 months ago

There is no need to use the broken annotation feature. If you use the certManager region configuration it should work.

fcomte commented 4 months ago

can you check the annotation of the ingress generated on rstudio when certManager is enable in your region configuration

athomaj commented 4 months ago

Well i just tried again and indeed it works now, i must have tested to early after i changed my configs. I added a few things at the same time so not sure what fixed it. This is the current one that's working now:

expose: { domain: "onyxia.atelier.ovh", annotations: { "cert-manager.io/cluster-issuer": "letsencrypt" }, ingressClassName: "nginx", ingress: true, "certManager": { "useCertManager": true, "certManagerClusterIssuer": "letsencrypt" } },

I'll just do a few more tests removing to make sure of what fixed it, and let you know, because that's for sure that i had this one from the beginning: "certManager": { "useCertManager": true, "certManagerClusterIssuer": "letsencrypt" }

Thanks for the help

athomaj commented 4 months ago

Alright, so to confirm:

ingressClassName: "nginx" was the missing piece. Annotations seems indeed useless. This is the default and only ingressclass in my cluster and i never have the need to specify it but there it looks like its not adding the annotations if its not set.

Thanks again for the help, and keep up the good work!