fix(function): handle code signing config unavailable in unsupported regions - #230
fix(function): handle code signing config unavailable in unsupported regions#230gobos12 wants to merge 5 commits into
Conversation
…regions - Add new error `ErrCodeSigningNotAvailable` for regions without AWS Signer support - Handle `AccessDeniedException` in `setFunctionCodeSigningConfig` with graceful fallback when no CSC ARN is specified - Add e2e test `test_function_code_signing_in_unsupported_region` to verify code signing behavior
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gobos12 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
'assert_condition_state_message' compares the full message (not substrings), so update assertion to read substring within AWS response instead
| rm.metrics.RecordAPICall("UPDATE", "PutFunctionCodeSigningConfig", err) | ||
| if err != nil { | ||
| if awsErr, ok := ackerr.AWSError(err); ok && awsErr.ErrorCode() == "AccessDeniedException" { | ||
| return ackerr.NewTerminalError(err) |
There was a problem hiding this comment.
@gobos12 this change does address the concern about the reported error message being misleading, but thinking about it treating AccessDenied as Terminal in every case could be problematic. Generally, we don't consider AccessDenied to be a Terminal condition as it usually means the controller doesn't have the necessary IAM permissions. In that case if the user updates the ACK controller's role with corrected permissions we don't want to force them to bump the Function' spec just for the controller to retry with the new permissions.
If we do want to use a Terminal condition here to prevent unnecessary retries when this is used in a opt-in region we can check for an error message specific to that failure mode. Although I'm not sure that this protection is necessary the error will still be reported to the user as an ACK.Recoverable condition.
| if err != nil { | ||
| if awsErr, ok := ackerr.AWSError(err); ok && awsErr.ErrorCode() == "AccessDeniedException" { | ||
| if ko.Spec.CodeSigningConfigARN != nil && *ko.Spec.CodeSigningConfigARN != "" { | ||
| return ackerr.NewTerminalError(err) |
There was a problem hiding this comment.
Similar to the above comment. We probably don't want to treat this as a Terminal condition if the we're not sure this is due to the region being unsupported. Otherwise, we could be forcing a user to bump the Functions spec to unwedge an IAM permission issue.
| rm.metrics.RecordAPICall("GET", "GetFunctionCodeSigningConfig", err) | ||
| if err != nil { | ||
| if awsErr, ok := ackerr.AWSError(err); ok && awsErr.ErrorCode() == "AccessDeniedException" { | ||
| if ko.Spec.CodeSigningConfigARN != nil && *ko.Spec.CodeSigningConfigARN != "" { |
There was a problem hiding this comment.
Just a note, while this will prevent improper updates of CodeSigningConfig it could lead to the out of band additions of a CodeSigningConfig being silently ignored where they would normally be deleted when the controller's role is not granted the GetFunctionCodeSigningConfig read permission.
3 participants
Issue #, if available: community#2971
Description of changes:
ErrCodeSigningNotAvailablefor regions without AWS Signer supportAccessDeniedExceptioninsetFunctionCodeSigningConfigwith graceful fallback when no CSC ARN is specifiedtest_function_code_signing_in_unsupported_regionto verify code signing behaviorBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.