[BB-1449] Improve account assignment handling by adding account status verification #92
Conversation
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
FeliLucero1
requested review from
a team,
JavierCarnelli-ConductorOne,
agustin-conductor,
aldevv,
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
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
560-566: Consider addingAccessDeniedExceptionhandling in the Revoke retry loop for consistency.The Grant retry loop (lines 369-375) handles
AccessDeniedExceptionby logging and continuing to retry. The Revoke retry loop lacks this handling. While the initial Revoke status check setscomplete = trueon access denial (preventing loop entry), if the first check succeeds but a subsequent poll encounters a transient permission issue, Revoke would fail whereas Grant would retry.For consistency, consider adding similar handling:
complete, err = o.checkDeleteAccountAssignmentStatus(waitCtx, l, deleteOut.AccountAssignmentDeletionStatus) if err != nil { if strings.Contains(err.Error(), "Received a 404 status error") { l.Info("aws-connector: Grant already revoked.") annos.Append(&v2.GrantAlreadyRevoked{}) return annos, nil } + var ae *awsSsoAdminTypes.AccessDeniedException + if errors.As(err, &ae) { + l.Warn("aws-connector: access denied while checking deletion status, will retry", zap.Error(err)) + complete = false + continue + } return nil, err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
README.mdis excluded by none and included by none
📒 Files selected for processing (2)
pkg/connector/account.go(6 hunks)pkg/connector/connector.go(1 hunks)
🔇 Additional comments (4)
pkg/connector/connector.go (1)
158-168: LGTM - Sensible optimization to avoid redundant STS call.The short-circuit correctly bypasses the second assume-role when the source and target roles match, reusing the already-validated
bindingCreds. The config construction mirrors the pattern used in the non-short-circuit path.pkg/connector/account.go (3)
315-324: LGTM - Good error context and wrapping.The
ConflictExceptionhandling provides useful context (account ID, potential cause, actionable tip) while preserving the original error chain via%w.
344-349: LGTM - Conservative approach to access denial during status checks.Setting
complete = falseand retrying until timeout is safer than assuming success when permissions prevent verification. This handles transient permission issues gracefully.Also applies to: 369-377
506-515: LGTM - Consistent with Grant path.
FeliLucero1
left a comment
FeliLucero1
left a comment
There was a problem hiding this comment.
Mateo fixed ci test here #91 , I'm wating for it to be merged
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/account.go (1)
558-558: Use waitCtx.Err() for semantic correctness.The error should wrap
waitCtx.Err()instead ofctx.Err()sincewaitCtxis the context that timed out. This is inconsistent with the Grant method at line 364, which correctly useswaitCtx.Err().- return nil, fmt.Errorf("aws-connector: account assignment deletion timed out: %w", ctx.Err()) + return nil, fmt.Errorf("aws-connector: account assignment deletion timed out: %w", waitCtx.Err())
♻️ Duplicate comments (1)
pkg/connector/account.go (1)
477-491: Consider switch statement for consistency.Similar to the Grant method, this if-else chain could be refactored to a switch statement as suggested by static analysis. For code consistency, consider applying the same refactoring pattern to both Grant and Revoke methods.
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
284-298: Consider switch statement for error type checking.The static analysis tool suggests refactoring the if-else chain to a switch statement for consistency with Go idioms. While the current code is correct, a switch may be slightly cleaner.
- if err != nil { - var ade *types.AccessDeniedException - if errors.As(err, &ade) { - // we don't have permissions to verify: warn but continue (backward compatibility) - // this maintains backward compatibility with clients that don't have the permission - // if the account is suspended, AWS will return ConflictException that we handle later - couldNotVerifyStatus = true - l := ctxzap.Extract(ctx).With(zap.String("account_id", binding.AccountID)) - l.Warn("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). " + - "Proceeding with assignment creation. If account is suspended, this will fail with ConflictException. " + - "Consider adding organizations:DescribeAccount to your IAM policy for better error messages.") - } else { - // other errors: fail - return nil, fmt.Errorf("aws-connector: DescribeAccount failed: %w", err) - } + switch { + case err != nil: + var ade *types.AccessDeniedException + if errors.As(err, &ade) { + // we don't have permissions to verify: warn but continue (backward compatibility) + // this maintains backward compatibility with clients that don't have the permission + // if the account is suspended, AWS will return ConflictException that we handle later + couldNotVerifyStatus = true + l := ctxzap.Extract(ctx).With(zap.String("account_id", binding.AccountID)) + l.Warn("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). " + + "Proceeding with assignment creation. If account is suspended, this will fail with ConflictException. " + + "Consider adding organizations:DescribeAccount to your IAM policy for better error messages.") + } else { + // other errors: fail + return nil, fmt.Errorf("aws-connector: DescribeAccount failed: %w", err) + } + case descOut.Account == nil: + return nil, fmt.Errorf("aws-connector: DescribeAccount returned nil account for %s", binding.AccountID) + case descOut.Account.Status != types.AccountStatusActive: + // if we could verify and the account is not active, fail + return nil, fmt.Errorf("aws-connector: account %s is not active, status: %s", binding.AccountID, descOut.Account.Status) + } - } else if descOut.Account == nil { - return nil, fmt.Errorf("aws-connector: DescribeAccount returned nil account for %s", binding.AccountID) - } else if descOut.Account.Status != types.AccountStatusActive { - // if we could verify and the account is not active, fail - return nil, fmt.Errorf("aws-connector: account %s is not active, status: %s", binding.AccountID, descOut.Account.Status) - }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/account.go(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
pkg/connector/account.go
[error] 284-284: golangci-lint: ifElseChain: rewrite if-else to switch statement (gocritic).
🪛 GitHub Check: go-lint
pkg/connector/account.go
[failure] 284-284:
ifElseChain: rewrite if-else to switch statement (gocritic)
[failure] 477-477:
ifElseChain: rewrite if-else to switch statement (gocritic)
🔇 Additional comments (6)
pkg/connector/account.go (6)
299-300: Nil check correctly addresses previous review feedback.The defensive nil check on
descOut.Accountproperly addresses the potential nil pointer dereference flagged in previous reviews. This ensures graceful failure with a clear error message instead of a panic.
317-326: Excellent enhanced error handling for ConflictException.The improved error message provides clear context about potential suspended accounts and actionable guidance when permissions are missing. This significantly improves the developer experience.
371-377: Appropriate AccessDeniedException handling in retry loop.The retry logic correctly handles permission issues by logging a warning and continuing to retry rather than failing immediately. This maintains backward compatibility while still attempting to verify completion within the timeout window.
492-493: Nil check correctly addresses previous review feedback.The defensive nil check on
descOut.Accountin the Revoke method properly addresses the potential nil pointer dereference flagged in previous reviews, consistent with the fix in the Grant method.
510-519: Consistent and helpful ConflictException handling.The error handling mirrors the Grant method's approach, providing consistent user experience across both operations with actionable guidance for resolving issues.
563-571: Consider handling AccessDeniedException in Revoke retry loop for consistency.The Grant method's retry loop (lines 371-377) handles
AccessDeniedExceptionby logging a warning and continuing to retry. The Revoke retry loop only handles 404 errors. While the initial status check in Revoke assumes completion onAccessDeniedException(line 546), it would be more consistent to handle it in the retry loop as well.Consider whether this asymmetry is intentional or if similar handling should be added:
complete, err = o.checkDeleteAccountAssignmentStatus(waitCtx, l, deleteOut.AccountAssignmentDeletionStatus) if err != nil { if strings.Contains(err.Error(), "Received a 404 status error") { l.Info("aws-connector: Grant already revoked.") annos.Append(&v2.GrantAlreadyRevoked{}) return annos, nil } + var ae *awsSsoAdminTypes.AccessDeniedException + if errors.As(err, &ae) { + l.Warn("aws-connector: access denied while checking deletion status, will retry", zap.Error(err)) + complete = false + continue + } return nil, err }
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/account.go (2)
347-386: Inconsistent AccessDenied handling between Grant and Revoke.In the
Grantfunction, whenAccessDeniedExceptionoccurs during status checks (lines 350-356, 377-381), the code logs a warning and continues without assuming completion. However, in theRevokefunction at lines 554-556,AccessDeniedExceptionis handled by assuming completion (complete = true).This inconsistency means:
- Grant: Retries until timeout if access is denied during status checks
- Revoke: Immediately assumes success if access is denied during initial status check
Consider aligning the behavior. If the operations have different risk profiles (where assuming deletion success is safer than assuming creation success), document this reasoning. Otherwise, apply consistent handling.
var ae *awsSsoAdminTypes.AccessDeniedException if errors.As(err, &ae) { - // we couldn't verify the status, but we already started the operation - // in this case, we can't assume success - it could be in progress or have failed - // but for backward compatibility, we assume it will eventually complete - l.Warn("aws-connector: access denied while attempting to check status. Cannot verify if account assignment was created. Operation may still be in progress.", zap.Error(err)) - // we don't assume complete, but we don't fail - we let the timeout handle this + // we couldn't verify the status, but we already started the operation + // we continue retrying until timeout + l.Warn("aws-connector: access denied while checking status, will retry", zap.Error(err)) complete = false + // Note: Different from Revoke which assumes completion. Consider documenting why. } else {Or update Revoke to match Grant's approach (continue retrying instead of assuming completion).
568-568: UsewaitCtx.Err()instead ofctx.Err()for timeout error.Line 568 returns
ctx.Err()in the timeout case, but should returnwaitCtx.Err()to correctly reflect the timeout context's cancellation. This is inconsistent with theGrantfunction at line 369, which correctly useswaitCtx.Err().case <-waitCtx.Done(): - return nil, fmt.Errorf("aws-connector: account assignment deletion timed out: %w", ctx.Err()) + return nil, fmt.Errorf("aws-connector: account assignment deletion timed out: %w", waitCtx.Err()) case <-time.After(AccountAssignmentRetryDelay):
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
474-507: Consider extracting duplicate account status verification logic.This account status verification code is nearly identical to lines 276-309 in the
Grantfunction. Extracting this into a helper function would reduce duplication and make future updates easier.Example refactor:
// Add before Grant function: func (o *accountResourceType) verifyAccountStatus(ctx context.Context, accountID string, operation string) (couldNotVerify bool, err error) { descOut, err := o.orgClient.DescribeAccount(ctx, &awsOrgs.DescribeAccountInput{ AccountId: awsSdk.String(accountID), }) if err != nil { var accessDeniedErr *types.AccessDeniedException if errors.As(err, &accessDeniedErr) { l := ctxzap.Extract(ctx).With(zap.String("account_id", accountID)) l.Warn(fmt.Sprintf("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). "+ "Proceeding with assignment %s. If account is suspended, this will fail with ConflictException. "+ "Consider adding organizations:DescribeAccount to your IAM policy for better error messages.", operation)) return true, nil } return false, fmt.Errorf("aws-connector: DescribeAccount failed: %w", err) } if descOut.Account == nil { return false, fmt.Errorf("aws-connector: DescribeAccount returned nil account for %s", accountID) } if descOut.Account.Status != types.AccountStatusActive { return false, fmt.Errorf("aws-connector: account %s is not active, status: %s", accountID, descOut.Account.Status) } return false, nil }Then use:
couldNotVerifyStatus, err := o.verifyAccountStatus(ctx, binding.AccountID, "creation")in Grant and"deletion"in Revoke.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/account.go(6 hunks)
🔇 Additional comments (3)
pkg/connector/account.go (3)
276-309: LGTM! Well-designed pre-flight check with graceful degradation.The account status verification logic correctly balances safety with backward compatibility. The nil check for
descOut.Accountaddresses the previous review concern, and the AccessDenied handling preserves compatibility with deployments lackingorganizations:DescribeAccountpermission while still providing helpful guidance.
320-333: LGTM! Clear error messaging with actionable guidance.The ConflictException handling provides helpful context about suspended accounts and conditionally suggests adding permissions when verification wasn't possible. The error wrapping preserves the original error for debugging.
518-531: LGTM! Consistent ConflictException handling.The error handling mirrors the
Grantfunction's approach, providing helpful context about suspended accounts and permission suggestions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
311-398: Good integration of account verification with clear error messaging.The Grant flow correctly integrates account status verification:
- Checks account status before attempting assignment creation
- Provides enhanced error messages for
ConflictExceptionwith actionable guidance- Handles
AccessDeniedExceptionduring status checks by continuing retries rather than failing immediately- Updates timeout messages to reflect verification limitations
One consideration: when
AccessDeniedExceptionoccurs during status checks (lines 385-391), the code retries until the full 5-minute timeout. Since permissions don't typically change during this window, users will always wait the full duration when missingssoadmin:DescribeAccountAssignmentCreationStatuspermissions. This is safe but potentially frustrating. The current approach prioritizes correctness (the operation might still complete) over user experience, which is reasonable for this critical path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/account.go(10 hunks)
🔇 Additional comments (2)
pkg/connector/account.go (2)
249-282: LGTM! Solid implementation with good error handling.The
verifyAccountStatusmethod is well-structured with appropriate error handling:
- Gracefully handles
AccessDeniedExceptionwith a clear warning message that guides users to add the missing permission- Includes the nil check on
descOut.Account(addresses past review comments)- Provides clear error messages with context (account ID, status)
- Maintains backward compatibility by allowing operations to proceed when verification permissions are missing
483-581: LGTM! Consistent implementation with Grant flow.The Revoke flow mirrors the Grant implementation with:
- Account status verification before deletion
- Enhanced
ConflictExceptionhandling with contextual guidance- Appropriate
AccessDeniedExceptionhandling that continues retrying until timeout- Clear timeout error messages
The consistency between Grant and Revoke flows is excellent for maintainability.
…ng retry logic with explicit error messages for access denied scenarios.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/connector/account.go (2)
358-391: Inconsistent AccessDeniedException handling between initial check and retry loop.There's a logic inconsistency here:
- Lines 358-369: On initial
AccessDeniedException, the comment states "we let the timeout handle this" and setscomplete = falseto continue retrying.- Lines 385-391: In the retry loop,
AccessDeniedExceptionimmediately returns an error.This means the code will: (1) get
AccessDeniedException, (2) setcomplete = false, (3) enter the loop, (4) wait 1 second, (5) retry, (6) fail immediately on the same exception.Since
AccessDeniedExceptionis a permissions issue that won't change between retries, consider making the behavior consistent. Based on the previous review feedback, failing immediately would be more appropriate:if err != nil { var ae *awsSsoAdminTypes.AccessDeniedException if errors.As(err, &ae) { - // we couldn't verify the status, but we already started the operation - // in this case, we can't assume success - it could be in progress or have failed - // we continue retrying until timeout - l.Warn("aws-connector: access denied while attempting to check status. Cannot verify if account assignment was created. Operation may still be in progress.", zap.Error(err)) - // we don't assume complete, but we don't fail - we let the timeout handle this - complete = false + // permission issues won't resolve on retry - fail immediately + return nil, fmt.Errorf("aws-connector: access denied while checking account assignment creation status. Cannot verify if operation completed. Missing required permissions: %w", err) } else { return nil, err } }
533-570: Same inconsistent AccessDeniedException handling as in Grant.The same inconsistency exists here: lines 533-543 set
complete = falseto continue retrying, but lines 566-570 fail immediately onAccessDeniedExceptionin the retry loop.Apply the same fix pattern as suggested for Grant:
if err != nil { ... var ae *awsSsoAdminTypes.AccessDeniedException if errors.As(err, &ae) { - // we couldn't verify the status, but we already started the operation - // in this case, we can't assume success - it could be in progress or have failed - // we continue retrying until timeout - l.Warn("aws-connector: access denied while attempting to check status. Cannot verify if account assignment deletion was completed. Operation may still be in progress.", zap.Error(err)) - // we don't assume complete, but we don't fail - we let the timeout handle this - complete = false + // permission issues won't resolve on retry - fail immediately + return nil, fmt.Errorf("aws-connector: access denied while checking account assignment deletion status. Cannot verify if operation completed. Missing required permissions: %w", err) } else { return nil, err } }
🧹 Nitpick comments (1)
pkg/connector/account.go (1)
249-282: LGTM! Well-structured account verification with proper nil check.The method correctly handles the AccessDeniedException for backward compatibility while providing helpful logging. The nil check for
descOut.Accountaddresses the previous review concern.Minor style note: The log message construction at lines 265-267 could be simplified using a single
fmt.Sprintf:l.Warn(fmt.Sprintf("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). "+ "Proceeding with assignment %s. If account is suspended, this will fail with ConflictException. "+ "Consider adding organizations:DescribeAccount to your IAM policy for better error messages.", operation))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.github/workflows/ci.yamlis excluded by none and included by none
📒 Files selected for processing (1)
pkg/connector/account.go(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
pkg/connector/account.go (3)
311-318: LGTM!Good placement of the account status verification before creating the assignment. The captured
couldNotVerifyStatusflag is appropriately used for conditional error messaging downstream.
331-340: LGTM! Good contextual error messaging.The ConflictException handling provides helpful context about the potential cause (suspended account) and actionable guidance when the status couldn't be verified due to missing permissions.
480-509: LGTM! Consistent with Grant implementation.The Revoke method follows the same pattern as Grant for account status verification and ConflictException handling, providing consistent behavior and error messaging across both operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/connector/account.go (2)
356-386: Good handling of AccessDeniedException, but timeout error could be more precise.The immediate return on AccessDeniedException when checking operation status is correct - it prevents pointless retries when permissions are missing (addressing a previous review concern). The distinction between treating
AccessDeniedExceptionas non-fatal during verification (line 260-268) but fatal during status checking is appropriate: verification is optional for backward compatibility, but being unable to confirm the outcome of an initiated operation is a genuine problem.However, the timeout error message "Cannot verify if operation completed successfully" may be misleading—the timeout could occur because the operation is genuinely slow, not because verification failed. Consider distinguishing between "operation timed out while in progress" versus "timed out and unable to verify final status."
Example refinement:
- return nil, fmt.Errorf("aws-connector: account assignment creation timed out. Cannot verify if operation completed successfully: %w", waitCtx.Err()) + return nil, fmt.Errorf("aws-connector: account assignment creation did not complete within %s: %w", AccountAssignmentMaxWaitDuration, waitCtx.Err())
519-560: Consistent with Grant, same timeout message consideration applies.The AccessDeniedException handling in Revoke mirrors Grant—returning immediately prevents retrying when permissions are missing, which is correct.
The timeout error message has the same consideration as noted in Grant: it could more precisely distinguish between "operation is slow" versus "cannot verify completion." The same optional refactor suggested for Grant applies here:
- return nil, fmt.Errorf("aws-connector: account assignment deletion timed out. Cannot verify if operation completed successfully: %w", waitCtx.Err()) + return nil, fmt.Errorf("aws-connector: account assignment deletion did not complete within %s: %w", AccountAssignmentMaxWaitDuration, waitCtx.Err())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/connector/account.go(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
pkg/connector/account.go (3)
249-282: LGTM! Well-designed verification method.The
verifyAccountStatusmethod is well-implemented with appropriate error handling:
- Gracefully handles missing
organizations:DescribeAccountpermission by logging a warning and allowing the operation to continue, maintaining backward compatibility.- Includes defensive nil check for
descOut.Account, addressing the previous review concern.- Fails fast when account status is not active, preventing unnecessary API calls.
The dual return value
(couldNotVerifyStatus bool, error)cleanly distinguishes between "couldn't verify but should proceed" and "verification failed."
311-340: LGTM! Enhanced error handling with helpful user guidance.The integration of account status verification before the assignment operation is well-executed:
- Proactively verifies account status to avoid ConflictException when possible.
- ConflictException error messages now include the account ID and helpful context about suspended accounts, with a conditional tip to add the permission when verification was not possible.
The error messages strike a good balance between being informative and actionable.
474-503: LGTM! Consistent implementation with Grant method.The Revoke method mirrors the Grant improvements:
- Verifies account status before deletion, with the same backward-compatible handling of missing permissions.
- ConflictException handling includes account context and conditional permission hint, matching the Grant pattern.
The symmetry between Grant and Revoke strengthens maintainability.
| complete, err = o.checkCreateAccountAssignmentStatus(waitCtx, l, createOut.AccountAssignmentCreationStatus) | ||
| if err != nil { | ||
| return nil, err | ||
| var ae *awsSsoAdminTypes.AccessDeniedException |
There was a problem hiding this comment.
why checkCreateAccountAssignmentStatus could return access denied error and we should trait it different here?
There was a problem hiding this comment.
Just backward compatibility
| } | ||
| return nil, err | ||
|
|
||
| var ae *awsSsoAdminTypes.AccessDeniedException |
There was a problem hiding this comment.
same question here, why it is affect backward compatibility?
There was a problem hiding this comment.
It affects backward compatibility because some clients may have permissions to delete assignments (sso:DeleteAccountAssignment) but not to check the status (sso:DescribeAccountAssignmentDeletionStatus).
…use gRPC status codes
btipling
left a comment
btipling
left a comment
There was a problem hiding this comment.
Maybe I am mistaken, but this PR seems to assume warns are surfaced to users?
| // this maintains backward compatibility with clients that don't have the permission | ||
| // if the account is suspended, AWS will return ConflictException that we handle later | ||
| l := ctxzap.Extract(ctx).With(zap.String("account_id", accountID)) | ||
| l.Warn("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). " + |
There was a problem hiding this comment.
This should be a Debug message, Chris is removing warns
| l := ctxzap.Extract(ctx).With(zap.String("account_id", accountID)) | ||
| l.Warn("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). " + | ||
| fmt.Sprintf("Proceeding with assignment %s. If account is suspended, this will fail with ConflictException. ", operation) + | ||
| "Consider adding organizations:DescribeAccount to your IAM policy for better error messages.") |
There was a problem hiding this comment.
If this is meant for customers, they don't see these warn log messages, they go into datadog.
| // if the account is suspended, AWS will return ConflictException that we handle later | ||
| l := ctxzap.Extract(ctx).With(zap.String("account_id", accountID)) | ||
| l.Warn("aws-connector: Cannot verify account status (missing organizations:DescribeAccount permission). " + | ||
| fmt.Sprintf("Proceeding with assignment %s. If account is suspended, this will fail with ConflictException. ", operation) + |
There was a problem hiding this comment.
You don't need to use Sprintf zap loggers, use zap types
| if errors.As(err, &ae) { | ||
| l.Info("aws-connector: access denied while attempting to check status. Assuming account assignment creation is complete.", zap.Error(err)) | ||
| // we don't have permissions to verify: warn but assume complete (backward compatibility) | ||
| l.Warn("aws-connector: access denied while attempting to check status. Assuming account assignment creation is complete.", zap.Error(err)) |
| var ae *awsSsoAdminTypes.AccessDeniedException | ||
| if errors.As(err, &ae) { | ||
| // we don't have permissions to verify: warn but assume complete (backward compatibility) | ||
| l.Warn("aws-connector: access denied while attempting to check status. Assuming account assignment creation is complete.", zap.Error(err)) |
| var accessDeniedErr *types.AccessDeniedException | ||
| switch { | ||
| case errors.As(err, &accessDeniedErr): | ||
| // we don't have permissions to verify: warn but continue (backward compatibility) |
There was a problem hiding this comment.
We should not warn. Customers do not see warning logs. They see errors.
DOC: https://conductorone.atlassian.net/wiki/spaces/~71202071336165eb264ed294c4e93f1fb25f01/pages/2779086859/AWS+connector+error+during+account+assignment
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.