Skip to content

Conversation

@linglingye001
Copy link
Member

No description provided.

keyvault.go Outdated
// keyVaultReferenceResolver resolves Key Vault references to their actual secret values
type keyVaultReferenceResolver struct {
clients map[string]secretClient
resolver SecretResolver
Copy link

Choose a reason for hiding this comment

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

Do we need to export this interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we export SecretResolver interface which need to be implemented by user.

keyvault.go Outdated
return nil, fmt.Errorf("failed to create Key Vault client: %w", err)
}

r.clients[vaultURL] = client
Copy link

Choose a reason for hiding this comment

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

resolveSecrets would be called concurrently in different routines, hence this resovler.getSecretClient. Becareful of clients, it needs to be multi-routine-safe, too.

@linglingye001 linglingye001 requested a review from jhzhu89 March 17, 2025 09:43
// SecretResolver is an interface to resolve secret from key vault reference
type SecretResolver interface {
// keyVaultReference: "https://{keyVaultName}.vault.azure.net/secrets/{secretName}/{secretVersion}"
ResolveSecret(ctx context.Context, keyVaultReference url.URL) (string, error)
Copy link
Member

Choose a reason for hiding this comment

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

How about rename this parameter to secretUrl?

Copy link
Member Author

Choose a reason for hiding this comment

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

Prefer keyVaultReference since it's widely used in our public docs

Copy link

@jhzhu89 jhzhu89 left a comment

Choose a reason for hiding this comment

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

LGTM.

Maybe you could consider adding some test cases for covering concurrent r/w scenarios, you can run these tests with race enabled. It could help you capture race conditions are not well handled.

@linglingye001 linglingye001 merged commit 23149a8 into release/v0 Mar 19, 2025
1 check passed
@linglingye001 linglingye001 deleted the linglingye/keyvault branch March 19, 2025 04:57
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.

4 participants