fix typo and duplicated declaration of ingressclasses #8591

spaced · 2022-03-02T13:47:08Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:
Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fix typo and duplicated declaration of ingressclasses

k8s-ci-robot · 2022-03-02T13:47:16Z

Hi @spaced. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

cristicalin · 2022-03-02T17:13:43Z

.../kubernetes-apps/ingress_controller/ingress_nginx/templates/clusterrole-ingress-nginx.yml.j2

@@ -16,15 +16,16 @@ rules:
  - apiGroups: [""]
    resources: ["services"]
    verbs: ["get", "list", "watch"]
-  - apiGroups: ["extensions","networking.k8s.io"]


I expect this was in place for some beta API compatibility. Did you check the impact of removing the extensions apiGroup?

The stable API(networking.k8s.io) of ingress has been added since v1.19, and the beta API of ingress has been removed since Kubernetes v1.22 as https://kubernetes.io/docs/reference/using-api/deprecation-guide/#ingress-v122
On Kubespray, kube_version_min_required is v1.21.0.
So I think it is fine to remove this extensions.

Thanks @oomichi for the clarification!

cristicalin · 2022-03-02T17:15:32Z

.../kubernetes-apps/ingress_controller/ingress_nginx/templates/clusterrole-ingress-nginx.yml.j2

    verbs: ["get", "list", "watch"]
  - apiGroups: [""]
    resources: ["events"]
    verbs: ["create", "patch"]
  - apiGroups: ["extensions","networking.k8s.io"]
    resources: ["ingresses/status"]
    verbs: ["update"]
-  - apiGroups: [networking.k8s.io"]
+  - apiGroups: ["networking.k8s.io"]


The list of verbs and apiGroups is the same as the one above. It looks cleaner to just group resources in the entry above and delete this typo'ed line.

That is a nice point.
I agree with @cristicalin here

@cristicalin @oomichi
nope, i do not agree: the one above is only for ingress/status and update.
I recommend to not reinvent the wheel and stay close to https://github.com/kubernetes/ingress-nginx/blob/main/deploy/static/provider/baremetal/1.21/deploy.yaml#L209 for easier merge in the future.

Thank you for your explanation, that makes sense for easy maintenance.

/ok-to-test
/lgtm

oomichi

/cc @oomichi

oomichi · 2022-03-02T23:29:13Z

.../kubernetes-apps/ingress_controller/ingress_nginx/templates/clusterrole-ingress-nginx.yml.j2

@@ -16,15 +16,16 @@ rules:
  - apiGroups: [""]
    resources: ["services"]
    verbs: ["get", "list", "watch"]
-  - apiGroups: ["extensions","networking.k8s.io"]


The stable API(networking.k8s.io) of ingress has been added since v1.19, and the beta API of ingress has been removed since Kubernetes v1.22 as https://kubernetes.io/docs/reference/using-api/deprecation-guide/#ingress-v122
On Kubespray, kube_version_min_required is v1.21.0.
So I think it is fine to remove this extensions.

oomichi · 2022-03-02T23:30:57Z

.../kubernetes-apps/ingress_controller/ingress_nginx/templates/clusterrole-ingress-nginx.yml.j2

    verbs: ["get", "list", "watch"]
  - apiGroups: [""]
    resources: ["events"]
    verbs: ["create", "patch"]
  - apiGroups: ["extensions","networking.k8s.io"]
    resources: ["ingresses/status"]
    verbs: ["update"]
-  - apiGroups: [networking.k8s.io"]
+  - apiGroups: ["networking.k8s.io"]


That is a nice point.
I agree with @cristicalin here

cristicalin · 2022-03-10T10:15:54Z

ping @spaced

Could you please address the comments above so we can merge this contribution?

spaced · 2022-03-10T13:55:07Z

Could you please address the comment

i already commented, we should not

The list of verbs and apiGroups is the same as the one above.

this is not the same: it has resources: ["ingresses/status"]

I recommend to stay close what upstream provides: https://github.com/kubernetes/ingress-nginx/blob/main/deploy/static/provider/baremetal/1.21/deploy.yaml#L209 for easier merge in the future.

cristicalin · 2022-03-13T07:34:00Z

Sorry, I missed your comment @spaced , thanks for the explanation.

/approve

k8s-ci-robot · 2022-03-13T07:34:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, spaced

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:

~~OWNERS~~ [cristicalin]

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

…s#8591)

fix typo and duplicated declaration of ingressclasses

3cdf1d2

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 2, 2022

k8s-ci-robot requested review from EppO and oomichi March 2, 2022 13:47

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 2, 2022

cristicalin reviewed Mar 2, 2022

View reviewed changes

oomichi reviewed Mar 2, 2022

View reviewed changes

k8s-ci-robot requested a review from oomichi March 2, 2022 23:31

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 Mar 11, 2022

k8s-ci-robot assigned oomichi Mar 11, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2022

k8s-ci-robot merged commit 2b79be6 into kubernetes-sigs:master Mar 13, 2022

sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022

fix typo and duplicated declaration of ingressclasses (kubernetes-sig…

35064a1

…s#8591)

oomichi mentioned this pull request May 28, 2022

Release Proposal v2.19 #8885

Closed

LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 29, 2023

fix typo and duplicated declaration of ingressclasses (kubernetes-sig…

352ac78

…s#8591)

LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Oct 23, 2023

fix typo and duplicated declaration of ingressclasses (kubernetes-sig…

ad68094

…s#8591)

spaced deleted the fix-nginx-controller-clusterrole branch June 10, 2024 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix typo and duplicated declaration of ingressclasses #8591

fix typo and duplicated declaration of ingressclasses #8591

spaced commented Mar 2, 2022 •

edited by floryut

Loading

k8s-ci-robot commented Mar 2, 2022

cristicalin Mar 2, 2022

oomichi Mar 2, 2022

cristicalin Mar 3, 2022

cristicalin Mar 2, 2022

oomichi Mar 2, 2022

spaced Mar 9, 2022

oomichi Mar 11, 2022

oomichi left a comment

oomichi Mar 2, 2022

oomichi Mar 2, 2022

cristicalin commented Mar 10, 2022

spaced commented Mar 10, 2022 •

edited

Loading

cristicalin commented Mar 13, 2022

k8s-ci-robot commented Mar 13, 2022

fix typo and duplicated declaration of ingressclasses #8591

fix typo and duplicated declaration of ingressclasses #8591

Conversation

spaced commented Mar 2, 2022 • edited by floryut Loading

k8s-ci-robot commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomichi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristicalin commented Mar 10, 2022

spaced commented Mar 10, 2022 • edited Loading

cristicalin commented Mar 13, 2022

k8s-ci-robot commented Mar 13, 2022

spaced commented Mar 2, 2022 •

edited by floryut

Loading

spaced commented Mar 10, 2022 •

edited

Loading