criblio / helm-charts

Repository for Cribl Helm Charts
MIT License
33 stars 38 forks source link

Hardcoded transport `tcp` comms to the leader node #192

Open michaelhyatt opened 2 months ago

michaelhyatt commented 2 months ago

The template seem to hardcode tcp as the protocol. Can you please add support for tls?

https://github.com/criblio/helm-charts/blob/c04136618a64067ecb42088c817872f5b9ea208f/helm-chart-sources/logstream-leader/templates/_pod.tpl#L89 https://github.com/criblio/helm-charts/blob/c04136618a64067ecb42088c817872f5b9ea208f/helm-chart-sources/logstream-leader/templates/_pod.tpl#L91

yenoromm commented 2 months ago

A string value at config.overrideLeaderURL would be ideal. This would provide a very flexible value for also configuring tls.\<settingName> in the url

bdalpe commented 1 month ago

@michaelhyatt @yenoromm You can do this today.

  1. Create a secret (either on your own or use the extraObjects value)
  2. Configure your Helm Values like the following:
    
    config: 
    useExistingSecret: true

envValueFrom:

Bonus setting for extraObjects:

extraObjects:
  - apiVersion: v1
    kind: Secret
    metadata:
      name: cribl-leader-secret
    type: Opaque
    data:
      leaderUrl: "tls://authToken@criblleader:4200?group=foo"  

Extra objects supports a string or object for each array item. You can see more examples of how to use this in https://github.com/criblio/helm-charts/blob/master/helm-chart-sources/logstream-workergroup/tests/extras_test.yaml

yenoromm commented 1 month ago

@bdalpe

If you look at the first comment or the PR, you will notice we are talking about the leader helm chart and not the workergroup chart.

The config.useExistingSecret is well documented and what we use to set the leader url for the workergroup.

bdalpe commented 1 month ago

Sorry, @yenoromm I missed that detail.

You can still do something similar for the leader helm chart by redefining the environment variable. Your new definition in the helm values would win.

https://stackoverflow.com/questions/73443818/precedence-rule-in-container-env-list-with-duplicate-name-key-in-kubernetes

I agree this is not perfect and should be addressed long-term. IMO, the best way to do this would be to remove the current way it's done and replace with user specifying all the details. That would be a major breaking change though.

yenoromm commented 1 month ago

In the PR it supplies the current value if an override value is not supplied. There should be no breaking change only added functionality if the user decides to use it.

We are using Pulumi to handle deployments. The tooling throws an error with duplicate keys even if it isn’t caused by helm directly. Therefore this isn’t a solution for us, other Pulumi shops and possibly even Terraform shops if the underlying provider is what throws the error.