[argo-cd] Add ability to specify backend service in ingress for argocd-server

jdomag commented 1 year ago

Is your feature request related to a problem?

Template for argo-cd/argocd-server/ingress.yaml is build in a way that: 1) Each path specified in server.ingress.path has the same type of pathType. I would like to specify pathType for each path on the list 2) It's not possible to specify backend service name and port for default path. My use case: We use AWS ALB as the entry point of our k8s cluster. We use istio ingress gateway in k8s to receive all the ingress traffic and distribute it across the different services. This setup requires to create an ingress object with a given name e.g. argocd.mydomain.com and point it to the istio ingress gateway service. Current helm chart doesn't allow to overwrite the backend service name or port - it always uses argocd-server service.

Related helm chart

argo-cd

Describe the solution you'd like

1) pathType value is under array of server.ingress.paths instead of server.ingress 2) Add following fields to values.yaml: server.ingress.paths[].backend.service.name: - if not specified argocd-server service is used server.ingress.paths[].backend.service.port: - Can be string or int. If not specified use argocd-server service as default

Describe alternatives you've considered

No response

Additional context

No response

jdomag commented 1 year ago

If this request sounds reasonable I can provide PR.

ankitcharolia commented 1 year ago

@jdomag : we are using argocd with istio ingress gateway. We are using ingress gateway for receiving all the ingress traffic (we do't use Ingress Nginx controller). It would be awesome if official helm chart supports creating Ingress gateway and virtualService with some values.yaml parameter.

pdrastil commented 1 year ago

@ankitcharolia For this please use extraObjects, otherwise this will lead to managing custom resources for Istio, Linkerd, Traefik, Gloo, Kong, etc. to provide additional ingress controller integration.

jdomag commented 1 year ago

@ankitcharolia I agree with @pdrastil, for VirtualService objects, extraObjects: [] should be used. However for ingress, it would be nice to reuse ingress.yaml, however it's not possible with the current structure hence I proposed enhancement. Moving prefixType into the paths: [], sounds like a breaking change though as people would need to adjust the values.yaml they currently use.

pdrastil commented 1 year ago

@jdomag - I agree it's not possible and I have draft version with more Ingress cleanup. There are more things that went sideways when trying to support multiple ingress contollers (AWS, GKE, Nginx passthrough / TLS and support for gRPC via extra ingress). Take a look into this draft that was part of refactoring I've done few months back and I'm pushing individual parts into this repo. Would that be something that would be helpful to prioritise?

Domain for Argo CD should be reusable for all components via global.domain
Additional paths are weird as they targets only Argo CD server and not any sidecar with extra services
Prefix is not templated
If domain is not provided Argo CD tooks over all traffic via wildcard route (not great for security)
TLS section should pick up configured secrets automatically
Routing is incorrect (--insecure flag should change port to 80 and not use 443)

pdrastil commented 1 year ago

@jdomag Btw I've been poking in current implementation - you can use extraPaths to have your custom backend. The ingress.paths[] are used only for server.

jdomag commented 1 year ago

@pdrastil You are right, I can set .Values.server.ingress.paths: [] and use extraPaths instead. However it doesn't solve my issue as for istio, the ingress gateway is in the different namespace than argo-cd and k8s requires the ingress and the backend service that it's referencing to, to be in the same namespace. I will submit PR that allows people to control the namespace in which ingress should be installed and this should solve this case.

pdrastil commented 1 year ago

@jdomag For Istio this will not work anyway because Istio have terrible support for native Ingress as the design of this service mesh forces you to rethink your cluster networking completely and use their CRDs for everything. You can always target Istio gateway in different namespace via gateways: <namespace>/<gateway-name> syntax.

jdomag commented 1 year ago

@pdrastil it's a common case that people use combination of native ingress and isitio CRDs (virtual service + gateway). See example and the explanation why it may be useful:

https://maytan.work/aws-alb-with-istio/ or https://rtfm.co.ua/en/istio-external-aws-application-loadbalancer-and-istio-ingress-gateway/

It works as expected, but it comes with a price - you need to create both ingress and virtual service for every service you expose via ALB+istio and the ingress must be in the same namespace as istio ingress gateway pods. This is what we currently use, but due to lack of namespace parameter in ingress we had to move all the ingress definition to extraObjects in the argo-cd helm chart.

pdrastil commented 1 year ago

@jdomag Yeah I also run this for a few clients (actually I run Traefik for normal stuff like Argo CD + Istio for enterprise deployments that needs that). That's why I recommended to provide VirtualService via extra object and point it to your gateway in other namespace. See gateways docs here: https://istio.io/latest/docs/reference/config/networking/virtual-service/. In article you've posted the Ingress resource is created only for Istio control plane in AWS to provision and configure ALB. The services that should use Istio as a gateway are using VirtualService resources.

jdomag commented 1 year ago

@pdrastil Those examples in deed created single ingress for istio ingress gateway, however exposing any service via ALB + istio ingress gateway will require ingress object for each of those service to update DNS (we use external DNS) - since istio-ingress-gateway in that scenario uses NodePort service type, external DNS won't be able to create an entry for e.g. argo-cd-server based on VirtualService. This case is described here: https://rtfm.co.ua/en/istio-shared-ingress-aws-alb-helm-chart-with-conditions-istio-and-externaldns/#ExternalDNS_doesnt_create_records_for_Istio_Gateway_and_VirtualService_No_endpoints_could_be_generated

I don't like the workaround to hardcode the ALB url in the VirtualService as shown in the example, hence creating ingress object for each service seems to be more elegant way.

pdrastil commented 1 year ago

@jdomag At that point the cleaner approach would be to use ingress classes + External DNS annotations. See here: https://istio.io/latest/docs/tasks/traffic-management/ingress/kubernetes-ingress/#configuring-ingress-using-an-ingress-resource

jdomag commented 1 year ago

@pdrastil I haven't found a way to use ingressClass with multiple istio ingress gateways - we have public and private gatewyas, how to bind an Ingress to a specific ingress gateway so it updates the externalIP of the ingress and allow externalDNS to update the DNS entry?

pdrastil commented 1 year ago

@jdomag I don't think this is possible as Istio support of native Ingress is really limited. However I've found something that might help you out with VirtualService + External DNS.

https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/istio.md

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

argoproj / argo-helm