-
-
Notifications
You must be signed in to change notification settings - Fork 487
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
feat:SSL for postgres #2473
base: main
Are you sure you want to change the base?
feat:SSL for postgres #2473
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
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!
@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? |
@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! |
@mdelapenya Updated to use your package. |
There was a problem hiding this 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?
@mdelapenya Update so that the entrypoint is coupled with the ssl settings. What about doing something similar for the conf file? |
@mdelapenya added docs. |
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
There was a problem hiding this 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.
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
Co-authored-by: Manuel de la Peña <[email protected]>
@mdelapenya Anything else on this? I'm going on holiday and am closing out my open pr's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response :(
I think we are close to make it into the project, but let's clarify the custom entry point, as I'm not sure if I understand its use case properly
Thanks!
os.RemoveAll(tmpDir) | ||
}) | ||
|
||
caCert := tlscert.SelfSignedFromRequest(tlscert.Request{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered using tlscert.SelfSignedCA here instead? See https://github.com/mdelapenya/tlscert/blob/a038576e80ef259d2a60317ab51e37cdda8da3c8/tlscert.go#L135
chown "$pUID":"$pGID" /tmp/data/ca_cert.pem | ||
chown "$pUID":"$pGID" /tmp/data/server.cert | ||
chown "$pUID":"$pGID" /tmp/data/server.key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be enough?
chown "$pUID":"$pGID" /tmp/data/ca_cert.pem | |
chown "$pUID":"$pGID" /tmp/data/server.cert | |
chown "$pUID":"$pGID" /tmp/data/server.key | |
chown -R "${pUID}:${pGID}" /tmp/data |
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.