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

KANSUP74-12 add ingressClass config #163

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

RVanhuysseXenit
Copy link
Contributor

Since kubernetes 1.18, the IngressClass object (& ingressClassName property) are intended as a replacement for the deprecated kubernetes.io/ingress.class annotation.

This pr provides default config for our alfresco deployment.

https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation

@Brobrechts
Copy link
Contributor

Brobrechts commented Sep 27, 2024

I'm trying to understand what it does,

So by adding the field:

ingressClassName: nginx to the Ingress resources...

it will automatically look for the ingressClass resources named nginx.

this defintions inside the ingressClass resource then appends? the controller: k8s.io/ingress-nginx field defined in the class to the original one?

@RVanhuysseXenit
Copy link
Contributor Author

I'm trying to understand what it does,

So by adding the field:

ingressClassName: nginx to the Ingress resources...

it will automatically look for the ingressClass resources named nginx.

this defintions inside the ingressClass resource then appends? the controller: k8s.io/ingress-nginx field defined in the class to the original one?

If I understood correctly, the nginx-controller used to look for annotations on the ingress object to detect whether it should interact. However, this was implicit behaviour and not officially standardized.
Now a separate object is made that explicitly links to the ingress through the ingressClassName property and explicitly defines the controller that interacts. The ingressClass object also allows us to define additional config for the target controller, and allows that config to be loaded from other external objects.
The annotation is still present, and should be interpreted for backwards compatibility, but towards the future we should focus on ingressClass.

…ssClassName configurable, update templating for annotation vs property
Copy link
Contributor

@gert-glassee gert-glassee left a comment

Choose a reason for hiding this comment

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

  • Why the local-values.yaml?
  • Now you define the ingress class 2x, once as an annotation in the values file and once as an ingress.ingressClass variable. Ideally this should be only one since both values should be the same. The variable should also be used to define the annotations.
    Unfortunately this annotation is already overwritten by some local deployments (OUP) where we set the ingress.class:alb
  • also update the readme file
  • and preferably also the changelog, certainly when this would introduce a breaking change if we overwrite the ingressAnnotations with another value.

@RVanhuysseXenit
Copy link
Contributor Author

  • Why the local-values.yaml?

local-values.yaml was a pre-existing part of the repo to install the chart against a local k8s cluster. It provides some overrides/extensions of the default values that are required. I used it for testing, as well as for demonstrating how to fall back on the annotation.

  • Now you define the ingress class 2x, once as an annotation in the values file and once as an ingress.ingressClass variable. Ideally this should be only one since both values should be the same. The variable should also be used to define the annotations.
    Unfortunately this annotation is already overwritten by some local deployments (OUP) where we set the ingress.class:alb
    The current impl is to allow the legacy usage of the annotation with a custom value:
  1. When ingressClass is present (by default now through the included values), the ingressClassName property is set and the annotation would be filtered out if it is present
  2. If ingressClass is empty (or set to null), the annotation would not be filtered, allowing it to be set with whatever value it has defined
  • also update the readme file
  • and preferably also the changelog, certainly when this would introduce a breaking change if we overwrite the ingressAnnotations with another value.

Will do

Copy link
Contributor

@gert-glassee gert-glassee left a comment

Choose a reason for hiding this comment

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

Best to indicate in the changelog that this could be a breaking change on AWS where we need to set ingressClass to ALB.

@RVanhuysseXenit RVanhuysseXenit merged commit 7ef3a08 into master Oct 2, 2024
1 check passed
@RVanhuysseXenit RVanhuysseXenit deleted the KANSUP74-12_ingressClass branch October 2, 2024 09:21
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.

4 participants