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:

#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

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]

[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 assign rikatz for approval. 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

[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	7c2b047
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/6613855142bea800089da0b3

[strongjz]

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"

This sounds like trying to get around a cluster admin not allowing deployments.

[strongjz]

So it seems this is meant to implement what the docs talk about below

Namespace-scoped parameters help the cluster operator delegate control over the configuration (for example: load balancer settings, API gateway definition) that is used for a workload. If you used a cluster-scoped parameter then either:

the cluster operator team needs to approve a different team's changes every time there's a new configuration change being applied.
the cluster operator must define specific access controls, such as RBAC roles and bindings, that let the application team make changes to the cluster-scoped parameters resource.

https://kubernetes.io/docs/concepts/services-networking/ingress/#ingressclass-scope

[strongjz]

/kind feature
/prority backlog
/ok-to-test

[yong-jie-gong]

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"

This sounds like trying to get around a cluster admin not allowing deployments.

Yes, exactly, with current k8s implementation,IngressClass is always cluster visible resource, in most of shared cluster, cluster admin don't allow the namespaced application to access any cluster level resource.

[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