fix incorrect error handling in route53 dns provider#8167
fix incorrect error handling in route53 dns provider#8167Peac36 wants to merge 1 commit intocert-manager:masterfrom
Conversation
|
Hi @Peac36. Thanks for your PR. I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes incorrect error handling in the Route53 DNS provider by refactoring the error redaction mechanism to properly preserve error chains while removing sensitive request IDs from AWS error messages.
- Replaced the problematic
removeReqIDfunction with a newRedactedErrorwrapper type - Updated error formatting to use
%wverb instead of%sto maintain error wrapping - Renamed test function to reflect the new error handling approach
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/issuer/acme/dns/route53/route53.go | Replaced removeReqID function with RedactedError type and updated error formatting |
| pkg/issuer/acme/dns/route53/route53_test.go | Updated test function name and call to use new error handling function |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
b7b9288 to
8be9473
Compare
Signed-off-by: Nikola <peac36@abv.bg>
8be9473 to
f87cf45
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| func (e *RedactedError) Error() string { | ||
| errString := e.Err.Error() | ||
| var responseError *awshttp.ResponseError | ||
| if errors.As(err, &responseError) { | ||
| if errors.As(e.Err, &responseError) { | ||
| before := responseError.Error() | ||
| // remove the request id from the error message | ||
| responseError.RequestID = "<REDACTED>" | ||
| after := responseError.Error() | ||
| return errors.New(strings.Replace(err.Error(), before, after, 1)) | ||
| return strings.Replace(errString, before, after, 1) | ||
| } | ||
| return err | ||
| return errString | ||
| } |
There was a problem hiding this comment.
The Error() method modifies the original ResponseError by setting RequestID to '', which could affect other parts of the code that reference the same error. This is a side effect that violates the principle that Error() should be a read-only operation. Consider creating a copy of the ResponseError before modifying it.
| type RedactedError struct { | ||
| Err error | ||
| } |
There was a problem hiding this comment.
The RedactedError struct should validate that Err is not nil to prevent potential panics in the Error() and Unwrap() methods. Consider adding validation in a constructor function or checking for nil in the methods.
|
/cc @wallrj I think you have been in route53-land before. 😅 |
|
/cybr |
|
@wallrj-cyberark - did you have a chance to review the changes? |
|
@Peac36 I've been trying out a slightly different approach in #8221 I moved the redaction / stabilization code nearer to where it is required and use it only where it is required (for the Reason field), so that the full detailed errors are once again included in the Kubernetes Events which are attached to the Challenge resources. |
|
Closing in favor of #8221 |
Pull Request Motivation
Fixes: #8166
/bug
CyberArk tracker: VC-46126