camunda / camunda-platform-helm

Camunda Platform 8 Self-Managed Helm charts
https://docs.camunda.io/docs/self-managed/overview/
Apache License 2.0
74 stars 137 forks source link

[ISSUE] When zeebeGateway.ingress.rest.enabled contextPath and paths are causing issues. #2282

Closed drodriguez-305 closed 1 month ago

drodriguez-305 commented 2 months ago

Describe the issue:

When zeebeGateway.ingress.rest.enabled is set to true we have constraints that the zeebeGateway.contextPath and zeebeGateway.ingress.rest.path need to be the same.

Actual behavior: I then set zeebeGateway.contextPath= / and zeebeGateway.ingress.rest.path= /

zeebe-gateway:
  replicas: 1
  affinity: {}
  contextPath: /
  ingress:
    rest:
      enabled: true
      host: zeebe-gateway.dev.cosmdream.com
      tls:
        enabled: true
        secretName: "dev-cosmdream-com-tls"
      path: /

when running it the zeebe-gateway pod doesn't get in the ready state. Looking at the deployment we can see there is an additional / in the readiness probe.

        readinessProbe:
          failureThreshold: 5
          httpGet:
            path: //actuator/health/readiness

Then I edited and removed the /

        readinessProbe:
          failureThreshold: 5
          httpGet:
            path: /actuator/health/readiness

After that you need to kill the pod and confirm the correct one runs.

Expected behavior: There should be no extra / and shouldn't have to edit the deployment to get a working configuration.

How to reproduce:

Logs:

Related: SUPPORT-23427

Please note: Without the following info, it's hard to resolve the issue and probably it will be closed.

daniel-ewing commented 2 months ago

Since you have added the constraint that when zeebeGateway.ingress.rest.enabled: true the zeebeGateway.contextPath and zeebeGateway.ingress.rest.path need to be the same, then when you fix this, you should also fix it by setting the default value of zeebeGateway.ingress.rest.path: /. It is a bit strange that I do not set zeebeGateway.contextPath in my values.yaml, so it has the default value "/", so now I have to add to my values.yaml a new value that I never needed before: zeebeGateway.ingress.rest.path: /, just to satisfy this new constraint that was added. The constraint should already be satisfied by the values in the default values.yaml.

megglos commented 1 month ago

To conclude, double slashes // in URI paths are not sanitized to single slashes / by spring. You may be able to configure that overriding some defaults but that may cause issues on spring security which apparently is not supporting sanitizing either. I wouldn’t go for that due to the risk of side-effects down the line.

See also https://github.com/spring-projects/spring-framework/issues/15845#issuecomment-453407951. They also mention that according to RFC3986 double slash characters are actually illegal see here.

Thus we need to have some handling in the chart to avoid double-slashes. Effectively setting zeebeGateway.contextPath=/ at all seems redundant to me, setting a real path zeebeGateway.contextPath=/path may be problematic if a trialing slash is added though? Can you sanitize this on helm chart side?