-
Notifications
You must be signed in to change notification settings - Fork 100
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
Creating an optional interface to get secret id references for driver with secrets. #7306
Creating an optional interface to get secret id references for driver with secrets. #7306
Conversation
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Driver | ||
|
||
// FindSecretIDs gets the secret ID references from environment definition. | ||
FindSecretIDs(ctx context.Context, config recipes.Configuration, definition recipes.EnvironmentDefinition) (string, error) |
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.
Should this return ([]string, error)
?
What would happen in the future if a driver needed to read multiple secrets?
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.
currently we are keeping it to return module secret. But in the future when we have provider secrets etc, return type will be map because we may need to specify secret kind then ex: module secret or provider secret. It will be discussed in detail during the design for provider secrets.
pkg/recipes/engine/engine.go
Outdated
if secretStore != "" { | ||
secrets, err = e.options.SecretsLoader.LoadSecrets(ctx, secretStore) | ||
if err != nil { | ||
return nil, nil, fmt.Errorf("failed to fetch secrets from the secret store resource id %s for Terraform recipe %s deployment: %w", secretStore, definition.TemplatePath, err) |
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.
Does this need its own error code?
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 agree, we could add a new error code for load secrets failures.
thoughts on naming it as LOAD_SECRETS_FAILED
?
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.
ARM-like error codes PascalCase so it would be LoadSecretsFailed
@@ -249,6 +249,11 @@ func (d *terraformDriver) GetRecipeMetadata(ctx context.Context, opts BaseOption | |||
return recipeData, nil | |||
} | |||
|
|||
func (d *terraformDriver) FindSecretIDs(ctx context.Context, envConfig recipes.Configuration, definition recipes.EnvironmentDefinition) (string, error) { |
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.
nit: Please add comment.
@@ -42,6 +42,13 @@ type Driver interface { | |||
GetRecipeMetadata(ctx context.Context, opts BaseOptions) (map[string]any, error) | |||
} | |||
|
|||
type DriverWithSecrets interface { |
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.
nit: Please add comment
pkg/recipes/driver/types.go
Outdated
type DriverWithSecrets interface { | ||
Driver | ||
|
||
// FindSecretIDs gets the secret ID references from environment definition. |
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.
Could you add more details in this comment? What is the secret ID for example? Where does it read it from in the environment definition - recipe config. Why is it needed - to access private modules (for now, with providers change it would extend to secrets passed into the providers config as well)
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Vishwanath Hiremath <[email protected]>
Signed-off-by: Karishma Chawla <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
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.
@vishwahiremat can this be merged?
Signed-off-by: Vishwanath Hiremath <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Vishwanath Hiremath <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
Refactoring module secret workflow.
Type of change
Fixes: #issue_number