ingress-nginx icon indicating copy to clipboard operation
ingress-nginx copied to clipboard

enabling internal LB is not usable because needs 2 ingressclasses on one controller

Open cdenneen opened this issue 2 years ago • 32 comments

Currently when you set controller.service.internal.enabled=true it creates 1 Deployment/Daemonset and 1 IngressClass (1 ServiceMonitor if enabled) but 2 Services

Since setting internal.enabled=true creates 2 services:

NAME                                 TYPE           CLUSTER-IP      EXTERNAL-IP                                                                     PORT(S)                      AGE
ingress-nginx-controller             LoadBalancer   172.20.196.30   LB1-external.elb.us-east-1.amazonaws.com   80:30731/TCP,443:30157/TCP   196d
ingress-nginx-controller-admission   ClusterIP      172.20.131.47   <none>                                                                          443/TCP                      196d
ingress-nginx-controller-internal    LoadBalancer   172.20.67.174   LB2-internal.elb.us-east-1.amazonaws.com   80:32359/TCP,443:32765/TCP   163m
ingress-nginx-controller-metrics     LoadBalancer   172.20.88.89    LB3-internal.elb.us-east-1.amazonaws.com   10254:32552/TCP              163m

You currently have no way to distinguish when using something like external-dns which service to use since the ingressClassName is the same being it's a single controller. So either would need a way for DNS to understand which service to bind (CNAME) to since both load balancers should route Ingress resources fine or you'd need 2 controllers/ingressclass to be created.

If creating DNS manually you could setup your dns to point to the ingress-nginx-controller DNS or the ingress-nginx-controller-internal but no way to automate this as of now without some sort of annotation which will select the proper SVC dns to use.

cdenneen avatar Jun 07 '23 20:06 cdenneen

/triage accepted /assign

longwuyuan avatar Jun 07 '23 23:06 longwuyuan

@cdenneen I think you are right about the difficulty to use external-dns in this scene. Lets wait an see if someone with ideas comments here.

longwuyuan avatar Jun 10 '23 15:06 longwuyuan

@longwuyuan I thought about opening issue to fix in external-dns but then thought this would be issue with any other integration as well cert-manager, etc. Guess trying to understand the purpose of the chart enabling the external and internal services if they can’t be differentiated by IngressClass or something.

cdenneen avatar Jun 11 '23 21:06 cdenneen

Once again you are very right and this is a bug. The history is that before v1.22 of K8S, this was super helpful due to non-existence of the need to use ingressClassName. This and to some extent the namespace scoping of a controller are pending but on the backburner, because of some serious priorities needing attention.

I think the right thing to do is test creating a second ingressclass manually to be managed by same controller. I will try

/assign /triage accepted /remove -kind feature /kind bug

/retitle enabling internal LB is not usable because needs 2 ingressclasses on one controller

/priority important-long-term

@ cc @rikatz as needing advise on the direction this will take

longwuyuan avatar Jun 12 '23 00:06 longwuyuan

@longwuyuan: The label(s) priority/important-long-term cannot be applied, because the repository doesn't have them.

In response to this:

Once again you are very right and this is a bug. The history is that before v1.22 of K8S, this was super helpful due to non-existence of the need to use ingressClassName. This and to some extent the namespace scoping of a controller are pending but on the backburner, because of some serious priorities needing attention.

I think the right thing to do is test creating a second ingressclass manually to be managed by same controller. I will try

/assign /triage accepted /remove -kind feature /kind bug

/retitle enabling internal LB is not usable because needs 2 ingressclasses on one controller

/priority important-long-term

@ cc @rikatz as needing advise on the direction this will take

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 avatar Jun 12 '23 00:06 k8s-ci-robot

/priority important-longterm

@cdenneen just to be very clear, I think its a bug because of the important of letsencrypt and external-dns integration. By itself, just having one single ingressclass for all ingress objects, that use a single controller with both internal & external service, is expected to work by itself, per-se, because the value of the field ingress.spec.rules.host needs to match the internal or the external service's external-ip and hence the corresponding internal or external dns-name.

I can't test this, but I am beginning to think that if your ingress object's host field maps to a internal DNS server zone or a external DNS server zone, actually would work just fine. Let me know your thoughts. i wish you actually tried this with host field values being appropriately configured and then reported results.

longwuyuan avatar Jun 12 '23 04:06 longwuyuan

@longwuyuan you can have same zone with different zone ids (internal and external) so the zones would be the same.

Like externalhosf.example.com And internalhost.example.com

cdenneen avatar Jun 12 '23 11:06 cdenneen

@cdenneen I will have to loop up bind/other nameserver docs etc to understand what zone-id is.

But the relevance is that the value of the field ingress.spec.rules.host will resolve to the external-ip field of the service of --type LoadBalancer and hence facilitate 2 distinct ip-subnets for ingress routing. Hence the ingress-nginx controller is not broken or buggy, per se.

longwuyuan avatar Jun 12 '23 11:06 longwuyuan

DNS docs would call if split zone. AWS calls them zone ids

cdenneen avatar Jun 12 '23 11:06 cdenneen

/remove-kind bug /kind feature

longwuyuan avatar Jun 14 '23 23:06 longwuyuan

Can anyone please explain how to configure 2 ingress classes using helm chart 4.6.1. https://artifacthub.io/packages/helm/ingress-nginx/ingress-nginx/4.6.1

anuja-kulkarni avatar Jun 21 '23 15:06 anuja-kulkarni

@anuja-kulkarni https://kubernetes.github.io/ingress-nginx/user-guide/k8s-122-migration/#how-can-i-easily-install-multiple-instances-of-the-ingress-nginx-controller-in-the-same-cluster

longwuyuan avatar Jun 21 '23 15:06 longwuyuan

@anuja-kulkarni https://kubernetes.github.io/ingress-nginx/user-guide/k8s-122-migration/#how-can-i-easily-install-multiple-instances-of-the-ingress-nginx-controller-in-the-same-cluster

Thanks, I created one controller, when I tried installing another internal ingress controller in same namespace I get below error, can you tell the correct way to set annotations or if there is some other issue there?

Error: INSTALLATION FAILED: rendered manifests contain a resource that already exists. Unable to continue with install: ClusterRole "ingress-nginx-1" in namespace "" exists and cannot be imported into the current release: invalid ownership metadata; annotation validation error: key "meta.helm.sh/release-namespace" must equal "ingress-nginx-2": current value is "ingress-nginx-ns"

only one ingress class that I created first is present currently, check output: kubectl get ingressclasses
NAME CONTROLLER PARAMETERS AGE nginx-two example.com/ingress-nginx-2 46m

command I ran to install second nginx ingress controller: helm install ingress-nginx-1 ingress-nginx/ingress-nginx --version 4.6.1
--namespace ingress-nginx-2
--set controller.electionID=nginx-two-leader
--set controller.ingressClassResource.name=nginx-one
--set controller.ingressClass=nginx-one
--set controller.ingressClassResource.controllerValue="example.com/ingress-nginx-1"
--set controller.ingressClassResource.enabled=true
--set controller.ingressClassByName=true
--set controller.service.internal.enabled=true
--set-string controller.service.internal.annotations."networking.gke.io/load-balancer-type"="Internal"

Thanks in advance.

anuja-kulkarni avatar Jun 21 '23 17:06 anuja-kulkarni

@anuja-kulkarni Look at the next section in the same docs page

longwuyuan avatar Jun 21 '23 18:06 longwuyuan

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

github-actions[bot] avatar Jul 22 '23 01:07 github-actions[bot]

You can try to use external-dns.alpha.kubernetes.io/target annotation to specify internal LB.

firaxis avatar Aug 06 '23 07:08 firaxis

@firaxis since the ingress item specifies an ingressClass which will select the DNS record for the ingress-nginx controller how would it know which service (internal or external) to add. In order to use the annotation recommended I would need to know the DNS for the internal one, which can change. There would be no way to "query" that item and a static entry wouldn't be helpful in any sort of automated fashion.

@longwuyuan So while having 1 controller and 1 ingress class with 2 services (enabling the internal one in the helm release) technically would work the issue comes down to how to update in automated fashion the Ingress resource would need tell the controller which service to return:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: app-ingress
  namespace: default
spec:
  ingressClassName: nginx
  rules:
  - host: app.domain.com
    http:
      paths:
      - backend:
          service:
            name: app-svc
            port:
              number: 80
        path: /
        pathType: Prefix
  tls:
  - hosts:
    - app.domain.com
status:
  loadBalancer:
    ingress:
    - hostname: external-loadbalancer.amazonaws.com

maybe some sort of optional ingressClassService:

spec:
  ingressClassName: nginx
  ingressClassService: internal

or some sort of optional annotation:

metadata:
  annotations:
    nginx.ingress.kubernetes.io/internal: true
spec:
  ingressClassName: nginx

I'm actually more in favor of the latter with the annotation allowing the controller to return which service (loadbalancer) to return.

cdenneen avatar Oct 07 '23 20:10 cdenneen

@cdenneen I need to test to wrap my head around the use-case you describe. But deciding factors are clear. If request is internal, then it will not be resolved on the internet and hence will resolve to a LB destination, that has been created with cloud-provider-specific-annotation for internal LB. So I could be wrong but I suspect there is no question at all about which service to return because the request will use a destination FQDN that resolves to only the internal LB or only to the external-LB

longwuyuan avatar Oct 10 '23 11:10 longwuyuan

The 2 services have the annotation for internal or not. The Ingress resources however don’t have that annotation so it’s the controller either ingresses.go or status.go that tell it which to use. So something needs to tell the controller which one to return to the ingress status.

cdenneen avatar Oct 10 '23 12:10 cdenneen

Only other option I could foresee would be if internal service is enabled then a corresponding IngressClass would need to be defined that allowed for this routing to happen.

cdenneen avatar Oct 10 '23 12:10 cdenneen

my apologies but AFAIK, the trend has been to install distinct instances of the controller for internal and external, as per docs.

Additionally, that internal+external LB from a single instance of the controller has been a grey area for some time now, as not too many support requests are coming on that.

If I need to think through it, I would say that the ingress resources would have the same ingressClassName field but depending on the value of the host field, the traffic would be routed to the internal or the external LB.

longwuyuan avatar Oct 10 '23 16:10 longwuyuan

@longwuyuan I could spin up multiple releases for this but it wouldn't scale very well. The purpose of the "grey area" in my opinon would be to have an external and internal for each controller. Also your thoughts on the host field in many cases does not resolve (nothing in DNS) until external-dns populates it. In which case it would populate on the hostname returned in the Ingress.status.

As far as not scaling (my point here is):

  • In order to offer internal/external you'd need to spin up a release for each (2 releases).
  • If you want to offer ALB and NLB then you'd now need 4 releases (internal and external offering for each).
  • I'm sure there could be specific subnets/PCI compliance, etc. needs so now we are talking about even more releases.

In a perfect world the service.internal offering would allow you to have 1 release for each of these cases. So in my case it would allow me to have a release for ALB (internal/external) and a release for NLB (internal/external) rather than 4 releases.

I agree the single ingressClassName should be able to handle the external/internal service offering which would bring us back to using an annotation of something like: nginx.ingress.kubernetes.io/internal: true in order for the controller to return to Ingress.status.loadBalancer.ingress[0].hostname the proper service DNS name to return.

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: app-ingress
  namespace: default
spec:
...
status:
  loadBalancer:
    ingress:
    - hostname: external-loadbalancer.amazonaws.com

cdenneen avatar Oct 10 '23 17:10 cdenneen

@cdenneen the conversation is now cyclic. So I suggest we gather info live on screen share ;

  • if you are willing then please ping me on slack
  • we need raw data on what happens on AWS when you just use the values file or flag for internal+external
  • based on what we find, I can comment further
  • idea is to test only and only the "controller.service.internal.enabled" key in the values file

longwuyuan avatar Oct 11 '23 03:10 longwuyuan

After screen-share and discussion in dev channel on slack, the question is about using external-dns to work with a hostname that resolves to the internal-lb. @cdenneen is going to try manual CNAME record pointing to internal-LB and see if the ingress status field picks up the internal LB hostname

longwuyuan avatar Oct 12 '23 18:10 longwuyuan

@longwuyuan so the manual entry resolves fine but the external LB is still the one that returns in the Ingress status

cdenneen avatar Oct 12 '23 18:10 cdenneen

I think you ought to make one more test but this time, do not create the ingress until the CNAME has propagated . The test is to check the status field, when the hostname used is resolving to a already propagated dns record.

This test will give us data to go back to that conversation on slack.

longwuyuan avatar Oct 13 '23 01:10 longwuyuan

+1 This would be very useful for us as well. As it stands today, the option in the Helm chart to create an additional internal service isn't helpful because there is no logic to update the watched ingresses' .status.loadBalancer.ingress with the internal NLB's hostname instead of the internet-facing one. I hope that this additional context will be helpful.

Our use case is as follows:

  • We have a single public zone in AWS Route53, call it example.company.com.
  • We have dev and prod VPCs, each with a private Route53 zone for the same TLD example.company.com.
  • We have one EKS cluster in each VPC.
  • In each EKS cluster, we run 2 deployments of external-dns: 1 that filters for internal ingresses/services and updates only the private Route53 zone for that VPC, and 1 that filters for internet-facing ingresses/services and updates the shared public zone (dev apps get hostnames with -dev suffix added so there is no conflict between dev and prod apps in the public zone).
  • We want to use the same hostnames in the private and public zones, so that the same host can be requested regardless of whether the traffic originates from inside OR outside the VPC; the only difference is whether the internet-facing or the internal LB host is returned for the DNS query, but both LBs will route to the same targets.
  • In addition to convenience (not needing to know when to use internal vs. external hostnames), this also avoids routing otherwise internal traffic over public internet, which costs more with AWS. In our case requests originating from inside the VPC resolve DNS queries to hosts inside the VPC, thus the route stays all within the VPC (as it should).

Since the ingress-nginx controller does not have any logic to map ingresses to the internal service instead of the external one, all of our ingresses created with class nginx get the same internet-facing NLB as their .status.loadBalancer.ingress[].hostname. External DNS uses that value for both the public and the private zone, instead of properly mapping an internal ingress to the internal service/NLB hostname.

I would also add that what @cdenneen is suggesting, using an annotation to tell the controller which service to map to the ingress, is in line with what controllers like AWS Load Balancer Controller are doing--an annotation of alb.ingress.kubernetes.io/scheme (ingresses) or service.beta.kubernetes.io/aws-load-balancer-scheme (services) with values of either internal or internet-facing determines which type of ALB/NLB will be created and mapped to the ingress's status fields.

Happy to help with more context if needed.

nimjor avatar Oct 20 '23 15:10 nimjor

I personally need to think through how one you can use same FQDN in 2 ingress objects, if that is what you meant.

The status field referring to internet-facing LB is a a problem clearly, though. But I tested (with @cdenneen ) and he could get landing page of image nginx:alpine from internal-lb even though status field returned external-lb

longwuyuan avatar Oct 21 '23 07:10 longwuyuan

But I tested (with @cdenneen ) and he could get landing page of image nginx:alpine from internal-lb even though status field returned external-lb

We manually added entry. The issue again here is for external-dns to update the proper hosted zone (public or private) with the proper load balancer address. so you can have 2 ingress (one for public and one for private) that on the backend have same service selector. Only reason for 2 is the 2 dns updates but rest would be same.

cdenneen avatar Oct 21 '23 12:10 cdenneen

For me what this issue comes down to is understanding why the helm chart allows one to create both an external and internal service for the controller, when it seems that only one (the external) will be used. If there is an explanation for this, it would be helpful to hear what that is. As it stands today, sure, we could create a second/custom ingress class for internals and create a second instance of the controller that only watches that class, but the way the helm chart is written, that feels like it should be unnecessary; it seems like we should be able to make use of both external and internal controller services since the chart allows creating them, but we would need a way for the controller to know which service to map to which ingress objects. And for that, as @cdenneen and I have said, it seems like a custom ingress annotation would provide an easy filter.

Looking over the controller code, there would need to be a way to pass both a --publish-service and a --publish-service-internal flag, and then the controller would probably need a syncStatusInternal field added for a second statusSync object, which would get created here if config.PublishServiceInternal != "" (you would pass config.PublishServiceInternal to the status.Config.PublishService field). You would also need to pass a type to the status.Config.IngressLister that has a ListIngresses() func that returns only those ingresses of class "nginx" AND having the custom annotation (something like nginx.ingress.kubernetes.io/internal: "true").

nimjor avatar Oct 23 '23 17:10 nimjor