-
Notifications
You must be signed in to change notification settings - Fork 174
feat(worker): add AWS IAM flags to worker command for RDS IAM auth #1557
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
base: main
Are you sure you want to change the base?
Changes from all commits
1803a71
3fcc7d8
69317ed
7bad67d
a108e42
a1132aa
15eb724
efb626c
b056c62
397188d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import ( | |
| "google.golang.org/grpc" | ||
| "google.golang.org/grpc/credentials/insecure" | ||
|
|
||
| "github.com/formancehq/go-libs/v5/pkg/cloud/aws/iam" | ||
| "github.com/formancehq/go-libs/v5/pkg/fx/storagefx" | ||
| "github.com/formancehq/go-libs/v5/pkg/observe/metrics" | ||
| "github.com/formancehq/go-libs/v5/pkg/observe/traces" | ||
|
|
@@ -93,6 +94,23 @@ func NewWorkerCommand() *cobra.Command { | |
| Use: "worker", | ||
| SilenceUsage: true, | ||
| RunE: func(cmd *cobra.Command, _ []string) error { | ||
| // Fail fast if the operator combines --postgres-aws-enable-iam with | ||
| // --aws-role-arn. iam.LoadOptionFromFlags does not yet consume | ||
| // AWSRoleArnFlag, so role assumption would silently not occur and | ||
| // the worker would fall back to the default credential chain instead | ||
| // of the intended role. Return an explicit error rather than | ||
| // misleading the operator. | ||
| if iamEnabled, _ := cmd.Flags().GetBool(connect.PostgresAWSEnableIAMFlag); iamEnabled { | ||
| if roleArn, _ := cmd.Flags().GetString(iam.AWSRoleArnFlag); roleArn != "" { | ||
| return fmt.Errorf( | ||
| "--%s is not yet supported: iam.LoadOptionFromFlags does not "+ | ||
| "consume the role ARN and role assumption will silently not occur; "+ | ||
| "remove --%s or add role-assumption support to go-libs first", | ||
| iam.AWSRoleArnFlag, iam.AWSRoleArnFlag, | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| connectionOptions, err := connect.ConnectionOptionsFromFlags(cmd.Flags(), cmd.Context()) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -132,6 +150,10 @@ func NewWorkerCommand() *cobra.Command { | |
| connect.AddFlags(cmd.Flags()) | ||
| metrics.AddFlags(cmd.Flags()) | ||
| traces.AddFlags(cmd.Flags()) | ||
| iam.AddFlags(cmd.Flags()) | ||
|
NumaryBot marked this conversation as resolved.
NumaryBot marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 [major] --aws-role-arn flag is registered but silently ignored in the Postgres IAM connection path The worker command registers --aws-role-arn (alongside --postgres-aws-enable-iam), but connect.ConnectionOptionsFromFlags loads AWS config via iam.LoadOptionFromFlags, which reads region/static keys/session/profile but not role ARN. A worker started with --postgres-aws-enable-iam --aws-role-arn will not assume the requested role; it silently falls back to the default/static credential chain. MarkHidden (if used) only hides the flag from help output — Cobra still parses and accepts it — so the no-op behaviour is invisible to operators. Suggestion: Either (a) make the connection path consume AWSRoleArnFlag by wrapping the loaded credentials with stscreds.NewAssumeRoleProvider + aws.NewCredentialsCache when the flag is non-empty, (b) do not register --aws-role-arn on the worker command at all until the underlying library supports it and document the limitation clearly, or (c) add an explicit pre-run validation that returns a clear error when --aws-role-arn is set alongside --postgres-aws-enable-iam so operators are not silently misled. |
||
| // Hide --aws-role-arn until go-libs consumes it; exposing a no-op flag | ||
| // would silently mislead operators configuring RDS IAM role assumption. | ||
| _ = cmd.Flags().MarkHidden(iam.AWSRoleArnFlag) | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package cmd | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/formancehq/go-libs/v5/pkg/cloud/aws/iam" | ||
| "github.com/formancehq/go-libs/v5/pkg/storage/bun/connect" | ||
| ) | ||
|
|
||
| // TestWorkerCommandHasAWSIAMFlags verifies that the worker command registers | ||
| // all required AWS IAM flags, so that users running the standalone worker binary | ||
| // (e.g. on AWS ECS Fargate) can configure RDS IAM auth for the primary store | ||
| // via CLI flags, not just via environment variables. | ||
| func TestWorkerCommandHasAWSIAMFlags(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := NewWorkerCommand() | ||
| flags := cmd.Flags() | ||
|
|
||
| // These flags come from connect.AddFlags and enable IAM on the primary store | ||
| t.Run("postgres-aws-enable-iam flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(connect.PostgresAWSEnableIAMFlag) | ||
| require.NotNil(t, f, "--postgres-aws-enable-iam flag must be registered on the worker command") | ||
| assert.Equal(t, "false", f.DefValue) | ||
| }) | ||
|
|
||
| // These flags come from iam.AddFlags (added to fix issue #1556). | ||
| t.Run("aws-region flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSRegionFlag) | ||
| require.NotNil(t, f, "--aws-region flag must be registered on the worker command") | ||
| }) | ||
|
|
||
| t.Run("aws-access-key-id flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSAccessKeyIDFlag) | ||
| require.NotNil(t, f, "--aws-access-key-id flag must be registered on the worker command") | ||
| }) | ||
|
|
||
| t.Run("aws-secret-access-key flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSSecretAccessKeyFlag) | ||
| require.NotNil(t, f, "--aws-secret-access-key flag must be registered on the worker command") | ||
| }) | ||
|
|
||
| t.Run("aws-session-token flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSSessionTokenFlag) | ||
| require.NotNil(t, f, "--aws-session-token flag must be registered on the worker command") | ||
| }) | ||
|
|
||
| t.Run("aws-profile flag is registered", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSProfileFlag) | ||
| require.NotNil(t, f, "--aws-profile flag must be registered on the worker command") | ||
| }) | ||
|
|
||
| // --aws-role-arn is registered by iam.AddFlags but is hidden on the worker | ||
| // command until iam.LoadOptionFromFlags in go-libs consumes it. Exposing a | ||
| // no-op flag would silently mislead operators configuring RDS IAM role | ||
| // assumption. Full support is tracked for a future go-libs update. | ||
| t.Run("aws-role-arn flag is registered but hidden", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(iam.AWSRoleArnFlag) | ||
| require.NotNil(t, f, "--aws-role-arn flag must be registered on the worker command") | ||
| assert.True(t, f.Hidden, "--aws-role-arn must be hidden until go-libs consumes it") | ||
| }) | ||
| } | ||
|
|
||
| // TestWorkerCommandRejectsRoleArnWithIAM verifies that the worker command | ||
| // returns a clear error when --aws-role-arn is combined with | ||
| // --postgres-aws-enable-iam, rather than silently ignoring the ARN. | ||
| func TestWorkerCommandRejectsRoleArnWithIAM(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := NewWorkerCommand() | ||
| require.NoError(t, cmd.Flags().Set(connect.PostgresAWSEnableIAMFlag, "true")) | ||
| require.NoError(t, cmd.Flags().Set(iam.AWSRoleArnFlag, "arn:aws:iam::123456789012:role/MyRole")) | ||
|
|
||
| err := cmd.RunE(cmd, nil) | ||
| require.Error(t, err) | ||
| assert.Contains(t, err.Error(), "--"+iam.AWSRoleArnFlag) | ||
| assert.Contains(t, err.Error(), "not yet supported") | ||
| } | ||
|
|
||
| // the same AWS IAM flags for parity (regression guard). | ||
| func TestServeCommandHasAWSIAMFlags(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| cmd := NewServeCommand() | ||
| flags := cmd.Flags() | ||
|
|
||
| // Verify the IAM-enable flag defaults to false on serve as well | ||
| f := flags.Lookup(connect.PostgresAWSEnableIAMFlag) | ||
| require.NotNil(t, f, "--postgres-aws-enable-iam flag must be registered on the serve command") | ||
| assert.Equal(t, "false", f.DefValue) | ||
|
|
||
| iamFlags := []string{ | ||
| connect.PostgresAWSEnableIAMFlag, | ||
| iam.AWSRegionFlag, | ||
| iam.AWSAccessKeyIDFlag, | ||
| iam.AWSSecretAccessKeyFlag, | ||
| iam.AWSSessionTokenFlag, | ||
| iam.AWSProfileFlag, | ||
| iam.AWSRoleArnFlag, | ||
| } | ||
|
|
||
| for _, flagName := range iamFlags { | ||
| t.Run(flagName+" is registered on serve", func(t *testing.T) { | ||
| t.Parallel() | ||
| f := flags.Lookup(flagName) | ||
| require.NotNil(t, f, "--%s flag must be registered on the serve command", flagName) | ||
| }) | ||
| } | ||
|
|
||
| } |
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] Registering --aws-role-arn breaks IRSA/web-identity environments via env-var binding
service.Execute populates every registered flag from its uppercase environment variable equivalent, so adding iam.AddFlags (which registers --aws-role-arn) causes AWS_ROLE_ARN to be bound to that flag. In IRSA or web-identity environments where AWS_ROLE_ARN is legitimately set for the AWS SDK's standard credential chain alongside POSTGRES_AWS_ENABLE_IAM=true, any validation check that rejects or errors on --aws-role-arn will abort startup even though the operator never intended to pass this flag explicitly.
Suggestion: Avoid registering --aws-role-arn on the worker command unless the connection path actually consumes it. If the flag must be registered, ensure that the presence of AWS_ROLE_ARN via environment variable does not cause an error or unexpected behaviour for operators using standard SDK credential flows.