Features from ParamountCommerce

[glaberge]

Hey guys!

Gabriel from Paramount Commerce. Saw your issue on the official repo and since we heavily use this chart as well, we had already forked and added some stuff as well as merging some of the PRs. I've integrated some of these changes into this PR (all non disruptive). We would love to collaborate with you in maintaining this chart.

Please don't hesitate to request changes and provide feedback.

Looking forward to cooperate with you

Addresses these issues/PRs from upstream chart:

• Configurable default ingress service http_status:400  cloudflare/helm-charts#70
• Pod topology spread constraints cloudflare/helm-charts#88
• Optional protocol config option added cloudflare/helm-charts#34
• feat(cloudflare-tunnel): add option to disable default 404 cloudflare/helm-charts#69
• [cloudflare-tunnel] Service and ServiceMonitor resources cloudflare/helm-charts#57

[gooseops]

I've got eyes on it. Will hopefully be able to review this week. My initial thoughts are that the service template definition is probably fine, but I'd rather not introduce CRDs (ServiceMonitor from Prometheus Operator) into the chart that are dependent on operators outside our control.

[gooseops]

I would also be interested in links to github issues (if any) from the cloudflare repo that these changes solve. Thanks!

[glaberge]

Updated with issues and PR from upstream chart.

As for the serviceMonitor, while I understand the comment, Prometheus is pretty much the de facto monitoring system today and these options are included in almost every mainstream charts. These are disabled by default, don't add any overhead or complexity. The reasoning for adding those is to make this chart appealing to most users and prevent having a bunch of diverging forks.

[gooseops]

lgtm.

Agree that prometheus servicemonitor is fine because of industry standard implementation. Thank you for the additions!!

adding comments first

[gooseops]

@adamrlynch @glaberge because this removes the default - service: http_status:404 line, it might have an edge case breaking change where users of the chart are currently not configuring their catchall service and now need to. If that's the case, their new chart would not have a catchall service causing unintended behavior.

Can we either make that backwards compatible by joining that service in someway or looks for it to be defined, or make this version change a breaking change? Not sure how Renovate checks for breaking changes.

[gooseops]

@glaberge
For example, we might have it templated in the config map to specifically define a catchall service that has the default http_status:404 and that gets appended to the cloudflare ingress on the configmap after the cloudflare.ingress value.

Something like this quasi code
configmap.yaml

ingress:
      {{- with .Values.cloudflare.ingress }}
      {{- toYaml . | nindent 6 }}
      {{- end }}
      - service: {{- .Values.cloudflare.catchall_service }}

and then
values.yaml

cloudflare:
...

...
  ingress: []
    # The first rule proxies traffic to the httpbin sample service named web-server at port 80
    # - hostname: tunnel.example.com
    #  service: http://web-service:80
    # This rule sends traffic to the built-in hello-world HTTP server. This can help debug connectivity
    # issues. If hello.example.com resolves and tunnel.example.com does not, then the problem is
    # in the connection from cloudflared to your local service, not from the internet to cloudflared.
    # - hostname: hello.example.com
    #   service: hello_world
  catchall_service: "https_status:404"