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

S3 Backend: load keys from file with automatic reload #46

Merged
merged 20 commits into from
Sep 19, 2023

Conversation

ahouene
Copy link
Contributor

@ahouene ahouene commented Sep 4, 2023

Introduce a github.com/minio/minio-go/v7/pkg/credentials.Provider implementation, accepting credentials split accross different files as with Kubernetes Secrets or Docker Secrets.

This way, the backend can get its S3 credentials with a configuration similar to:

{
  "access_key_file": "/etc/secret-volume/access-key",
  "secret_key_file": "/etc/secret-volume/secret-key"
}

@ahouene ahouene self-assigned this Sep 4, 2023
backends/s3/credentials.go Outdated Show resolved Hide resolved
type K8sSecretProvider struct {
// Path to the fiel containing the access key,
// e.g. /etc/s3-secrets/access-key.
AccessKeyFilename string `json:"access_key_file"`
Copy link
Member

Choose a reason for hiding this comment

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

JSON name does not match the variable name. In this case it would commonly be called access_key_filename, or the variable named AccessKeyFile. There is no good reason to deviate from this pattern here.

backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/credentials_test.go Outdated Show resolved Hide resolved
backends/s3/credentials_test.go Outdated Show resolved Hide resolved
backends/s3/s3testing/port.go Show resolved Hide resolved
backends/s3/s3testing/minio.go Outdated Show resolved Hide resolved
backends/s3/s3testing/minio.go Outdated Show resolved Hide resolved
@ahouene ahouene changed the title S3 Backend: New Kubernetes Secrets credentials provider S3 Backend: New Secrets credentials provider Sep 6, 2023
@ahouene ahouene requested a review from wojas September 7, 2023 07:19
backends/s3/credentials.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3testing/minio.go Outdated Show resolved Hide resolved
backends/s3/s3testing/minio.go Show resolved Hide resolved

stop := func() error {
_ = cmd.Process.Signal(os.Interrupt)
return cmd.Wait()
Copy link
Member

Choose a reason for hiding this comment

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

This could hang forever if the context does not have a timeout.

@wojas
Copy link
Member

wojas commented Sep 7, 2023

The PR description mentions *_file, while the implementation uses *_filename. Having *_file is better, see other comment.

@ahouene ahouene requested a review from wojas September 12, 2023 07:59
Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

LGTM, aside from two minor things.

backends/s3/credentials.go Outdated Show resolved Hide resolved
backends/s3/s3testing/minio.go Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
backends/s3/s3.go Outdated Show resolved Hide resolved
Copy link
Member

@wojas wojas left a comment

Choose a reason for hiding this comment

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

LGTM!

@nvaatstra
Copy link
Contributor

LGTM

Tested this PR in 2 applications in a Kubernetes deployment and both worked as expected: Cycling the access/secret keys in the Secret leads to Simpleblob picking up the updated credentials once the Secret is synced to the filesystem of the container.

@wojas wojas changed the title S3 Backend: New Secrets credentials provider S3 Backend: load keys from file with automatic reload Sep 15, 2023
@wojas wojas merged commit 033e00a into PowerDNS:main Sep 19, 2023
2 checks passed
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