Skip to content

Allow hosts to inject AWS STS clients - #47

Open
c1-squire-dev[bot] wants to merge 2 commits into
mainfrom
paul.querna/sts-regional-fallback-hooks
Open

Allow hosts to inject AWS STS clients#47
c1-squire-dev[bot] wants to merge 2 commits into
mainfrom
paul.querna/sts-regional-fallback-hooks

Conversation

@c1-squire-dev

@c1-squire-dev c1-squire-dev Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Add optional constructor hooks for the AWS SDK config loader and STS client factory. Baton EKS defaults remain unchanged. A host can now apply its shared STS policy to the connector's base, binding-account, customer-account, and on-prem role assumptions.

Test

  • go test ./pkg/connector

Keep Baton EKS defaults unchanged while letting hosts inject their
credential-resolution and STS implementations for every role hop.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment thread pkg/connector/connector.go Outdated
config *config.Eks
awsConfig awsSdk.Config
baseClient *http.Client
loadAWSConfig AWSConfigLoader

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: Connector.loadAWSConfig is assigned at line 146 but never read anywhere else — the loader is only used locally inside New via opts.loadAWSConfig. Unlike newSTSClient (used in getCallingConfig), this field is dead state. Consider dropping it from the struct, or wiring it into the paths that build configs later so injected loaders actually apply post-construction.

Comment on lines +108 to +121
opts := options{
loadAWSConfig: awsConfig.LoadDefaultConfig,
newSTSClient: func(cfg awsSdk.Config) stscreds.AssumeRoleAPIClient {
return sts.NewFromConfig(cfg)
},
}
for _, fn := range optFns {
if fn != nil {
fn(&opts)
}
}
if opts.loadAWSConfig == nil || opts.newSTSClient == nil {
return nil, fmt.Errorf("eks connector: AWS config loader and STS client factory are required")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: The PR adds new exported API (Option, AWSConfigLoader, STSClientFactory, WithAWSConfigLoader, WithSTSClientFactory) whose entire purpose is host-side injection, but no test exercises it. pkg/connector already has table-driven tests; a small test that passes a fake loader/STS factory into New would lock in the defaults-unchanged guarantee and the nil-option rejection at line 119.

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Connector PR Review: Allow hosts to inject AWS STS clients

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 8a74bb7a5a21.
Review mode: full
View review run: https://github.com/ConductorOne/baton-eks/actions/runs/33847034024

Review Summary

Scanned the full PR diff for security and correctness. The change is confined to pkg/connector/connector.go: it adds a variadic Option seam to New() for the AWS config loader and STS client factory, and threads the factory into getCallingConfig and getOnPremAWSConfig. Defaults preserve existing behavior (awsConfig.LoadDefaultConfig and sts.NewFromConfig), and the variadic parameter keeps the existing New(ctx, cfg) call site in cmd/baton-eks/main.go:52 source-compatible, so this is not a breaking change. No dependency manifests changed and no new scopes or config fields were introduced. No security or correctness issues found - two non-blocking suggestions below.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • pkg/connector/connector.go:66 - Connector.loadAWSConfig is stored but never read after New() returns; dead struct state (confidence: high).
  • pkg/connector/connector.go:108-121 - the new exported injection API has no test coverage despite existing specifically for host-side injection (confidence: high).
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/connector.go`:
- Around line 66 (and its assignment at line 146): the `loadAWSConfig AWSConfigLoader`
  field on the `Connector` struct is written during construction but never read
  anywhere afterwards. Only `opts.loadAWSConfig` is used, locally inside `New`. Unlike
  `newSTSClient`, which is genuinely needed later by `getCallingConfig`, this field is
  dead state. Either remove `loadAWSConfig` from the `Connector` struct and from the
  struct literal at line 146, or, if the intent is for injected loaders to also apply
  to configs built after construction, actually call `o.loadAWSConfig` in those paths.
- Around lines 108-121: add a test in `pkg/connector` covering the new exported
  injection API (`Option`, `AWSConfigLoader`, `STSClientFactory`,
  `WithAWSConfigLoader`, `WithSTSClientFactory`). Cover at minimum: first, that a fake
  `AWSConfigLoader` is invoked by `New` with the options produced by
  `GetAwsConfigOptions`; second, that a fake `STSClientFactory` is used in place of
  `sts.NewFromConfig` on the assume-role paths; third, that passing
  `WithAWSConfigLoader(nil)` or `WithSTSClientFactory(nil)` returns the
  "AWS config loader and STS client factory are required" error rather than
  panicking later.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No blocking issues found.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
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.

1 participant