Skip to content

Conversation

@Chickensoupwithrice
Copy link
Contributor

@Chickensoupwithrice Chickensoupwithrice commented Jan 16, 2025

REL-638
raised issues that this flag did not get mentioned in the docs where a
customer wanted to use TLS. It also outlined the recommended fixes that
this commit implements.

This PR has an accompanying docs PR

Checklist

Test plan

Manually test this out? Would require spinning up an RDS instance or something, but that's fine.
I'm in the process of doing so now.

@Chickensoupwithrice
Copy link
Contributor Author

Chickensoupwithrice commented Jan 17, 2025

I've finally verified that this fix works. Below are screenshots showing that sslmode is passed in as an environment variable, and it's able to connect successfully to an external database.
image
image
image

Here is the patch I used with the password removed:

Patch file for testing

diff --git a/charts/sourcegraph/values.yaml b/charts/sourcegraph/values.yaml
index c74b21a..5b8e717 100644
--- a/charts/sourcegraph/values.yaml
+++ b/charts/sourcegraph/values.yaml
@@ -148,24 +148,24 @@ cadvisor:

codeInsightsDB:

-- Enable codeinsights-db PostgreSQL server

  • enabled: true
  • enabled: false
    auth:

    -- Name of existing secret to use for Code Insights credentials

    The secret must contain the keys user, password, database, host and port.

    auth.user, auth.password, etc. are ignored if this is enabled

    existingSecret: ""

    -- Sets codeinsights-db database name

  • database: "postgres"
  • database: "codeinsights-db"

    -- Sets codeinsights-db host

  • host: "codeinsights-db"
  • host: "database-1-sslmode-test.cwpsk693immt.us-west-2.rds.amazonaws.com"

    -- Sets codeinsights-db username

    user: "postgres"

    -- Sets codeinsights-db password

  • password: "password"
  • password: ""

    -- Sets codeinsights-db port

    port: "5432"

    -- Sets codeinsights-db SSL mode

  • sslmode: "disable" # set to "require" to enable SSL
  • sslmode: "require" # set to "require" to enable SSL

-- Environment variables for the codeinsights-db container

env: {}

-- Name of existing ConfigMap for codeinsights-db. It must contain a postgresql.conf key.

@@ -223,24 +223,24 @@ codeInsightsDB:

codeIntelDB:

-- Enable codeintel-db PostgreSQL server

  • enabled: true
  • enabled: false
    auth:

    -- Name of existing secret to use for CodeIntel credentials

    The secret must contain the keys user, password, database, host and port.

    auth.user, auth.password, etc. are ignored if this is enabled

    existingSecret: ""

    -- Sets codeintel-db database name

  • database: "sg"
  • database: "codeintel-db"

    -- Sets codeintel-db host

  • host: "codeintel-db"
  • host: "database-1-sslmode-test.cwpsk693immt.us-west-2.rds.amazonaws.com"

    -- Sets codeintel-db username

  • user: "sg"
  • user: "postgres"

    -- Sets codeintel-db password

  • password: "password"
  • password: ""

    -- Sets codeintel-db port

    port: "5432"

    -- Sets codeintel-db SSL mode

  • sslmode: "disable" # set to "require" to enable SSL
  • sslmode: "require" # set to "require" to enable SSL

-- Name of existing ConfigMap for codeintel-db. It must contain a postgresql.conf key

existingConfig: ""

-- Additional PostgreSQL configuration. This will override or extend our default configuration.

@@ -709,24 +709,24 @@ nodeExporter:

pgsql:

-- Enable pgsql PostgreSQL server

  • enabled: true
  • enabled: false
    auth:

    -- Name of existing secret to use for Postgres credentials

    The secret must contain the keys user, password, database, host and port.

    auth.user, auth.password, etc. are ignored if this is enabled

    existingSecret: ""

    -- Sets postgres database name

  • database: "sg"
  • database: "postgres"

    -- Sets postgres host

  • host: "pgsql"
  • host: "database-1-sslmode-test.cwpsk693immt.us-west-2.rds.amazonaws.com"

    -- Sets postgres username

  • user: "sg"
  • user: "postgres"

    -- Sets postgres password

  • password: "password"
  • password: ""

    -- Sets postgres port

    port: "5432"

    -- Sets postgres SSL mode

  • sslmode: "disable" # set to "require" to enable SSL
  • sslmode: "require" # set to "require" to enable SSL

-- Name of existing ConfigMap for pgsql. It must contain a postgresql.conf key

existingConfig: "" # Name of an existing configmap

-- Additional PostgreSQL configuration. This will override or extend our default configuration.

[REL-638](https://linear.app/sourcegraph/issue/REL-638/configure-aws-rds-databases-for-tls-connections-in-helm-chart)
raised issues that this flag did not get mentioned in the docs where a
customer wanted to use TLS. It also outlined the recommended fixes that
this commit implements.
- update pgsql secret checksum
- codeinsights db checksum
- codeintel db checksum
- pgsqlAuth test checksum
- the other pgsql checsum...
@Chickensoupwithrice Chickensoupwithrice enabled auto-merge (squash) January 17, 2025 17:27
@Chickensoupwithrice Chickensoupwithrice merged commit bbc52d1 into main Jan 17, 2025
5 checks passed
@Chickensoupwithrice Chickensoupwithrice deleted the al/REL-638/pg-ssl-mode branch January 17, 2025 17:27
michaellzc added a commit that referenced this pull request Jan 25, 2025
regression from
#617

it's now rendering `_SSLMODE` twice in the grafana sts

### Checklist

- [ ] Follow the [manual testing
process](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/TEST.md)
- [ ] Update
[changelog](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/CHANGELOG.md)
- [ ] Update [Kubernetes update
doc](https://docs.sourcegraph.com/admin/updates/kubernetes)

### Test plan

CI
michaellzc added a commit that referenced this pull request Jan 25, 2025
…afana sts (#628)

regression from
#617

it's now rendering `_SSLMODE` twice in the grafana sts

### Checklist

- [ ] Follow the [manual testing
process](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/TEST.md)
- [ ] Update
[changelog](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/CHANGELOG.md)
- [ ] Update [Kubernetes update
doc](https://docs.sourcegraph.com/admin/updates/kubernetes)

### Test plan

CI <br> Backport bb2d4f1 from #627

Co-authored-by: Michael Lin <[email protected]>
enriquegh pushed a commit that referenced this pull request Jul 10, 2025
[REL-638](https://linear.app/sourcegraph/issue/REL-638/configure-aws-rds-databases-for-tls-connections-in-helm-chart)
raised issues that this flag did not get mentioned in the docs where a
customer wanted to use TLS. It also outlined the recommended fixes that
this commit implements.

This PR has an accompanying [docs
PR](sourcegraph/docs#900)

### Checklist

- [x] Follow the [manual testing
process](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/TEST.md)
- [ ] Update
[changelog](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/CHANGELOG.md)
- [ ] Update [Kubernetes update
doc](https://docs.sourcegraph.com/admin/updates/kubernetes)

### Test plan
Manually test this out? Would require spinning up an RDS instance or
something, but that's fine.
I'm in the process of doing so now.
<!--
As part of SOC2/GN-104 and SOC2/GN-105 requirements, all pull requests
are REQUIRED to
provide a "test plan". A test plan is a loose explanation of what you
have done or
implemented to test this, as outlined in our Testing principles and
guidelines:

https://docs.sourcegraph.com/dev/background-information/testing_principles
  Write your test plan here after the "Test plan" header.
-->
enriquegh pushed a commit that referenced this pull request Jul 10, 2025
regression from
#617

it's now rendering `_SSLMODE` twice in the grafana sts

### Checklist

- [ ] Follow the [manual testing
process](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/TEST.md)
- [ ] Update
[changelog](https://github.com/sourcegraph/deploy-sourcegraph-helm/blob/main/charts/sourcegraph/CHANGELOG.md)
- [ ] Update [Kubernetes update
doc](https://docs.sourcegraph.com/admin/updates/kubernetes)

### Test plan

CI
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.

3 participants