Support namespaced ingress without accessing the IngressClass

[yong-jie-gong]

…ect and using the annotation.

so it is better support namespaced ingressClass without accessing the IngresClass object and using the annotation.
suggestions:

1. IngressController needn't cluster level permission to access the IngressClass for namespaced Ingress
2. consumer drop annotation "kubernetes.io/ingress.class" from ingress
3. Consumer set the ingressClassName by ingress.Spec.IngressClassName
4. IngressController accept the incoming ingress object when
   1. IngressController has permission to IngressClass, keep the current implementation.
   2. IngressController dont' have permission to access the IngressClass but ingress.Spec.IngressClassName is equals to the ingress class name specified by CLI parameter "--ingress-class"

What this PR does / why we need it:

with latest ingress/ingressClass design, there is no way to run application without requesting read permission on cluster level resource "IngressClass"

This is how issue happens
==As k8s administrator==

1. create the namespace for application "demoapp1" to be installed

kubectl create ns demoapp1

2. create the role and rolebinding for application "demoapp1"

kubectl create role demoapp1-admin --resource="*" --verb="*"

3. genenate the kubeconfig file "demoapp1-kubeconfig.yaml" from role "demoapp1-admin"

===As application administrator===

1. get the kubeconfig file of "demoapp1-admin" from k8s administrator
2. install demoapp1 such as "helm install demoapp1 demoapp1.tgz --kube-config demoapp1-kubeconfig.yaml"
   in demoapp1, there is some ingresses as below

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: ingress-class-name-no-perm
  namespace: ingress-nginx
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /
spec:
  ingressClassName: nginx

in demoapp1, there is one ingress-controller deployed which watch all of ingress whose ingressClassName is "nginx". this ingress-controller should start and run without any error

as shown above, there is no cluster role binding for "application administrator" to install "demoapp1", it means any attempt to access k8s cluster resource will be denied

unfortunately, k8s reference doesn't provide clear guidance on how to valid the .spec.ingressClassName, so most of ingress-controller implementation will try to read the IngressClass object from .spec.ingressClassName.
obvisously, it will fail

in big shared k8s cluster, k8s administrator don't want to shared cluster level resource permission to applications running the namespace. so typically, they just provide least privileged kubeconfig for application installation

#11222 #11222

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

Which issue/s this PR fixes

ingress-controller/ingress can works without read permission on cluster reosurce "IngressClass"

fix #11222

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 unit and/or e2e tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

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]

Hi @yong-jie-gong. 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.

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	9d3afe4
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/670779ff0149610008475eea

[longwuyuan]

• I can't find documentation that describes installing namespace scoped instance

• Before Kubernetes v1.24, there used to be attempts and use-cases of namespace scoped installation

• Even though you mentioned that some cluster-admins do not allow users to access cluster-wide resources, the upstream K8S Ingress API based design of this controller involves to access to cluster-wide resources. So I don't think its a improvement to change the the behavior of the controller to restrict access to a namespace

• There are far too important and huge number of users who use the annotation for the ingressClassName, particularly cert-manager. So this proiect has to continue support for the annotation

• I don't see the practical benefit of the change you suggest here because I don't see how a ingress-controller is appropriate to be running in a cluster without the cluster-admin's approval/consent & co-operation. The PR you submitted does not have a description of the solution. At least describe the entire solution in small details so that a valid case is presented to the reader

• I see a hard change just to 2 go files, without any consideration to how it will impact a user's experience and tests to show the working of the changes.

• Does your changes impact the rest of the controller's features like --default-ssl-certificate etc,

[yong-jie-gong]

• I can't find documentation that describes installing namespace scoped instance
• Before Kubernetes v1.24, there used to be attempts and use-cases of namespace scoped installation
• Even though you mentioned that some cluster-admins do not allow users to access cluster-wide resources, the upstream K8S Ingress API based design of this controller involves to access to cluster-wide resources. So I don't think its a improvement to change the the behavior of the controller to restrict access to a namespace
• There are far too important and huge number of users who use the annotation for the ingressClassName, particularly cert-manager. So this proiect has to continue support for the annotation
• I don't see the practical benefit of the change you suggest here because I don't see how a ingress-controller is appropriate to be running in a cluster without the cluster-admin's approval/consent & co-operation. The PR you submitted does not have a description of the solution. At least describe the entire solution in small details so that a valid case is presented to the reader
• I see a hard change just to 2 go files, without any consideration to how it will impact a user's experience and tests to show the working of the changes.
• Does your changes impact the rest of the controller's features like --default-ssl-certificate etc,

@longwuyuan thanks for quick response.
in most practical deployment, there are two typcially deployment model

1. customer provide the ALB/NLB with ingress-controller
   • for this case, applicatio can simply disable the application shipped nginx-ingresss-controller and create their Ingress with IngressClassName="customer defined IngressClass"
2. customer provide the ALB/NLB only without ingress-controller
   1. k8s cluster is dedicated for specific application
      • for this case, cluster admin don't mind to grant cluster level resource to application. application can create IngressClass and deploy the nginx-ingress-controller with --controller
   2. k8s cluster is shared for many applications
      • for this case, cluster admin is very sensitive to grant cluster level permission to any application. with current k8s design for IngressClass scope. none of them works for namespaced Ingress. only choice for applications is to stay on ingress annotation solution to define what ingressclass belongs to.

with statement above, if applications are deployed in different customer environment secenario 1, scenario 2.2, all of Ingress objects in applications have to be created with IngressClassName(ingreess.spec.IngressClassName) for some customers or annotations(ingress.metadata.annotaiton.<ingress-classs>) for some other customer. besides, from k8s 1.24, it continue to print warnings if any ingress contains annnotation "kubernetes.io/ingress.class".

Regarding namespace scoped instance, before k8s 1.24, Ingress object is pure the Namespace objects and not assoicate with cluster level resource IngressClass by attribute Ingress.Spec.IngresssClassName.
Regarding impacts, I didn't see some other features will be impacted

[longwuyuan]

@yong-jie-gong I think you completely missed the key aspects I pointed out.

• It is very clear that you have a need to install this controller and not create/use any cluster-scope resources. I think it will be great if it becomes possible. But this is not possible currently and its also not possible with the trivial change you proposed. This is mostly because such a use case has to be optional. Like using a flag .

• I think the project has wanted to make it possible to provide the namespace scoped controller instance installation for a long time. But it never came to starting to work on that because the circumstances have changed beyond control. One part is the details I listed above.

• You mentioned about "Customer" and "Providing ALB/NLB". Niether of these factors are a basis for making decision of namespace scoped controller. ALB is NOT supported by this controller. And NLB needs to be automatically created when the service of --type LoadBalancer is created when anyone installs the controller. Does not matter if customer created the LB or non-customer creates the LB. An external-ip gets used and routing becomes possible from outside to inside the cluster so such a change has to involve the cluster-admin, regardless of someone likes it or does not like it.

• Using the field ingress.spec.ingressClassName is required for all users. A choice is available to complicate this by using the watchIngressWithoutClass option. But it is not recommended. It is a exception when a users does not have the option of using the ingressClassName field.

• Projects like cert-manager reported to this project that they have to consistently use the annotation for the ingressClass as that behaviour has to be persistent across all ingress-controllers. THis is why this project still supports the annotation. Hence it is a exceptional use case

So I don't think its a improvement to change the code of the project itself permanently to namespace scoped controller install. If you are installing this project's ingress-controller and the cluster-admin is not aware that a external-ip address is provisioned to input traffic into the cluster from outside the cluster, then that is a breach of trust and should not be allowed to happen.

And then I glanced at your code changes. I am not a developer but from what I understand, your changes do not help other users of this project and your changes are not even implemented with best practices. For example you did not care to even write a test or describe what will happen to --default-ssl-certificate flag when the cert is not in the same namespace as the controller.

[yong-jie-gong]

@yong-jie-gong I think you completely missed the key aspects I pointed out.

• It is very clear that you have a need to install this controller and not create/use any cluster-scope resources. I think it will be great if it becomes possible. But this is not possible currently and its also not possible with the trivial change you proposed. This is mostly because such a use case has to be optional. Like using a flag .

• Do you mean add one CLI-Parameter like --ignore-ingress-class-validation=true to support this scenario?

• I think the project has wanted to make it possible to provide the namespace scoped controller instance installation for a long time. But it never came to starting to work on that because the circumstances have changed beyond control. One part is the details I listed above.
• You mentioned about "Customer" and "Providing ALB/NLB". Niether of these factors are a basis for making decision of namespace scoped controller. ALB is NOT supported by this controller. And NLB needs to be automatically created when the service of --type LoadBalancer is created when anyone installs the controller. Does not matter if customer created the LB or non-customer creates the LB. An external-ip gets used and routing becomes possible from outside to inside the cluster so such a change has to involve the cluster-admin, regardless of someone likes it or does not like it.

• --type LoadBalancer works only for customer Edge NLB which has ability to watch the internally maanged k8s cluster. it means it can automatically watch the k8s service endpoint and update its own configuration. typically it works only for very limited cases such as Cloud Provider NLB. but many on-premise customer don't have such NLB. typically they use F5/nginx/apache http as Edge NLB.

• Using the field ingress.spec.ingressClassName is required for all users. A choice is available to complicate this by using the watchIngressWithoutClass option. But it is not recommended. It is a exception when a users does not have the option of using the ingressClassName field.

• Do you mean set watchIngressWithoutClass=true and watchNameNamespace=<current namespace>?

• Projects like cert-manager reported to this project that they have to consistently use the annotation for the ingressClass as that behaviour has to be persistent across all ingress-controllers. THis is why this project still supports the annotation. Hence it is a exceptional use case

• Does it mean the nginx-ingress-controller may stop supporting annotations. without code change and permission on cluster IngressClass, that is only choice to make it work.

So I don't think its a improvement to change the code of the project itself permanently to namespace scoped controller install. If you are installing this project's ingress-controller and the cluster-admin is not aware that a external-ip address is provisioned to input traffic into the cluster from outside the cluster, then that is a breach of trust and should not be allowed to happen.

And then I glanced at your code changes. I am not a developer but from what I understand, your changes do not help other users of this project and your changes are not even implemented with best practices. For example you did not care to even write a test or describe what will happen to --default-ssl-certificate flag when the cert is not in the same namespace as the controller.

• not sure there is misunderstanding, --default-ssl-certificate is used to define what default server certficate/key should be used if ingress doesn't specify the ingress.spec.tls.SecretName. it seems irrlevant to what ingress should be watched. no matter where secrets defined by --default-ssl-certificate is stored, it doesn't impacts what ingress resource should be watched for current nginx-ingress-controller. could you share more details about your concern

[longwuyuan]

I request that you edit the issue-description in Issue #11222 as per below idea

• Assume that the sample application to be deployed is --image nginx:alpine
  kubectl create deployment test0 --image nginx:alpine --port 80

• Assume that the service for this is kubectl expose deployment test0 --port 80

• Now write a ingress resource yaml file for it and keep it ready for use after the clusrter is ready

• Create a minikube cluster

• Fork the project on github

• create a branch and clone

• Make your changes to the code

• Run make dev-env

• Now there will be a cluster ready with your changes to the controller code

• Deploy your app and service and ingress

• Copy/paste all the test and logs and state related info as outputs of commands here on in the issue

• Then I will have more practical ways to copy your fork's branch and do the same and test your changed controller locally

• I can then put the default-ssl-certificate in a different namespace and see how I can configure ingress with TLS but without a cert

• I can then see first hand what you mean by not-using-cluster-ingress-class

[yong-jie-gong]

I request that you edit the issue-description in Issue #11222 as per below idea

• Assume that the sample application to be deployed is --image nginx:alpine
  kubectl create deployment test0 --image nginx:alpine --port 80
• Assume that the service for this is kubectl expose deployment test0 --port 80
• Now write a ingress resource yaml file for it and keep it ready for use after the clusrter is ready
• Create a minikube cluster
• Fork the project on github
• create a branch and clone
• Make your changes to the code
• Run make dev-env
• Now there will be a cluster ready with your changes to the controller code
• Deploy your app and service and ingress
• Copy/paste all the test and logs and state related info as outputs of commands here on in the issue
• Then I will have more practical ways to copy your fork's branch and do the same and test your changed controller locally
• I can then put the default-ssl-certificate in a different namespace and see how I can configure ingress with TLS but without a cert
• I can then see first hand what you mean by not-using-cluster-ingress-class

@longwuyuan As requested, I has updated more information in defect #11222

[yong-jie-gong]

@strongjz @tao12345666333 @rikatz @longwuyuan someone can help review?

[tao12345666333]

/assign
/hold

I will add this to my list and finish the review this week. Thank you

[yong-jie-gong]

/assign /hold

I will add this to my list and finish the review this week. Thank you

@tao12345666333 coudl you get time to check this PR?

[tao12345666333]

I will take a look today. Thank you for your contributions.

[tao12345666333]

Why did you change this?

[yong-jie-gong]

it is the same thing as variable "invalidHost := "noclassforyou""

[Gacko]

Hello @yong-jie-gong!

First of all thank you very much for all the effort you put in this PR!

We as the project maintainers recently had a meeting where we discussed the future of our project. Since we are comparable low on resources, we agreed on going in the direction of streamlining our code base to make is easier to maintain. This also includes sticking to the Kubernetes API given to us rather than building exceptions of it.

We therefore sadly cannot accept your contribution anymore as it is not in alignment with the Ingress API.

You're noting that this annotation is getting deprecated in Kubernetes v1.28. Actually and according to the code this warning is already in their since Kubernetes v1.27 and maybe even longer in other places because the annotation is not the preferred way for even longer already. Still considering it on its own in our code is one thing and we should probably get rid of it. But mixing it with the proper ingressClassName implementation doesn't make it better.

We hope you understand that!

[yong-jie-gong]

Had more discussion in slack channel kubernetes#sig-network and #ingress-nginx-dev
To summarize:
indeed, it is k8s ingress design drawback. we have 4 options:

1. go along the lines of @thockin's "Controller looks up its own namespace and looks for Ingresses that match that pattern. This is entirely under the controller's purvey" in the nginx-ingress-controller, with the caveat that the spec does not specifically support this, it just does not disallow it
   -- This is what this PR is trying to do.
2. don't set the .spec.ingressClassName in the Ingress, and hope that there's no undesired side-effect from the default ingress controller picking up these ingresses.
   -- This one considered as security alert by some k8s vendor such as redhat openshift.
3. accept that the namespace-only concept got broken and ask the cluster admin to create an extra ingressClass per each namespaced app
   -- stop support of namespaced Ingress
4. build an argument for extending the validation on this field to include values that can't possibly be real, and document them as reserved for private implementations, or something. To make this argument we would have to bring the gateway API in alignment. I can't see why doing it for one and not the other would make sense
   -- Need a complicate process and introduce special naming rule for .spec.ingressClassName since .spec.ingressClassName and .metadata.name share the same name convention at this time.

[yong-jie-gong]

/reopen

[k8s-ci-robot]

@yong-jie-gong: Reopened this PR.

In response to this:

/reopen

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-sigs/prow repository.

[linux-foundation-easycla]

• ✅ login: yong-jie-gong / name: emerson_gong (9d3afe4)
• ❌ The email address for the commit (bd7f2e8, c97e62a, a4a5367, dadd1c0, 7c2b047, d14ca86, f7556a3, 2653b8e, 61343bb, 51c58c3, 8ef4db3, 43354dd, cae5005, 24c4ec9, a436a32, 5ff4253) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yong-jie-gong
Once this PR has been reviewed and has the lgtm label, please ask for approval from tao12345666333. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[aojea]

Why option 2 is not valid?

don't set the .spec.ingressClassName in the Ingress

[aojea]

so you ignore IngressClassName for one thing but use it for another thing?

This is not right IMHO, you are changing the semantics of two options, one public API of Ingress and one config of ingress-nginx ... this is a -1 as it is a breaking change for two things

[yong-jie-gong]

==Before change, if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, ingress-controller mandate the IngressClass validation. it mean fails directly if no read permission on IngressClass resource

==After change
if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, stay as before. if IgnoreIngressClass is true && ( ing.Spec.IngressClassName == <ingress class defined by cli "--ingress-class" ), ignore the IngressClass resource matchness check

[longwuyuan]

How to set IgnoreIngressClass to true or false ? Is there a helm chart related values-file key-value pair ?

[Gacko]

How to set IgnoreIngressClass to true or false ? Is there a helm chart related values-file key-value pair ?

You can always pass it as controller.extraArgs. I never got why users would add new values to the chart for stuff that's just being passed directly in to the arguments. But that's not to be discussed here, just my 2 cents. 😅

if IgnoreIngressClass is false and ing.Spec.IngressClassName is not empty, stay as before. if IgnoreIngressClass is true && ( ing.Spec.IngressClassName == <ingress class defined by cli "--ingress-class" ), ignore the IngressClass resource matchness check

And that's what @aojea means: .spec.ingressClassName and what is should be used for is defined by a public API. We are not going to break this API.

https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#ingressspec-v1-networking-k8s-io

ingressClassName is the name of an IngressClass cluster resource. Ingress controller implementations use this field to know whether they should be serving this Ingress resource, by a transitive connection (controller -> IngressClass -> Ingress resource). Although the kubernetes.io/ingress.class annotation (simple constant name) was never formally defined, it was widely supported by Ingress controllers to create a direct binding between Ingress controller and Ingress resources. Newly created Ingress resources should prefer using the field. However, even though the annotation is officially deprecated, for backwards compatibility reasons, ingress controllers should still honor that annotation if present.

[yong-jie-gong]

Why option 2 is not valid?

don't set the .spec.ingressClassName in the Ingress

empty .spec.ingressClassName works functionally, but potentially, if multiple ingress-controllers are enabled to watch ingress with empty .spec.ingressClassName, it is not expected, that is why openshift consider it security alert https://access.redhat.com/solutions/7017955

[Gacko]

Please take this comment into account: #11223 (comment).

Additionally and as the API docs already say: Even though the annotation is officially deprecated, for backwards compatibility reasons, ingress controllers should still honor that annotation if present.

We do this and Kubernetes right now is just warning you about usage, even in latest releases. Also neither Kubernetes nor this project are responsible for what a third party is warning you about. As in: I don't see a reason to change the code just because RedHat warns you about stuff. As long as you know your use case and setup best, you should always be able to run it the way you want it to and not the way someone else, who might not know your setup, tells you to. With the current implementation you could easily define a dummy ingressClassName, so RedHat is not complaining anymore, and still enable --ignore-ingress-class and use the old annotation.

I totally understand your use case and I also get that Ingress API is not covering it. IMHO the solution for overcoming API design flaws should never be breaking API compliance of an API implementation but rather pushing to fixing the API in newer versions.

The issue you're addressing here is pretty clear:

In the past there has been and still nowadays there is the ingress.class annotation users can use to directly connect an Ingress resource to an Ingress controller.

Ingress resources are namespaced and as long as your controller is only watching Ingress resources of a specific namespace, one could easily run the controller in a quite isolated mode without access to any cluster-scoped resources.

Now .spec.ingressClassName and IngressClass, which is cluster-scoped, kick in. These were designed as an official successor of the ingress.class annotation.

Since migrating from the annotation to .spec.ingressClassName according to how the API was designed would require access to cluster-scoped resources, this is clearly breaking your use case of a namespace-local Ingress controller. This is because Ingress API and Ingress controllers were meant to be provided by your cluster administrator for the whole cluster and not by yourself on a namespace level. Again: I know this does not always match reality.

We are aware of this design flaw and we also don't like it. But still this is no reason to break Ingress NGINX' compliance with the Ingress API.

From here, we have several possibilities to proceed: At the moment some really encouraged folks are pushing forward Gateway API. Now is the perfect time to make sure it is not running into the same design flaws. So one could contribute to Gateway API and make sure it covers use cases like yours in the future.

If this does not resonate with you, you could still approach SIG Network and talk about changes in the Ingress API.

Anyway: Merging this PR is none of the possibilities and I therefore will close it again.