langfuse / langfuse-k8s

Community-maintained Kubernetes config and Helm chart for Langfuse
https://langfuse.com
MIT License
44 stars 24 forks source link

Add hostname to config values of the chart #12

Closed Eulenator closed 3 months ago

Eulenator commented 3 months ago

I had to configure the hostname to 0.0.0.0 to be able to reach the langfuse ui outside of my aks cluster with a port-forwarding. I think it makes sense to add hostname as a configurable value in the helm chart.

CLAassistant commented 3 months ago

CLA assistant check
All committers have signed the CLA.

marcklingen commented 3 months ago

@mautini what do you think about this change? To me it seems like this is technically not necessary as this could be passed also via langfuse.additionalEnv. Currently Langfuse does not default to this, is there a downside to defaulting to 0.0.0.0?

mautini commented 3 months ago

Hi Marc,

As we discussed in a previous PR, I think additionalEnv is a great way to avoid releasing a new version of the Helm Chart each time you want to add a new ENV variable in Langfuse. That being said, I think that creating dedicated variables in the Helm Chart for common Langfuse ENV variables is more explicit and could be favored (it also depends on the time you can allocate to validate these MRs, as it could be time-consuming).

Regarding the default value, I think I would prefer to set it to the Langfuse default value (I don’t know it in this case), as this avoids discrepancies when installing Langfuse via Helm vs. docker-compose?

Also, I added some comments to the PR.

marcklingen commented 3 months ago

Hi Nicolas,

thanks a lot for your quick review of this change. I agree that we should not add this explicitly to avoid replicating the documentation of the container in this helm chart. The docs already include a full list of available optional ENV variables.

I agree. Let's avoid changing the default to 0.0.0.0 outside of a major version, as it could be a breaking or security-impacting change for some users.

Let's keeping this pull request open to consider merging it as part of the upcoming v3 change, which will also involve a more significant change to this Helm chart to reflect new services.

EDIT: I've added a more explicit reference to the docs to the readme.

marcklingen commented 3 months ago

@Eulenator can you pass hostname via langfuse.additionalEn in the meantime or is there another reason to add hostname explicitly to the chart?

Eulenator commented 3 months ago

@Eulenator can you pass hostname via langfuse.additionalEn in the meantime or is there another reason to add hostname explicitly to the chart?

Sure. I was not aware of this option. Without setting the hostname to 0.0.0.0, the Langfuse server was virtually unusable in a Kubernetes cluster.

marcklingen commented 3 months ago

Then let's close this issue as additionalEnv is the way to go here