Feat/account region protection #203
Conversation
a-hilaly
force-pushed
the
feat/account-region-protection
branch
from
513c7b4 to
1ca522a
Compare
|
/test unit-test |
a-hilaly
force-pushed
the
feat/account-region-protection
branch
from
1ca522a to
4cf28f2
Compare
michaelhtm
left a comment
michaelhtm
left a comment
There was a problem hiding this comment.
Great catch @a-hilaly!
left a few comments below
| } | ||
| parsedARN, err := arn.Parse(string(roleARN)) | ||
| if err != nil { | ||
| return ctrlrt.Result{}, fmt.Errorf("parsing role ARN %q from %q configmap: %v", roleARN, ackrtcache.ACKRoleTeamMap, err) |
There was a problem hiding this comment.
should we use https://github.com/aws-controllers-k8s/runtime/blob/main/pkg/runtime/reconciler.go#L1146-L1167 to return here? so that this error can be patched to the resource status
| region := r.getRegion(desired) | ||
| endpointURL := r.getEndpointURL(desired) | ||
| gvk := r.rd.GroupVersionKind() | ||
|
|
||
| // If the user has specified a region that is different from the | ||
| // region the resource currently exists in, we need to fail the | ||
| // reconciliation with a terminal error. | ||
| if r.regionDrifted(desired) { |
There was a problem hiding this comment.
should we use the region in line 267? It already parses resource annotation - namespace annotation - config
|
/retest |
2 similar comments
|
/retest |
|
/retest |
michaelhtm
force-pushed
the
feat/account-region-protection
branch
2 times, most recently
from
c763f55 to
2a128db
Compare
|
/test ecr-controller-test |
|
/retest |
|
/lgtm |
|
/hold |
ack-prow
bot
added
the
do-not-merge/hold
… resources Adds protection against attempting to manage AWS resources that exist in a different region or account than the controller is configured to use. This prevents accidental resource hijacking and provides clear error messages. - Add `regionDrifted()` and `accountDrifted()` helper functions - Check for drift before creating resource manager in Reconcile - Return terminal errors when drift is detected - Add comprehensive tests for both region and account drift scenarios
michaelhtm
force-pushed
the
feat/account-region-protection
branch
from
2a128db to
c271c63
Compare
|
/lgtm |
ack-prow
bot
added
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test s3-controller-test |
|
/test ec2-controller-test |
|
@a-hilaly: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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/test-infra repository. I understand the commands that are listed here. |
|
/test sagemaker-controller-test |
2 participants
Adds protection against attempting to manage AWS resources that exist in a
different region or account than the controller is configured to use. This
prevents accidental resource hijacking and provides clear error messages.
regionDrifted()andaccountDrifted()helper functionsBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.