fix(argo-cd): Pass argocd-server's ALB health check

[yu-croco]

Resolves #2552

Checklist:

• I have bumped the chart version according to versioning
• I have updated the documentation according to documentation
• I have updated the chart changelog with all the changes that come with this pull request according to changelog.
• Any new values are backwards compatible and/or have sensible default.
• I have signed off all my commits as required by DCO.
• My build is green (troubleshooting builds).

[yu-croco]

ALB Guide in README is also mentioning this, so I think it's reasonable to change default value, in addition to #2552 . 🙋

[pdrastil]

@yu-croco I would keep NodePort as is. The instance mode is default for AWS load balancer controller. The ClusterIP is needed only for alb.ingress.kubernetes.io/target-type: ip or by this default for the deployment.

[yu-croco]

Thank you for your review.
Then what about adding comments for IP mode ? 🙋

[pdrastil]

@yu-croco it's already there :)

# -- Service type for the AWS ALB gRPC service
## Can be of type NodePort or ClusterIP depending on which mode you are running.
## Instance mode needs type NodePort, IP mode needs type ClusterIP

[yu-croco]

Maybe like this...? 🤔

    # AWS specific options for Application Load Balancer
    # Applies only when `serv.ingress.controller` is set to `aws`
    ## Ref: https://argo-cd.readthedocs.io/en/stable/operator-manual/ingress/#aws-application-load-balancers-albs-and-classic-elb-http-mode
+   # The default value is for `Instance mode`. If you use `IP mode`, please refer https://github.com/argoproj/argo-helm/tree/main/charts/argo-cd#aws-application-load-balancer.
    aws:
      # -- Backend protocol version for the AWS ALB gRPC service
      ## This tells AWS to send traffic from the ALB using HTTP2. Can use gRPC as well if you want to leverage gRPC specific features
+     ## If you use `IP mode` for ALB controller, please configure as `GRPC`.
+     ## For more information: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/target-group-health-checks.html#health-check-settings
      backendProtocolVersion: HTTP2
      # -- Service type for the AWS ALB gRPC service
      ## Can be of type NodePort or ClusterIP depending on which mode you are running.
      ## Instance mode needs type NodePort, IP mode needs type ClusterIP
      ## Ref: https://kubernetes-sigs.github.io/aws-load-balancer-controller/v2.7/how-it-works/#ingress-traffic
      serviceType: NodePort

[updated]
delayed to post the message, resolved in previous message.

[mikutas]

@pdrastil As reported, if protocol version of target group (for gRPC Service) is HTTP2, the health check setting of the TG become invalid and target become unhealthy.
So aws.backendProtocolVersion is not selectable indeed.
I think this is the point of this issue.

On the other hand, serviceType is certainly selectable corresponding to AWS LBC's mode(instance / ip), and it's irrelevant to the issue of unhealthy target.
Therefore, I think the default value of serviceType need not to be changed in this PR and keeping NodePort as default is reasonable as well as AWS LBC's default mode is instance.

[mkilchhofer]

From my experience with ALB (and AWS EKS) serviceType is for sure relevant.
If you use ClusterIP and ALB target-type ip the health checks are handled different than in NodePort together with instance.

• instance by default checks traffic path until nodes
• ip always tries to check directly to the pod and uses / and "traffic port" as the default health-checking parameters

[pdrastil]

@mikutas Thanks for response. @yu-croco Changing the port to GRPC without any changes to service type should be enough.

[mikutas]

@mkilchhofer Thank you, my understanding seems incorrect.
More precicely speaking:

• backendProtocolVersion = HTTP2 and serviceType = ClusterIP (i.e. LBC is ip mode) -> unhealthy (my case)
• backendProtocolVersion = HTTP2 and serviceType = NodePort (i.e. LBC is instance mode) -> healthy (your case)
• backendProtocolVersion = GRPC and serviceType = ClusterIP (i.e. LBC is ip mode) -> healthy (my case)
• backendProtocolVersion = GRPC and serviceType = NodePort (i.e. LBC is instance mode) -> healthy (probably?)

[yu-croco]

Changing the port to GRPC without any changes to service type should be enough.

@pdrastil @mikutas
I'm sorry for my mess... I reverted unnecessary diff in f91e531 . 🙋

[tico24]

lgtm