-
Notifications
You must be signed in to change notification settings - Fork 1.5k
CORS-4055: migrate Route53 API calls to AWS SDK v2 #10265
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@tthvo: This pull request references CORS-4055 which is a valid jira issue. 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. |
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-ipi-shared-vpc-phz-sts-fips-openldap-mini-perm-f7 |
|
@tthvo: trigger 1 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/4a5f5af0-fc03-11f0-867f-09e93b4dd687-0 |
|
/label tide/merge-method-squash |
|
/label platform/aws /cc @barbacbd @yunjiang29 |
2d3d52e to
dcdd3d7
Compare
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-ipi-shared-vpc-phz-sts-fips-openldap-mini-perm-f7 |
|
@tthvo: trigger 1 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/4a588710-fc59-11f0-99ed-1e5f7c9cd027-0 |
|
/test golint |
|
/test golint flakes? |
dcdd3d7 to
64a0786
Compare
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-ipi-shared-vpc-phz-sts-fips-openldap-mini-perm-f7 |
|
@tthvo: trigger 1 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/d6b3fa80-fc83-11f0-8ca4-51e4c7d2cbe0-0 |
| return awsconfig.ValidateForProvisioning(client, ic.Config, ic.AWS) | ||
|
|
||
| privHzClient := publicHzClient | ||
| if len(ic.Config.AWS.HostedZoneRole) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we not add assumed roles in the previous version for this call? Was that a mistake or is this new behavior? It looks like this is the only behavior that is or can be different than the previous version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this change do preserve the behaviour 👇
IIUC, the following is true:
- If
platform.aws.hostedZoneRoleis set,platform.aws.hostedZonemust also be set. See here. ValidateForProvisioningbehaves like below:- Validates private hosted zone and existing records in that private zone when
platform.aws.hostedZoneis specified. - Otherwise, validates base domain (i.e. public zone) and existing records in that public zone.
- Validates private hosted zone and existing records in that private zone when
With that, we can look at the previous ValidateForProvisioning code. The route53 client indeed assumed role (if any) for private zone calls only. The code was not so clear to express it, but it does.
We can double-confirm with payload testing for byo hosted zone: https://pr-payload-tests.ci.openshift.org/runs/ci/d6b3fa80-fc83-11f0-8ca4-51e4c7d2cbe0-0. The install completes successfully with the new change.
The commit is an incremental step to migrate AWS API calls to AWS SDK v2. This only focuses on Route53 clients in the pkg/asset and dependent pkg(s).
64a0786 to
f39a9fe
Compare
|
/payload-job periodic-ci-openshift-openshift-tests-private-release-4.22-amd64-nightly-aws-ipi-shared-vpc-phz-sts-fips-openldap-mini-perm-f7 |
|
@tthvo: trigger 1 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/96b32720-fd5b-11f0-9d65-0f8c8c4b3605-0 |
barbacbd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: barbacbd 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 |
The PR is an incremental step to migrate AWS API calls to AWS SDK v2. This only focuses on Route 53 clients in the pkg/asset and dependent pkg(s).
This PR contains quite a lot refactoring in order to follow the AWS SDK v2 client pattern.