-
Notifications
You must be signed in to change notification settings - Fork 9
[database] Add TLS support for database connections #159
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
Conversation
…ease-image-integration-test. Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
service/vc/dbtest/container.go
Outdated
| // is ready to accept connections. | ||
| // It repeatedly executes `pg_isready` until the command | ||
| // returns a successful exit code (0) or the timeout is reached. | ||
| func (dc *DatabaseContainer) EnsurePostgresNodeReadiness(t *testing.T, port string) { |
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.
YugabyteDB should support pg_isready
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.
It is, but not naturally. We need to export the path to its postgres tools. It's easier to monitor its readiness by its logs.
service/vc/dbtest/container.go
Outdated
| func (dc *DatabaseContainer) ReadPasswordFromContainer(t *testing.T, filePath string) string { | ||
| t.Helper() | ||
| output, exitCode := dc.ExecuteCommand(t, []string{"cat", filePath}) | ||
| require.Zero(t, exitCode) |
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.
major: If the file doesn't exist, it fails the test.
But if it exists, and doesn't contain the password, it won't fail the tests.
This inconsistency is not justified.
Please fix or add a comment to justify this.
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 use this method only when a secured YugabyteDB node is started. If the file doesn’t exist, the test should fail. If the file exists but doesn’t contain a password, we fall back to the default password.
I'll add the above to the function's comment.
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.
But if we try the default password for the secured test, it will not work, right? So, isn't it best to fail the test?
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.
Actually, I’m not sure how the database behaves in that case, but for consistency, we’ll fail the test in this scenario as well.
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Signed-off-by: Dean Amar <[email protected]>
Type of change
Description
Related issues