Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[🚀 Feature]: Add option to disable nginx.ingress.kubernetes.io/proxy-ssl-secret annotation in selenium chart #2720

Closed
LluisCalm opened this issue Mar 20, 2025 · 5 comments · Fixed by #2724

Comments

@LluisCalm
Copy link

LluisCalm commented Mar 20, 2025

Feature and motivation

The ingress resource created by the chart adds always the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation which makes this ingress inaccessible by default if the tls secret is created, for example, by cert manager.

Code cample showing that the annotation is mandatory:

    {{- if not (empty .sslSecret) }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl .sslSecret $ | quote }}
    {{- else if (empty $.Values.ingress.tls) }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s/%s" $.Release.Namespace (include "seleniumGrid.tls.fullname" $)) $ | quote }}
    {{- else }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s/%s" $.Release.Namespace (index $.Values.ingress.tls 0).secretName) $ | quote }}
    {{- end }}

I propose that if .sslSecret is empty, that annotation should not be added. This can be achieved in multiple ways, one could be by simply disabling the default value:

    {{- if not (empty .sslSecret) }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl .sslSecret $ | quote }}
    {{- else if (empty $.Values.ingress.tls) }}
nginx.ingress.kubernetes.io/proxy-ssl-secret: {{ tpl (printf "%s/%s" $.Release.Namespace (include "seleniumGrid.tls.fullname" $)) $ | quote }}
    {{- end }}

Usage example

I would use this feature to be able to create an ingress resource without the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation.

Copy link

@LluisCalm, thank you for creating this issue. We will troubleshoot it as soon as we can.


Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

@VietND96
Copy link
Member

Hi, as my understanding, when you configure the ingress like

ingress:
  hostname: selenium-grid.prod.domain.com
  tls:
    - secretName: my-external-tls-secret
      hosts:
        - selenium-grid.prod.domain.com

The secretName here will not pass to ingress annotation, right?

@LluisCalm
Copy link
Author

The configuration you provided would work since you specify a secretName, and it could be a solution to the problem I was experimenting, thanks! But, if someone use cert manager and do not specify a secretName in the TLS config, cert manager will automatically create a secret for the ingress and the annotation would look like:

nginx.ingress.kubernetes.io/proxy-ssl-secret: my-namespace/%!s(<nil>)

@VietND96
Copy link
Member

Thanks for your input. So, below config should be considered as valid

ingress:
  hostname: selenium-grid.prod.domain.com
  tls:
    - hosts:
        - selenium-grid.prod.domain.com

And template should deal with this case properly to avoid nil value

@LluisCalm
Copy link
Author

Yes, imho the easiest way is that in case secretName does not exist, do not add the nginx.ingress.kubernetes.io/proxy-ssl-secret annotation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants