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

charts: add ingressClassName to ingress template #1932

Merged
merged 2 commits into from
May 3, 2024

Conversation

ccolic
Copy link
Contributor

@ccolic ccolic commented Apr 26, 2024

Description

This PR updates the existing ingress chart template and adds the field "ingress.ingressClassName".
According to the kubernetes documentation, setting the ingressClassName in the annotations is deprecated: https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation

This fixes issue #1895

Changes

Added one line to the ingress chart template to template the field Values.ingress.ingressClassName if it exists

Verification

I tested the change locally by defining a simple my-values.yaml file:

ingress:
  enabled: true
  ingressClassName: nginx
  hosts:
  - host: chart-example.local
  tls:
  - secretName: chart-example-tls
    hosts:
      - chart-example.local

Using helm template I confirmed that a correct Ingress was templated:

helm template headlamp --namespace headlamp --create-namespace -f my-values.yaml .

Results in:

# Source: headlamp/templates/ingress.yaml
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: headlamp
  labels:
    helm.sh/chart: headlamp-0.20.0
    app.kubernetes.io/name: headlamp
    app.kubernetes.io/instance: headlamp
    app.kubernetes.io/version: "0.23.1"
    app.kubernetes.io/managed-by: Helm
spec:
  ingressClassName: nginx
  tls:
    - hosts:
        - "chart-example.local"
      secretName: chart-example-tls
  rules:
    - host: "chart-example.local"
      http:
        paths:

Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @ccolic. Can you please add the field ingressClassName in values.yaml and the docs for it in charts/headlamp/README.md. Thanks

@joaquimrocha joaquimrocha marked this pull request as draft April 30, 2024 16:11
@ccolic
Copy link
Contributor Author

ccolic commented Apr 30, 2024

@knrt10 I have added it. I hope the comments are okay like this. I decided to also remove the deprecated annotation. I could also leave that in, but it seems duplicate for me.

* instead of setting the ingress class name as annotation, users can now set "ingress.ingressClassName"

Signed-off-by: Christian Colic <christian@colic.io>
* add to README
* add to values.yaml

Signed-off-by: Christian Colic <christian@colic.io>
@knrt10 knrt10 marked this pull request as ready for review May 2, 2024 04:39
Copy link
Contributor

@knrt10 knrt10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @ccolic.

@joaquimrocha joaquimrocha merged commit d4f7748 into headlamp-k8s:main May 3, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants