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.

Sign up for GitHub

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

Jump to bottom

feat:SSL for postgres #2473

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

feat:SSL for postgres #2473

wants to merge 31 commits into from
+312 −0

Conversation

bearrito
Copy link
Contributor

@bearrito bearrito commented Apr 8, 2024 •
edited

What does this PR do?

Enables SSL for postgres

The main thing to recognize when reviewing this is that the secret material must be owned by the postgres user.
The docker file copy api doesn't allowfor setting a user when copying. The only way I could think was to take wrap the entrypoint script with one that does what we want.

Why is it important?

User ran into issue trying to use SSL. It's not obviously supported (able) due to file permissions.

Related issues

Link related issues below. Insert the issue link or reference after the word "Closes" if merging this should automatically close it.

How to test this PR

Unit tests will work when complete.

@bearrito bearrito requested a review from a team as a code owner April 8, 2024 18:45
@netlify Netlify
Copy link

netlify bot commented Apr 8, 2024 •
edited

✅ Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit 53d9a1a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/662bd087d8a39a0008d6b1c4
😎 Deploy Preview https://deploy-preview-2473--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@bearrito bearrito changed the title [DRAFT] feat:SSL for postgres feat:SSL for postgres Apr 8, 2024
@bearrito bearrito mentioned this pull request Apr 8, 2024
Open
mdelapenya
mdelapenya requested changes Apr 9, 2024
View reviewed changes
Copy link
Collaborator

@mdelapenya mdelapenya left a comment •
edited

Choose a reason for hiding this comment

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

We are in the right track with this one, although I left some comments that need to be addressed.

Besides that, we need to include the new option in the docs. Please check the docs/modules/postgres.md file for that. Remember adding this right after the option title:

- Not available until the next release of testcontainers-go <a href="https://github.com/testcontainers/testcontainers-go"><span class="tc-version">:material-tag: main</span></a>

Thanks!

modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres.go Outdated Show resolved Hide resolved
modules/postgres/options.go Outdated Show resolved Hide resolved
@mdelapenya
Copy link
Collaborator

@bearrito could you please take a look at #2478? I'm adding helper code to deal with TLS certificates in order to simplify the story for the client code. Wdyt?

@bearrito
Copy link
Contributor Author

bearrito commented Apr 10, 2024 •
edited

@mdelapenya That makes a lot of sense. Much cleaner.

Do you want me to wait until that's done then base this on top of that?

@mdelapenya
Copy link
Collaborator

@bearrito I went ahead and extracted the TLS cert generation to a separate go package, which makes more sense: https://github.com/mdelapenya/tlscert

Please take a look and use it as you need here!

@bearrito
Copy link
Contributor Author

@mdelapenya Updated to use your package.

@bearrito bearrito mentioned this pull request Apr 11, 2024
Merged
mdelapenya
mdelapenya reviewed Apr 12, 2024
View reviewed changes
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

Just one final concern regarding the entrypoint: do you think we need to pass it? Or is it something we can internally handle in the module, being transparent to the end users?

modules/postgres/go.mod Outdated Show resolved Hide resolved
modules/postgres/testdata/docker-entrypoint-ssl.bash Outdated Show resolved Hide resolved
@bearrito
Copy link
Contributor Author

@mdelapenya Update so that the entrypoint is coupled with the ssl settings. What about doing something similar for the conf file?

@bearrito
Copy link
Contributor Author

@mdelapenya added docs.

mdelapenya
mdelapenya reviewed Apr 24, 2024
View reviewed changes
docs/modules/postgres.md Outdated Show resolved Hide resolved
docs/modules/postgres.md Show resolved Hide resolved
docs/modules/postgres.md Outdated Show resolved Hide resolved
docs/modules/postgres.md Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres.go Show resolved Hide resolved
bearrito and others added 6 commits April 24, 2024 12:09
@bearrito @mdelapenya
Update docs/modules/postgres.md
b9cd59b 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update docs/modules/postgres.md
ef017c9 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update docs/modules/postgres.md
fd3d3e5 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
cd1b63f 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
c7eebeb 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Embed resources + Use custom conf automatically
a36eafa
mdelapenya
mdelapenya reviewed Apr 26, 2024
View reviewed changes
Copy link
Collaborator

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

I left a few more comments, sorry if that's taking too long 😞 My only concern is about the default SSL conf file, please see my comments about it.

docs/modules/postgres.md Show resolved Hide resolved
docs/modules/postgres.md Show resolved Hide resolved
docs/modules/postgres.md Outdated Show resolved Hide resolved
modules/postgres/postgres.go Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/postgres_test.go Outdated Show resolved Hide resolved
modules/postgres/resources/postgres-ssl.conf Outdated Show resolved Hide resolved
bearrito and others added 13 commits April 26, 2024 11:04
@bearrito @mdelapenya
Update docs/modules/postgres.md
7ea323a 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update docs/modules/postgres.md
0557c9d 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update docs/modules/postgres.md
5d0fa1e 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
7805e1f 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
c80fa83 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
e73f9a5 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
@bearrito @mdelapenya
Update modules/postgres/postgres_test.go
1be6a31 
Co-authored-by: Manuel de la Peña <social.mdelapenya@gmail.com>
Revert to use passed in conf
4e6c6d7
Update doc for required conf
e28361a
@bearrito
Merge branch 'main' into feature/postgres-ssl
8502649
Error checking in the customizer
5d8598f
Few formatting fix
dffe996
Use non-nil error when err is nil
53d9a1a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdelapenya mdelapenya mdelapenya requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: File permissions with custom PostgreSQL image
2 participants
@bearrito @mdelapenya