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

add ingressClass option to helm chart - back compatibility with ingress.class annotations #8136

Merged
merged 1 commit into from
Feb 6, 2022

Conversation

tombokombo
Copy link
Contributor

What this PR does / why we need it:

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation only

Which issue/s this PR fixes

Hi, according networking.k8s.io/v1 specification for ingressClassName

DESCRIPTION:
IngressClassName is the name of the IngressClass cluster resource. The
associated IngressClass defines which controller will implement the
resource. This replaces the deprecated kubernetes.io/ingress.class
annotation. For backwards compatibility, when that annotation is set, it
must be given precedence over this field.

and

https://kubernetes.github.io/ingress-nginx/#i-have-more-than-one-controller-in-my-cluster-and-already-use-the-annotation
I have more than one controller in my cluster and already use the annotation ?
No problem. This should still keep working, but we highly recommend you to test!

it gives impression that ingress.class annotation should still work "out-of-box". But is not, we need to specify --ingress-class argument for controller, which must be provided as extraArgs. I suggest to bring back .Values.controller.ingressClass as it was in 3.X and older charts. This could be done at least for chart version in 4.0.X for smoother chart upgrades and nondisruptive migration to IngressClass and respective IngressClassName field.
Thx

How Has This Been Tested?

Checklist:

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

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 12, 2022
@k8s-ci-robot
Copy link
Contributor

@tombokombo: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority labels Jan 12, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @tombokombo. 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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 12, 2022
@k8s-ci-robot k8s-ci-robot added the area/helm Issues or PRs related to helm charts label Jan 12, 2022
@longwuyuan
Copy link
Contributor

@tombokombo , thank you for your contribution. It helps.

(1) Please create a issue first and move the description you have sown here to that issue.
(2) Link that issue to this PR
(3) Write tests and show your test config, data & results for other use cases after your sugested change.

The topic you have raised could be much more complicated in terms of the design, implementation, current state of the code and future roadmap. Potentially there could be discussions involved and comments from others. Hence tracking all that on a issue will help.

If you are a developer, please also look at the code and make any comments on the current implementation, with regard to the change you are sugesting. Thanks.

@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

@longwuyuan let me intervene on this one :D

We've been struggling a lot with this ingress class things, and how this thing reflected to the users.

Right now I understand that the proposed PR rollbacks something that we "deprecated" on past and "undeprecated" when we added the "ingress-class-by-name" so it is something missing on the helm charts.

The ingress class problem is much more complicated that it seems, and I've been really thinking on rollback all the deprecations we followed and say "ok, we are going to support again annotations OR ingressClass and wont say that annotations are deprecated anymore".

This probably is going to solve a lot of headaches we've had recently. This discussion was ongoing on sig-network in Slack, and my PERSONAL opinion is that although we need an ingressclass object, probably our timing wasn't good and it is what it is now, with a lot of broken deployments :) As far as I can see from GatewayAPI development, this wont be a problem

I wont be present on our next ingress-nginx meeting as I have a community event here, BUT I really want to bring this discussion back.

@strongjz @ElvinEfendi we should discuss this, I know the effort for ingressclass from kubernetes api and from our side was huge, but we need to make it better for our users (I'm saying from my side specifically!)

@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

/ok-to-test
/approve
/lgtm
About the PR, I'm not opposed to this.

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 16, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2022
@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 16, 2022
@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

@tombokombo we need to update the helm docs now when a new option is added. Please see: https://github.com/kubernetes/ingress-nginx/blob/main/RELEASE.md#d-edit-the-valuesyaml-and-run-helm-docs

@longwuyuan as we are using now helm-docs it would be good if in Makefile we have a "make helm-docs" that downloads the release from https://github.com/norwoodj/helm-docs/releases/tag/v1.5.0 (or the latest available) and generates all for the users. Can you tackle on that with someone?

Thanks!

@rikatz
Copy link
Contributor

rikatz commented Jan 16, 2022

/approve
Please fix the helm docs and we can merge it :)

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2022
@longwuyuan
Copy link
Contributor

@tombokombo , based on @rikatz comments, let me know if you want help with running helm-docs on your clone, of your fork, to commit a updated /charts/ingress-nginx/README.md in your clone. That will potentially circumvent some confusion we have right now in the helm-docs part of the CI pipeline and thus get this PR ready for merging.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 18, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 18, 2022
@tombokombo
Copy link
Contributor Author

@longwuyuan now, it should be Ok.

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 18, 2022

@rikatz , I think we need to use that bot option to squash commits here. Unless @tombokombo you want to squash commits in your fork

@tombokombo
Copy link
Contributor Author

@longwuyuan ok, squashed.

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 18, 2022

Thank you very much for squashing. Thank you very much for contribution.
@tombokombo , I am not sure if you are expected to bump chart version. That is part of the RELEASE process, afaik. @rikatz is it ok for this PR to bump chart version ?

Also, have been keen on improving the changelog so can you retitle your PR to something more co-herent like "add ingress.class annotation to helm chart" or something of that kind.

@tombokombo tombokombo changed the title helm: add ingress.class add ingressClass option to helm chart - back compatibility with ingress.class annotations Jan 18, 2022
@tombokombo
Copy link
Contributor Author

@longwuyuan title changed and regarding bumping chart vers, some ci expects it, some not, but as I remember correctly, I've checked some merged PR here and there was bump...

@longwuyuan
Copy link
Contributor

True. The PRs created as part of the RELEASE.md process will bump the Chart version as documented in RELEASE.md.
AFAIK, if Chart version is bumped by you, then I have to check if that results in the release of a new Chart version or the bumped version just sits there until the "documented" RELEASE process occurs

@rikatz
Copy link
Contributor

rikatz commented Jan 27, 2022

Let's release this :) I'm good with the bump

Just be aware the badge on the README.md seems to be pointing to the wrong chart version (or I may be wrong, which happens a lot when I'm reviewing PRs on cell phone)

@longwuyuan feel free to follow on this and lgtm

/approve
Thanks

@longwuyuan
Copy link
Contributor

ok. @tombokombo please rebase if needed and run helm-docs once again, in your local clone. @rikatz comment is very important about the badge in README.md and AFAIK, that badge updates only and only after a certain process. I forgot how the badge changed (was after helmdocs or was it after release not sure).

Please feel free to ping me on slack and I can help do the needful to get this merged asap. This will help a lot of users.

@longwuyuan
Copy link
Contributor

@tombokombo rebase is needed now

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2022
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2022
@tombokombo
Copy link
Contributor Author

@longwuyuan rebased

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 30, 2022 via email

Signed-off-by: tombokombo <[email protected]>
@tombokombo
Copy link
Contributor Author

@longwuyuan docs fixed

@longwuyuan
Copy link
Contributor

longwuyuan commented Jan 30, 2022 via email

@bmv126
Copy link

bmv126 commented Jan 30, 2022

@rikatz / @tombokombo
So now with this change is controller.ingressClassByName still needed in values.yaml. If yes, when do we use it and it's use case?

@tombokombo
Copy link
Contributor Author

tombokombo commented Jan 30, 2022

@rikatz / @tombokombo So now with this change is controller.ingressClassByName still needed in values.yaml. If yes, when do we use it and it's use case?

I will try to explain:

ingressClass option is way how to support Ingress object annotations.

IngressClassByName option is related to IngressClass object, there you are defining relation between ingressClassName field in Ingress object and ingress-controller. Controller should be specified in ingressClass spec.controller field, but some 3.X versions of helm chart has this field hard-coded and unfortunately this field is immutable ( https://github.com/kubernetes/ingress-nginx/blob/helm-chart-3.34.0/charts/ingress-nginx/templates/controller-ingressclass.yaml#L21 ). Some deployments are stucked with this ingressClass definition and they need ingressClassByName till they somehow solve this ( eg. delete and recreate ingressClass object ).

@rikatz
Copy link
Contributor

rikatz commented Feb 6, 2022

/lgtm
Thanks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rikatz, tombokombo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 4b4895b into kubernetes:main Feb 6, 2022
@tao12345666333 tao12345666333 mentioned this pull request Feb 27, 2022
rchshld pushed a commit to joomcode/ingress-nginx that referenced this pull request May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/helm Issues or PRs related to helm charts cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants