Azure / application-gateway-kubernetes-ingress

This is an ingress controller that can be run on Azure Kubernetes Service (AKS) to allow an Azure Application Gateway to act as the ingress for an AKS cluster.
https://azure.github.io/application-gateway-kubernetes-ingress
MIT License
672 stars 419 forks source link

AGIC generating random listener names since last week. #962

Open Vishal2696 opened 4 years ago

Vishal2696 commented 4 years ago

Describe the bug AGIC used to generate meaningful listener names such as "fl-<hostname>-<portno>-<public/privateip>". This is what everyone wants. but for the past two weeks AGIC has been generating random listener names such as "fl-5222170d53815f835f3b17c2c8904019". This behavior is seen exactly from the date Azure released a new feature for App gateway whose details can be found in the below link.

https://docs.microsoft.com/en-us/azure/application-gateway/multiple-site-overview

however this has so far not affected the base functioning of AGIC. Ingress yaml I'm using is very normal and not anything new which I'm posting below.

kind: Ingress
apiVersion: extensions/v1beta1
metadata:
  name: myingress
  namespace: mynamespace
  annotations:
    kubernetes.io/ingress.class: azure/application-gateway
    appgw.ingress.kubernetes.io/use-private-ip: "true"
spec:
  rules:
    - host: mydomain.com
      http:
        paths:
          - path: /
            backend:
              serviceName: my-service
              servicePort: 80

Kubernetes Version: 1.15.11

JorTurFer commented 4 years ago

Hi @akshaysngupta , This naming changes between 1.0.1-rc1 and 1.0.1-rc2, in this commit https://github.com/Azure/application-gateway-kubernetes-ingress/commit/2105e9e7ca00c6e40aeb7d83636996865e49a8e5. I suppose that the reason is that is written in the commit message. If wildcard is allowed in hostname, using the hostname in the listener name lost its importance. Am I right?

Vishal2696 commented 4 years ago

@JorTurFer Well okay then atleast make listener name something more meaningful such as listerner1,listerner2, etc.... Meaningful names is definitely a lot better than some random number. I'm sure this is easily doable. I would suggest this format f1-listener<1>-portno-<public/private>

JorTurFer commented 4 years ago

Hi @vishal8k , Sorry but I started my contributions in this repo 2 or 3 weeks ago, I don't have the context of the previous changes before. Maybe @akshaysngupta or @3quanfeng can give you more information about this and have more strong opinion about the listener's naming rules.

akshaysngupta commented 4 years ago

@vishal8k Thanks for the feedback. I think it will definitely make more sense to show readable names.

Please feel free to contribute to the repo if you want this earlier. We will try to take this up in some future sprint.

MarcelT-NL commented 4 years ago

+1 I have 30 or so listeners x2 (http redirect and https) and very hard to find the correct one

eugen-ananin commented 3 years ago

+1 Just as Marcel wrote, my situation is similar. It is really difficult to debug issues with cryptic names.

MarcelT-NL commented 3 years ago

Proposed solution: could we specify a name for the listener in the deployment file?

Or alternatively, name the listener to the domain that AGIC is provisioning? Like "listener-sub.domain.com-443"

akshaysngupta commented 3 years ago

@MarcelT-NL I think it might be easier to name the listener using the older convention.

This is the place where listener name is generated. https://github.com/Azure/application-gateway-kubernetes-ingress/blob/master/pkg/appgw/internaltypes.go#L146

Since AppGW started supporting multiple hostnames on a listener which may have a wild card (*, ?) which are not allowed in listener names, Naming logic will have to accommodate this fact.

https://github.com/Azure/application-gateway-kubernetes-ingress/blob/master/pkg/appgw/internaltypes.go#L64

This is the PR that made the code change for naming. https://github.com/Azure/application-gateway-kubernetes-ingress/commit/2105e9e7ca00c6e40aeb7d83636996865e49a8e5#diff-de7e79b139b4cf70236f52f028d8cf27dfd09116e444eda8027347aa4ed933af

Please feel free to contribute. This is a good starting issue.

MarcelT-NL commented 3 years ago

@MarcelT-NL I think it might be easier to name the listener using the older convention. @akshaysngupta Either way is fine, just as long as the names are readable and do not change for every release. E.g. adding a random number to the name would kill some of our scripts (like the random names do now).

But to make things more flexible to the future, a bespoke name as an annotation would be great!

robs commented 3 years ago

@akshaysngupta

Was the main reason for changing the naming convention the change from using hostname to hostnames? Is it as simple as using the original convention but using hostnames[0] if hostnames.length > 0 or were there other reasons?

zorg-the-blue commented 3 years ago

Any update on this issue?

I'm in agreement with several comments, but the main genesis for my concern is the way App Gateway handles wildcard processing and AGICs limitation in allowing us to work within those confines.

As others have said on linked issues, order matters when it comes to the new wildcard logic. It appears for the time being it is using first match, not best match and without the ability for us to manipulate the order, we are left with no work around within this solution.

Any annotation we can get to override the random, but repeatable, GUID name would be very helpful, not only from a way to manipulate the order but also for readability when troubleshooting. Attempting to remove and redeploy the ingress K8 object creates the same name so trying to move a listener to the end of the line based on different deployment attempts does not work.

Thank you.

david-garcia-garcia commented 7 months ago

Handling 100 listeners who's name is a GUID is guid or a random number makes troubleshooting more difficult.

robs commented 7 months ago

FWIW: We gave up on this and switched to NGINX ingress. It handles the rollout deployments much better too.