CXP-116 Containerization and refactor of caches#101
CXP-116 Containerization and refactor of caches#101JavierCarnelli-ConductorOne wants to merge 15 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces legacy pagination/token+annotations APIs with SyncOpAttrs/SyncOpResults across connector resource types, moves external-id/config validation into the connector, and adds session-backed caching and paginated state encoding for AWS SSO permission-set lists, details, grants, and entitlements. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Connector
participant Session as sessions.SessionStore
participant SSO as AWS SSO Admin API
rect rgba(200,200,255,0.5)
Client->>Session: GetJSON("permission-sets")
alt cached list present
Session-->>Client: permissionSetIDs
else cache miss
Session-->>Client: nil
Client->>SSO: ListPermissionSets()
SSO-->>Client: permissionSetIDs
Client->>Session: SetJSON("permission-sets", permissionSetIDs)
end
Client->>Session: GetManyJSON(["permission-set:<id>...", ...])
alt some details missing
Client->>SSO: DescribePermissionSet(id)
SSO-->>Client: permissionSetDetail
Client->>Session: SetJSON("permission-set:<id>", permissionSetDetail)
end
Client-->>Client: build entitlements/grants (encode page state if truncated)
Client-->>Client: return []*Entitlement / []*Grant + SyncOpResults
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
JavierCarnelli-ConductorOne
requested review from
a team,
FeliLucero1,
agustin-conductor,
aldevv,
btipling,
johnallers,
luisina-santos,
mateoHernandez123,
mateovespConductor and
sergiocorral-conductorone
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/config/config_test.go (1)
21-33:⚠️ Potential issue | 🟡 MinorAlign test validation with single‑hop behavior.
The test currently requires an external ID whenever
use-assumeis set, but the production validator only enforces it when a global role ARN is configured. This makes the tests reject valid single‑hop configs.Suggested fix
func validateConfig(ctx context.Context, v *viper.Viper) error { if v.GetBool(config.UseAssumeField.FieldName) { - err := connector.ValidateExternalID(v.GetString(config.ExternalIdField.FieldName)) - if err != nil { - return err - } - err = connector.IsValidRoleARN(v.GetString(config.RoleArnField.FieldName)) - if err != nil { - return err - } + if err := connector.IsValidRoleARN(v.GetString(config.RoleArnField.FieldName)); err != nil { + return err + } + if v.GetString(config.GlobalRoleArnField.FieldName) != "" { + if err := connector.ValidateExternalID(v.GetString(config.ExternalIdField.FieldName)); err != nil { + return err + } + } } return nil }pkg/connector/iam_user.go (1)
233-255:⚠️ Potential issue | 🟡 MinorBug: Inverted error-check logic in
CreateAccount.The condition
if result, err := o.iamClient.GetUser(...); err == nilmeans the user was found successfully. Inside this block, checkingerrors.As(err, &noSuchEntity)will always be false sinceerrisnil. The current logic returns the existing user as a success regardless, which may be intentional for idempotency, but the deadNoSuchEntityExceptioncheck is misleading.If the intent is to return an error when GetUser fails (for reasons other than "not found"), the logic should be inverted:
Suggested fix
- if result, err := o.iamClient.GetUser(ctx, &iam.GetUserInput{UserName: awsSdk.String(username)}); err == nil { - var noSuchEntity *iamTypes.NoSuchEntityException - if errors.As(err, &noSuchEntity) { - return nil, nil, nil, fmt.Errorf("aws-connector: iam.GetUser failed: %w", err) - } + result, err := o.iamClient.GetUser(ctx, &iam.GetUserInput{UserName: awsSdk.String(username)}) + if err != nil { + var noSuchEntity *iamTypes.NoSuchEntityException + if !errors.As(err, &noSuchEntity) { + return nil, nil, nil, fmt.Errorf("aws-connector: iam.GetUser failed: %w", err) + } + // User doesn't exist, proceed to create + } else { + // User already exists, return it annos := &v2.V1Identifier{ Id: awsSdk.ToString(result.User.Arn), }
🤖 Fix all issues with AI agents
In `@pkg/connector/account.go`:
- Around line 33-37: The comment above the cache key constants (the
"SessionStore cache keys" comment) is missing a terminating period; update that
comment to end with a period so the linter is satisfied. Locate the comment
above permissionSetsCacheKey and permissionSetDetailKeyPrefix in the file
(pkg/connector/account.go) and add the period to the end of the sentence.
In `@pkg/connector/connector.go`:
- Around line 36-38: The externalIDLengthMaximum constant
(externalIDLengthMaximum) is off-by-one compared to the ValidateExternalID logic
and error text; update externalIDLengthMaximum to 64 (to match
ValidateExternalID and the error message) or alternatively make
ValidateExternalID and its error message consistent with the constant—ensure the
symbols externalIDLengthMaximum, externalIDLengthMinimum and the
ValidateExternalID function all use the same max value so invalid IDs aren’t
allowed through.
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
644-711: Consider logging cache errors for observability.The caching implementation silently ignores errors from
session.GetJSON,session.SetJSON, andsession.SetManyJSON. While this is a common pattern for optional caching, logging these errors at debug level would help with troubleshooting cache issues in production.Example: Add debug logging for cache errors
func (o *accountResourceType) getPermissionSets(ctx context.Context, ss sessions.SessionStore) ([]*awsSsoAdminTypes.PermissionSet, error) { + l := ctxzap.Extract(ctx) if ss != nil { cached, found, err := session.GetJSON[[]*awsSsoAdminTypes.PermissionSet](ctx, ss, permissionSetsCacheKey) - if err == nil && found { + if err != nil { + l.Debug("aws-connector: failed to get permission sets from cache", zap.Error(err)) + } else if found { return cached, nil } }
There was a problem hiding this comment.
I agree with the comments left by @agustin-conductor . I think these changes are necessary
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/connector/account.go (2)
293-293: Dead code:assignmentsInput.NextTokenassignment is unused.The AWS SDK paginator manages pagination internally. Setting
assignmentsInput.NextTokenmanually has no effect since the paginator doesn't use this input after initialization. This line can be removed.🧹 Proposed fix
} - assignmentsInput.NextToken = assignmentsResp.NextToken if !assignmentsPaginator.HasMorePages() { break }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/account.go` at line 293, Remove the dead assignment to assignmentsInput.NextToken (it is set from assignmentsResp.NextToken) since the AWS SDK paginator manages pagination and ignores changes to the input after initialization; locate the pagination loop where assignmentsInput and assignmentsResp are used (the block that iterates with the paginator) and delete the line setting assignmentsInput.NextToken so the code relies solely on the paginator for advancing pages.
720-723: Inconsistent error handling for session operations.In
getPermissionSets, errors fromsession.GetManyJSONare silently ignored (line 722), while inGrants(lines 183-184), similar errors cause the function to return an error. The same inconsistency exists forSetManyJSON.If session errors should be fatal in
Grants, they should likely be consistent here. Alternatively, if ignoring cache errors is intentional (fall back to API), consider applying the same lenient approach inGrantsfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/account.go` around lines 720 - 723, getPermissionSets currently swallows errors from session.GetManyJSON while Grants returns on similar session errors, causing inconsistent behavior; update getPermissionSets (and mirror in any SetManyJSON usage) to handle session errors consistently with Grants by checking the error returned from session.GetManyJSON/SetManyJSON (when ss != nil) and either return the error up the stack (make it fatal like in Grants) or instead log the error and continue (make Grants lenient) — pick one approach and apply it consistently, referencing the session variable ss, the cachedDetails variable, and the GetManyJSON/SetManyJSON calls so the change is applied in the correct functions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/account.go`:
- Line 304: Fix the typo in the error message string by replacing "writting"
with "writing" in the fmt.Errorf call that returns "aws-connector: Session
Storage writting operation failed: %w" (found in pkg/connector/account.go);
update that message so it reads "aws-connector: Session Storage writing
operation failed: %w" to correct the spelling while preserving the error wrap.
---
Nitpick comments:
In `@pkg/connector/account.go`:
- Line 293: Remove the dead assignment to assignmentsInput.NextToken (it is set
from assignmentsResp.NextToken) since the AWS SDK paginator manages pagination
and ignores changes to the input after initialization; locate the pagination
loop where assignmentsInput and assignmentsResp are used (the block that
iterates with the paginator) and delete the line setting
assignmentsInput.NextToken so the code relies solely on the paginator for
advancing pages.
- Around line 720-723: getPermissionSets currently swallows errors from
session.GetManyJSON while Grants returns on similar session errors, causing
inconsistent behavior; update getPermissionSets (and mirror in any SetManyJSON
usage) to handle session errors consistently with Grants by checking the error
returned from session.GetManyJSON/SetManyJSON (when ss != nil) and either return
the error up the stack (make it fatal like in Grants) or instead log the error
and continue (make Grants lenient) — pick one approach and apply it
consistently, referencing the session variable ss, the cachedDetails variable,
and the GetManyJSON/SetManyJSON calls so the change is applied in the correct
functions.
agustin-conductor
left a comment
agustin-conductor
left a comment
There was a problem hiding this comment.
I think we might be able to process accounts entitlements more than 1 permission set at the time, other than that, LGTM
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Claude finished @JavierCarnelli-ConductorOne's task in 2m 18s —— View job PR Review: CXP-116 Containerization and Refactor of Caches
OverviewThis PR makes three major changes:
The overall architecture is solid. The Issues1. Bug (pre-existing): Inverted error-check logic in
|
|
Claude finished @JavierCarnelli-ConductorOne's task in 2m 19s —— View job PR Review: CXP-116 Containerization and Refactor of Caches
OverviewThis PR makes three major changes:
The PR has addressed the majority of prior review feedback well. The generic Issues1. Bug (pre-existing): Inverted error-check logic in
|
|
Claude finished @JavierCarnelli-ConductorOne's task in 3m 22s —— View job PR Review: CXP-116 Containerization and Refactor of Caches
OverviewThis PR makes three major changes:
The PR has addressed the vast majority of prior review feedback (7+ rounds). Generic Remaining Issues1. Bug (pre-existing): Inverted error-check logic in
|
| # | Severity | Description | Blocking? |
|---|---|---|---|
| 1 | Bug (pre-existing) | CreateAccount inverted error check — dead code + silent error swallowing |
No (pre-existing) |
| 2 | Medium | Test/production validation divergence on external-id | Should fix |
| 3 | Low | Missing ctx.Done() in pagination loops |
Nice to have |
| 4 | Low | account_iam.go missing wrapAWSError |
Should fix for consistency |
| 5 | Low (pre-existing) | secrets.go error swallowing |
No (pre-existing) |
| 6 | Nit | One inconsistent error prefix | Nice to have |
Issues 1 and 5 are pre-existing and don't need to block this PR. Issues 2 and 4 are worth fixing in this PR or a fast follow-up.
Migrate to ConnectorBuilderV2 and ResourceSyncerV2 interfaces to enable containerized Lambda deployment following the SDK v0.5.0+ pattern. - Update main.go to use config.RunConnector() pattern - Migrate all resource syncers to V2 interface signatures - Move validation functions to connector package to resolve import cycle - Add lambda support build tags to Makefile - Remove outdated Dockerfile Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> refactor: converts in memory caches to session storage cached data chore: adds final period to comment refactor: optimize session storage with batch operations Replace individual GetJSON/SetJSON calls with batch GetManyJSON/SetManyJSON operations in the Grants() method to reduce session storage round-trips from O(2N) to O(2) per account, improving Lambda performance. - Refactor Grants() into 5 phases: collect IDs, batch cache read, fetch API misses, build grants, batch cache write - Rename getPermissionSet to fetchPermissionSetFromAPI (pure API fetcher) - Enable SessionStore in connector runner - Add early return when no permission sets provisioned Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> fix: handles Session Storage operation errors refactor: use SDK pagination in Grants() for Lambda compatibility Replace internal for loops with SDK pagination via NextPageToken to: - Save progress per permission set, reducing blast radius on 429 errors - Enable retries to resume from last successful page instead of restarting - Cache permission set IDs per account across paginated calls Also adds wrapAWSError() to convert AWS throttling errors to codes.Unavailable for proper handling by baton-sdk retry mechanism. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> refactor: use SDK pagination in Entitlements() for Lambda compatibility Apply the same pagination pattern from Grants() to Entitlements(): - Process one permission set per call instead of all at once - Cache permission set IDs globally per sync - Return NextPageToken to save progress between calls - Remove unused getPermissionSets() function and permissionSetsCacheKey This reduces blast radius on 429 errors - only current permission set's work is lost instead of all progress. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…rt ops so we don't show in UI
…udes all resource types
…dling Move awsThrottleErrorCodes and wrapAWSError from account.go to helpers.go so the utility is available package-wide. Apply wrapAWSError to every AWS API call error return in role.go, iam_group.go, iam_user.go, sso_user.go, sso_group.go, and secrets.go so throttling errors from any builder surface as codes.Unavailable for the baton-sdk sync engine to retry. Add a comment documenting the intentional status.Error chain-breaking trade-off. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…enerics Consolidates encodeGrantsPageToken, decodeGrantsPageToken, encodeEntitlementsPageToken, and decodeEntitlementsPageToken into two generic functions encodePageToken[T] and decodePageToken[T] that share the identical JSON↔base64 logic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
laurenleach
force-pushed
the
jallers/cxp-116-containerization
branch
from
828e4c7 to
b7b5cc7
Compare
|
Claude finished @laurenleach's task in 2m 23s —— View job PR Review: CXP-116 Containerization and Refactor of Caches
OverviewThis PR makes three major changes:
The PR has addressed the vast majority of feedback from 7+ prior review rounds. The generic Remaining Issues1. Bug (pre-existing): Inverted error-check logic in
|
| # | Severity | Description | Blocking? |
|---|---|---|---|
| 1 | Bug (pre-existing) | CreateAccount inverted error check — dead code + silent error swallowing |
No (pre-existing) |
| 2 | Medium | Test/production validation divergence on external-id | Should fix |
| 3 | Low | account_iam.go missing wrapAWSError wrapping |
Should fix for consistency |
| 4 | Nit | One inconsistent error prefix in connector.go:262 |
Nice to have |
| 5 | Low (pre-existing) | secrets.go error swallowing |
No (pre-existing) |
Issues 1 and 5 are pre-existing and don't need to block this PR. Issues 2 and 3 are worth fixing in this PR or a fast follow-up.
Documentation gaps found during containerization validationThe following items were surfaced during the CXP-116 QA plan review and have been addressed in a separate PR: PR #111:
|
8 participants
Updates the connector for it to be able to containerize.
This applies changes to the structure, the workflows and refactors the use of caches on the connector. Also implements ResourceSyncerV2.
Will require extensive testing and validations.
Summary by CodeRabbit