fix(argo-cd): Remove invalid matches from default GRPCRoute

[downfa11]

Description

Fixes #3550

Fixes the default GRPCRoute configuration to comply with Gateway API specification.

Problem

The current GRPCRoute template generates invalid resources when server.grpcroute.enabled: true is set without custom rules.

At least one of service or method must be a non-empty string in GRPCMethodMatch.

The previous default configuration violated this requirement.

Changes

• Changed server.grpcroute.rules default value from invalid example to empty array [] in values.yaml
• Removed matches section entirely from the default rule in grpcroute.yaml template
• When matches is omitted, Gateway API spec defines that all gRPC traffic is matched, which is the desired default behavior

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.
• I have created a separate pull request for each chart according to pull requests
• My build is green (troubleshooting builds).

[yann-soubeyrand]

Hello,
Are you sure this PR is working? I tried to set an empty service and I get the following error spec.rules[0].matches[0].method: Invalid value: "object": service must only contain valid characters (matching ^(?i)\.?[a-z_][a-z_0-9]*(\.[a-z_][a-z_0-9]*)*$). Leaving empty rules seems to work.

[downfa11]

@yann-soubeyrand I'm really sorry for the confusion. You're right.

Now when server.grpcroute.rules is not specified, it generates:

# Source: argo-cd/templates/argocd-server/grpcroute.yaml
apiVersion: gateway.networking.k8s.io/v1
kind: GRPCRoute
metadata:
  name: argocd-server-grpc
  namespace: default
  labels:
    helm.sh/chart: argo-cd-9.1.2
    app.kubernetes.io/name: argocd-server
    app.kubernetes.io/instance: argocd
    app.kubernetes.io/component: server
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/part-of: argocd
    app.kubernetes.io/version: "v3.2.0"
spec:
  parentRefs:
    - name: gateway
      namespace: default
  rules:
    - backendRefs:
        - name: argocd-server
          port: 443
          weight: 1

[yann-soubeyrand]

I’m wondering if this GRPCRoute can work at all and if it’s even necessary:
https://gateway-api.sigs.k8s.io/api-types/grpcroute/?h=grpc#cross-serving

Implementations that support GRPCRoute must enforce uniqueness of hostnames between GRPCRoutes and HTTPRoutes. If a route (A) of type HTTPRoute or GRPCRoute is attached to a Listener and that listener already has another Route (B) of the other type attached and the intersection of the hostnames of A and B is non-empty, then the implementation must reject Route A. That is, the implementation must raise an 'Accepted' condition with a status of 'False' in the corresponding RouteParentStatus.

In general, it is recommended that separate hostnames be used for gRPC and non-gRPC HTTP traffic. This aligns with standard practice in the gRPC community. If however, it is a necessity to serve HTTP and gRPC on the same hostname with the only differentiator being URI, the user should use HTTPRoute resources for both gRPC and HTTP. This will come at the cost of the improved UX of the GRPCRoute resource.

[yann-soubeyrand]

I confirm that I was able to make GRPC work on the HTTPRoute by adding appProtocol: kubernetes.io/h2c on the http port of the argocd-server service. I guess this would be necessary for the GRPCRoute to work anyway.

[downfa11]

@yann-soubeyrand Thank you for checking it.

As you mentioned, I also think using HTTPRoute is more practical. But judging from the issue that was raised, there does seem to be some demand.

• a clear intention that "this route handles gRPC traffic."
• support for features like GRPCMethodMatch
• consistency with cases like the argo-rollouts chart and argo-cd’s ingressGrpc

[yann-soubeyrand]

I don’t have a strong opinion on whether the chart should provide a GRPCRoute or not, but it should be clearly stated in the doc that only the HTTPRoute must be deployed if one wants to serve HTTP and GRPC on the same domain (as stated in the Gateway API documentation).

[hostalp]

Unfortunately, Traefik controller can't handle both HTTP & GRPC requests using just one protocol for some reason.
If appProtocol isn't set on the service, only HTTP traffic works.
If appProtocol is set to kubernetes.io/h2c, only GRPC traffic works.
It's similar to what's described here: https://argo-cd.readthedocs.io/en/stable/operator-manual/ingress/#traefik-v30
Because the protocol can't be set at the HTTPRoute level here (unlike IngressRoute), I worked around it by adding a 2nd argocd-server service named argocd-server-h2c with almost identical configuration to the primary one, just with appProtocol set to kubernetes.io/h2c for the http port:

apiVersion: v1
kind: Service
metadata:
  annotations:
    meta.helm.sh/release-name: argocd
    meta.helm.sh/release-namespace: argocd
  labels:
    app.kubernetes.io/component: server
    app.kubernetes.io/instance: argocd
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: argocd-server
    app.kubernetes.io/part-of: argocd
  name: argocd-server-h2c
  namespace: argocd
spec:
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: 8080
    appProtocol: kubernetes.io/h2c
  - name: https
    port: 443
    protocol: TCP
    targetPort: 8080
  selector:
    app.kubernetes.io/instance: argocd
    app.kubernetes.io/name: argocd-server

And then modifying the httproute to route GRPC requests to that h2c service based on the Content-Type request header:

apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  annotations:
    meta.helm.sh/release-name: argocd
    meta.helm.sh/release-namespace: argocd
  labels:
    app.kubernetes.io/component: server
    app.kubernetes.io/instance: argocd
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: argocd-server
    app.kubernetes.io/part-of: argocd
  name: argocd-server
  namespace: argocd
spec:
  hostnames:
  - hostname
  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: traefik-gateway
    namespace: default
    sectionName: websecure
  rules:
  - backendRefs:
    - name: argocd-server
      port: 80
    matches:
    - path:
        type: PathPrefix
        value: /
  - backendRefs:
    - name: argocd-server-h2c
      port: 80
    matches:
    - path:
        type: PathPrefix
        value: /
      headers:
      - name: Content-Type
        value: application/grpc

However because the service as well as the httproute backendRefs are generated by the chart and the possibilities to add/change the above are currently limited, the only way to get this reasonably working is to disable the generated httproute and define both those resources in extraObjects.

[christianh814]

Wrote on the original issue #3550 (comment)

But It almost works, with the helm chart as-is, but requires some updates. Although I'm using Cilium

[yann-soubeyrand]

@hostalp I think I’ve something working based on your input: #3585.

[hostalp]

@yann-soubeyrand Great, that looks promising. Did you test it at some implementation other than Traefik? It would be good to have some sort of confirmation that it really works with multiple implementations.

Also perhaps update the description in both values.yaml & README.md to make it clear that those http2 parameters are there to enable gRPC (CLI) access via the (main) HTTPRoute - so that their purpose is clear from the description.

[yann-soubeyrand]

@hostalp no, I tested it with Istio only. I think I won’t be able to test it with other implementations soon.

[github-actions]

This pull request 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.