Chart: Add alias IngressClass resources.

[slimm609]

Add support for multiple ingress class names on a single controller

What this PR does / why we need it:

This allows supporting multiple ingress class names in a behind a single controller. The use case is that in production, there are multiple different ingress classes like internal and external. However, in limited development environments there is no "internal" vs "external" so this allows using a single controller for resource usage reductions and still provides the "look" of multiple ingress classes.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

How Has This Been Tested?

This was tested with nginx 1.9.3. The controller supports it but the helm chart did not.

ingress-ext-controller-86f58fdc98-vw6xk controller I1103 19:49:48.687926       7 store.go:440] "Found valid IngressClass" ingress="test/test-service1" ingressclass="nginx-ext"

ingress-ext-controller-86f58fdc98-vw6xk controller I1103 19:49:48.687398       7 store.go:440] "Found valid IngressClass" ingress="test/test-service2" ingressclass="nginx-int"

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	43fc43e
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/655238ccc84b7c0008aa1ee2

[k8s-ci-robot]

Welcome @slimm609!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @slimm609. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[slimm609]

cc: @rikatz

[Gacko]

inb4: I'm just a regular contributor, so please don't get me wrong, I'm just asking questions which came into my mind when reading your PR and added some chances for improvement.

I basically like the idea of having multiple IngressClasses configured with one chart! What I'm not understanding is the need for the tplvalues.render template.

As far as I understand this complex bit of code, it's mostly used for templating some value - the commonLabels in this particular case - in a specific context defined by scope. Right now we are not making use of this and there are way easier approaches to do so.

What's wrong about just making ingressClassResource an array and iterate over it? Yes, it's breaking, but it's simple, clean and keeps the possibility of configuring IngressClasses separately, especially their parameters. Of course the controllerValue could not be part of the array then.

[slimm609]

inb4: I'm just a regular contributor, so please don't get me wrong, I'm just asking questions which came into my mind when reading your PR and added some chances for improvement.

I basically like the idea of having multiple IngressClasses configured with one chart! What I'm not understanding is the need for the tplvalues.render template.

As far as I understand this complex bit of code, it's mostly used for templating some value - the commonLabels in this particular case - in a specific context defined by scope. Right now we are not making use of this and there are way easier approaches to do so.

What's wrong about just making ingressClassResource an array and iterate over it? Yes, it's breaking, but it's simple, clean and keeps the possibility of configuring IngressClasses separately, especially their parameters. Of course the controllerValue could not be part of the array then.

I was able to rework to avoid the template issue.

Switching to an array is a breaking change which I was trying to avoid as this likely won't be a common use-case. Contour supports the same thing in a similar way as well

[Gacko]

Yes, sure, using an array would be breaking and I think the way you're implementing it right now is the best compromise.

Maybe this can be converted to an array in a later major release.

For now you could also just add a controller.extraIngressClassResources object array or just a controller.ingressClassResource.extraNames string array to prevent the string splitting and have the feature in a rather strictly typed schema.

WDYT?

[slimm609]

OK. Updated to use the values from extraNames. This also makes the "default" flag still explicit

---
# Source: ingress-nginx/templates/controller-ingressclass.yaml
# We don't support namespaced ingressClass yet
# So a ClusterRole and a ClusterRoleBinding is required
apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  labels:
    helm.sh/chart: ingress-nginx-4.8.3
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: test
    app.kubernetes.io/version: "1.9.4"
    app.kubernetes.io/part-of: ingress-nginx
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: controller
  name: nginx
spec:
  controller: k8s.io/ingress-nginx
---
# Source: ingress-nginx/templates/controller-ingressclass.yaml
apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  labels:
    helm.sh/chart: ingress-nginx-4.8.3
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: test
    app.kubernetes.io/version: "1.9.4"
    app.kubernetes.io/part-of: ingress-nginx
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: controller
  name: nginx-external
spec:
  controller: k8s.io/ingress-nginx
---
# Source: ingress-nginx/templates/controller-ingressclass.yaml
apiVersion: networking.k8s.io/v1
kind: IngressClass
metadata:
  labels:
    helm.sh/chart: ingress-nginx-4.8.3
    app.kubernetes.io/name: ingress-nginx
    app.kubernetes.io/instance: test
    app.kubernetes.io/version: "1.9.4"
    app.kubernetes.io/part-of: ingress-nginx
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: controller
  name: test2
spec:
  controller: k8s.io/ingress-nginx
---

[slimm609]

@tao12345666333 this should be ready to go now

[rikatz]

/ok-to-test

@Gacko as you've been also helping on this review, is this fine for you?

[slimm609]

also please add a test case for that

@cpanato Do you have a reference of where there is a test case for the existing ingressClass?

[Gacko]

/assign

[Gacko]

Regarding the tests: Have a look into this directory. This manifest could be a good starting point.

I know we already changed the name of the property once. But what do you think about aliases instead of extraNames? This is just a suggestion, I'm fine with both of them.

Regarding the new IngressClass manifest: Could we either name it controller-ingressclass-aliases.yaml or controller-extra-ingressclasses.yaml (mind the dash) instead of controller-extraingressclass.yaml then?

[Gacko]

Suggested change

	{{- if and (.Values.controller.ingressClassResource.enabled) (.Values.controller.ingressClassResource.extraNames) -}}
	{{- if and .Values.controller.ingressClassResource.enabled .Values.controller.ingressClassResource.aliases -}}

[Gacko]

Left a last bunch of suggestions. If you agree on naming the new variable aliases, please also rename the controller-extraingressclass.yaml to controller-ingressclass-aliases.yaml. That would be nice! Apart from that we should be ready to merge. 🙂

[Gacko]

Suggested change

	# -- Additional ingressClass names
	extraNames: []
	# -- Aliases for the ingressClass
	aliases: []

[Gacko]

Suggested change

	| controller.ingressClassResource.extraNames | list | `[]` | Additional ingressClass names |
	| controller.ingressClassResource.aliases | list | `[]` | Aliases for the ingressClass |

You probably need to re-run helm-docs here.

[Gacko]

Suggested change

	{{- range $v := .Values.controller.ingressClassResource.extraNames }}
	{{- range $v := .Values.controller.ingressClassResource.aliases }}

[Gacko]

/kind feature
/priority backlog
/triage accepted
/hold

[Gacko]

/retitle Chart: Add alias IngressClass resources.

[Gacko]

Closing this in favor of #11109.

/close

[k8s-ci-robot]

@Gacko: Closed this PR.

In response to this:

Closing this in favor of #11109.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.