Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/invalid-bug
|
@rh-cbent: This pull request references Jira Issue OCPBUGS-86052, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe AWS destroyer now handles ISO and ISOB partition tagging regions. ISO regions create an additional tagging client when required. Tagging-client error messages use region constants. ChangesAWS tagging partition handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Merge Risk: ⚪ Minimal · up to The AWS partition tagging changes do not show a concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 golangci-lint (2.13.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
fbb6f52 to
a1b0628
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
- pkg/destroy/aws/aws.go
- Switch out hardcoded region strings in error messages for their
constants.
- Add region tagging for AWS ISO and ISOB.
- Add handling for US ISO East-1, West-1, and ISOB East-1 Destroy API
Client creation.
a1b0628 to
1c56947
Compare
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@rh-cbent: This pull request references Jira Issue OCPBUGS-86052, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
@rh-cbent: This pull request references Jira Issue OCPBUGS-86052, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @rochacbruno |
|
/cc @tthvo |
tthvo
left a comment
tthvo
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Cross-referencing the aws-sdk-v2 endpoint specs confirms us-iso-east-1 is the region for route53 in C2S (us-iso).
Note: we don't officially support us-iso-west-1 region yet based on openshift docs, but this is a great preparation for that 👍
|
Scheduling tests matching the |
openshift-ci
Bot
added
the
approved
|
/test e2e-aws-byo-subnet-role-security-groups |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-5.1-amd64-nightly-aws-sc2s-ipi-disc-priv-fips-f7 |
|
@tthvo: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/11a63570-ae2e-11f1-954d-7feb9a315555-0 |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-5.1-amd64-nightly-aws-sc2s-ipi-disc-priv-fips-f7 |
|
@tthvo: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/b52ef1e0-ae52-11f1-9a20-cb9c1b7c1eb8-0 |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-5.1-amd64-nightly-aws-sc2s-ipi-disc-priv-fips-f7 |
|
@tthvo: trigger 2 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/f8b7fc10-b224-11f1-9dd0-50f53456bf88-0 |
| if o.Region != awstypes.UsIsoEast1RegionID { | ||
| tagClient, err := awssession.NewResourceGroupsTaggingAPIClient(ctx, awssession.EndpointOptions{ | ||
| Region: awstypes.UsIsoEast1RegionID, | ||
| Endpoints: o.endpoints, | ||
| }, "", resourcegroupstaggingapi.WithAPIOptions(awsmiddleware.AddUserAgentKeyValue(awssession.OpenShiftInstallerDestroyerUserAgent, version.Raw))) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create resource tagging client for %s: %w", awstypes.UsIsoEast1RegionID, err) | ||
| } | ||
| tagClients = append(tagClients, tagClient) | ||
| } |
There was a problem hiding this comment.
What will happen in the case of a us-iso-west-1 cluster with a custom tagging endpoint?
Will the cluster's resources still get deleted, but cause destroy cluster to hangs forever instead of exiting?
The following suggestion is conservative, it can't wedge the destroy, but it means ISO users with a custom tagging endpoint don't get the global-resource discovery.
Another option may be to keep building the client but drop Endpoints so the SDK resolves its own us-iso-east-1 endpoint and that preserves the PR's intent but assumes the default ISO hostname is reachable in the customer's network, and if it isn't you're back to the wedge.
| if o.Region != awstypes.UsIsoEast1RegionID { | |
| tagClient, err := awssession.NewResourceGroupsTaggingAPIClient(ctx, awssession.EndpointOptions{ | |
| Region: awstypes.UsIsoEast1RegionID, | |
| Endpoints: o.endpoints, | |
| }, "", resourcegroupstaggingapi.WithAPIOptions(awsmiddleware.AddUserAgentKeyValue(awssession.OpenShiftInstallerDestroyerUserAgent, version.Raw))) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create resource tagging client for %s: %w", awstypes.UsIsoEast1RegionID, err) | |
| } | |
| tagClients = append(tagClients, tagClient) | |
| } | |
| // A custom "tagging" service endpoint has no region dimension | |
| // (ServiceEndpoint is just {Name, URL}), so it cannot be retargeted at | |
| // us-iso-east-1: the client would sign for us-iso-east-1 while talking to | |
| // the install-region URL, and every GetResources call would fail. | |
| _, hasCustomTagging := awssession.NewServiceEndpointResolver( | |
| awssession.EndpointOptions{Endpoints: o.endpoints}, | |
| ).GetCustomEndpoint(resourcegroupstaggingapi.ServiceID) | |
| switch { | |
| case o.Region == awstypes.UsIsoEast1RegionID: | |
| // Already covered by the base tagging client. | |
| case hasCustomTagging: | |
| o.Logger.Warnf("a custom tagging service endpoint is configured; skipping the additional %s tagging client, global resources in %s may not be discovered", | |
| awstypes.UsIsoEast1RegionID, awstypes.UsIsoEast1RegionID) | |
| default: | |
| tagClient, err := awssession.NewResourceGroupsTaggingAPIClient(ctx, awssession.EndpointOptions{ | |
| Region: awstypes.UsIsoEast1RegionID, | |
| Endpoints: o.endpoints, | |
| }, "", resourcegroupstaggingapi.WithAPIOptions(awsmiddleware.AddUserAgentKeyValue(awssession.OpenShiftInstallerDestroyerUserAgent, version.Raw))) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to create resource tagging client for %s: %w", awstypes.UsIsoEast1RegionID, err) | |
| } | |
| tagClients = append(tagClients, tagClient) | |
| } |
There was a problem hiding this comment.
@tthvo I see you commented we don't support iso region yet, so do you think we can ignore this issue with the destroy path for now or better to guard before we need?
I don't have the full context so that's why I am asking, my suggestion was to be more defensive in the branching.
There was a problem hiding this comment.
@rochacbruno Is this Custom Tagging problem specific to the ISO regions, or would it apply to any region?
There was a problem hiding this comment.
Not ISO-specific. The trigger is generic: a custom tagging endpoint plus any branch that builds a secondary tag client pinned to a different region than the install region. validateServiceEndpoints (pkg/types/aws/validation/platform.go:160-176) only checks for duplicate names and a parseable URL, so any region can set serviceEndpoints.
Every branch in this switch passes o.endpoints while pinning a different region, so on current main this already applies to:
| Branch | Install region | Secondary client | Affected today |
|---|---|---|---|
default: |
any commercial != us-east-1 |
us-east-1 |
yes |
| China | cn-north-1 |
cn-northwest-1 |
yes |
| GovCloud | us-gov-east-1 |
us-gov-west-1 |
yes |
| ISO (new) | us-iso-west-1 |
us-iso-east-1 |
once ISO ships |
Plus the HostedZoneRole assumed-role client (L293-300), which has the same shape whenever tagRegion != o.Region.
So this PR isn't introducing the problem, it's inheriting it, and since ISO isn't supported yet the new instance has no field impact. I don't think it should block this PR. I'd suggest a separate bug covering the whole switch, including the non-terminating poll loop (a permanently-failing tag client is retained at L491-493 and keeps loopError set, so the condition at L400 never satisfies), and folding in the us-isob-west-1 gap at L334.
There was a problem hiding this comment.
I filled a separate ticket to track the problem https://redhat.atlassian.net/browse/OCPBUGS-126492
There was a problem hiding this comment.
@rochacbruno @rh-cbent Thanks guys! AFAICT, this issue/gap exists for a long time, even before AWS SDK v2 migration. We can quickly check with CLI:
$ aws resourcegroupstaggingapi get-resources \
--endpoint-url https://tagging.us-west-2.amazonaws.com \
--region us-east-1
An error occurred (InvalidSignatureException) when calling the GetResources operation: Credential should be scoped to a valid regionWe haven't yet had any issues because the use cases for custom endpoints are:
- To support new AWS region(s) that the current SDK version cannot yet recognize. The region
us-iso-west-1also has been around for a while now. That said, SDK version bump is preferred :D - To support restricted network (e.g. disconnected) where the API calls need to be re-routed elsewhere, but anyone can just configure proxy for cluster and install host, which achieves the same goal.
If you have more, please share with me XD 👀 📓
Note: the 2nd tag client is to discover route53 hostedzone so if the destroy hangs, it's fine to work around by stopping it and manually deletes the remaining route53 resources.
Another option may be to keep building the client but drop Endpoints so the SDK resolves its own us-iso-east-1 endpoint and that preserves the PR's intent
Right, 👍 I think this is the cleanest and reasonable fix. For each partition, we always begin with the inaugural "global" region (e.g. us-east-1); thus, by the time we support another region, the "default" region's endpoint is already correctly resolvable by the SDK.
but assumes the default ISO hostname is reachable in the customer's network
Yes, we have made clear in [1] and [2] that accessing to tagging.us-east-1.amazonaws.com is required. This implicitly applies to other partitions (and their "default" region) too (maybe something for docs team to improve clarity).
There was a problem hiding this comment.
Hmm, thinking more, this issue also extends to custom Route53 and STS endpoints, where the signing region must be the "global" one. We can override the signing region there based on partition.
Sorry for the long message XD My understanding is that custom endpoint setting is not popularly used, so they went unnoticed. We can fix in a follow-up :D
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rochacbruno, tthvo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
As discussed, our CI deprovisioning step actually doesn't run the destroy in the emulated ISO regions (see here). So, I'm going to manually verify this change 👀 |
|
/test unit |
|
@rh-cbent: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
4 participants
constants.
Client creation.
Summary by CodeRabbit