Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Database ValueFrom #14

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

japan4415
Copy link

@japan4415 japan4415 commented Jul 10, 2024

Close #7

What I changed

  • Modified values.yaml to allow valueFrom to pull values from existing Secrets.
  • Added a process in the helper to create a single ValueFrom JSON that consolidates existing values and Secrets created from the PostgreSQL helm, among others.
    • For details, please refer to the diagram below.
flowchart TD
    subgraph langfuse
        F["Deployment"]
        subgraph values
            subgraph postgresql
                subgraph auth
                    subgraph username
                        F(("ValueFrom(DATABASE_USERNAME)"))
                        G(("Value(DATABASE_USERNAME)"))
                    end
                    subgraph password
                        A(("ValueFrom(DATABASE_PASSWORD)"))
                        B(("Value(DATABASE_PASSWORD)"))
                    end
                end
                C["Secret"]
                H["ConfigMap"]
            end
        end
        subgraph helper
            J(("Value(DATABASE_USERNAME)"))
            E(("Value(DATABASE_PASSWORD)"))
        end
        I["Deployment"]
    end
    subgraph postgres
        D["Secret"]
    end

    A --> E
    B --> C
    C --> E
    D --> E
    G --> H
    H --> J
    F --> J

    E --> I
    J --> I
Loading

@CLAassistant
Copy link

CLAassistant commented Jul 10, 2024

CLA assistant check
All committers have signed the CLA.

@japan4415 japan4415 marked this pull request as draft July 10, 2024 01:38
@japan4415 japan4415 force-pushed the add-existing-secret-option branch from b6f1438 to bb7cf7f Compare July 10, 2024 03:48
@japan4415 japan4415 marked this pull request as ready for review July 10, 2024 05:02
@japan4415 japan4415 force-pushed the add-existing-secret-option branch from bb7cf7f to 2ee5b1b Compare July 10, 2024 05:07
@japan4415 japan4415 force-pushed the add-existing-secret-option branch 6 times, most recently from 3e1069c to b839a38 Compare July 25, 2024 00:26
@japan4415 japan4415 force-pushed the add-existing-secret-option branch from b839a38 to 3c9619a Compare July 25, 2024 00:27
@aschaber1
Copy link

Hey @japan4415, looks pretty cool, could you maybe also add the alternative way of using DATABASE_URL instead of DATABASE_HOST, DATABASE_USERNAME, DATABASE_PASSWORD and DATABASE_NAME?

Source: https://langfuse.com/docs/deployment/self-host#configuring-environment-variables

{{- if not .Values.langfuse.nextauth.secret.valueFrom }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this one is not necessary with {{- if (not .Values.langfuse.nextauth.secret.valueFrom) }} at the beginning of the file

labels:
{{- include "langfuse.labels" . | nindent 4 }}
data:
{{- if not .Values.langfuse.nextauth.url.valueFrom }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary with {{- if (not .Values.langfuse.nextauth.url.valueFrom) }} (l.1)

{{- include "langfuse.labels" . | nindent 4 }}
type: Opaque
data:
{{- if not .Values.langfuse.salt.valueFrom }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless with {{- if (not .Values.langfuse.salt.valueFrom) }} at the top?

@@ -1,6 +1,6 @@
apiVersion: v2
name: langfuse
version: 0.2.1
version: 0.2.2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the changes, especially the non retro-compatibility introduced in values.yaml and semver specs, it needs a major bump

@mautini
Copy link
Contributor

mautini commented Aug 26, 2024

Added comments. It seems to be a great addition but break retro-compatibility

@japan4415
Copy link
Author

so sorry i missed notification from github
ill continue writing fix from today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Secure handling of secrets
4 participants