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

disable service account validation on DisableCredentialRotation #16

Merged
merged 1 commit into from
May 15, 2024

Conversation

kheina
Copy link
Contributor

@kheina kheina commented May 13, 2024

this should allow the plugin to work with other S3-compatible providers by using DisableCredentialRotation=true

…credential rotation. should allow other S3-compatible providers
@hugoghx hugoghx requested review from louisruch and hugoghx May 13, 2024 16:26
Copy link
Contributor

@louisruch louisruch left a comment

Choose a reason for hiding this comment

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

Mostly looks good, I have one question on where we are doing the service account check

if !sec.LastRotatedTime.IsZero() {
// since these are existing secrets, we only want to validate the
// service account if the credentials have been rotated previously
err = ensureServiceAccount(ctx, sa, sec)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this true? If it is the first time we are rotating credentials and its not a service account wont the rotation fail? My understanding was we cannot rotate if its not a service account, does this change then make it so we fail to rotate instead of validating the service account and fail on that? If that is the case is this check then just redundant?

Copy link
Contributor Author

@kheina kheina May 14, 2024

Choose a reason for hiding this comment

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

the first thing the rotation func does is run ensure service account, so that's done already:

func rotateCredentials(ctx context.Context, bucketName string, sa *StorageAttributes, inSec *StorageSecrets) (*StorageSecrets, deleteInputCredsFn, error) {
err := ensureServiceAccount(ctx, sa, inSec)
if err != nil {
return nil, nil, fmt.Errorf("failed to ensure minio service account credentials: %w", err)
}

@kheina kheina merged commit 9299172 into main May 15, 2024
2 checks passed
@kheina kheina deleted the dmiu_move-svcacct-check-rotate-only branch May 15, 2024 15:45
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