CORS-2645: AWS Cross-Account Private Hosted Zone: Add Further Validations by patrickdillon · Pull Request #7253 · openshift/installer

patrickdillon · 2023-06-16T13:52:51Z

Adds/refines validations for the cross-account private hosted zone use case:

do not allow mint credential mode
do not require unnecessary permissions when using an existing hosted zone
require sts:AssumeRole permissions

Also includes some refactoring that did not make it into the previous PR.

ccing previous reviewers:
/cc @barbacbd @r4f4 @mtulio @sadasu

Adds validation to require that either manual or passthrough credentials mode is used when specifying hosted zone role. In order to perform the AssumeRole operation on the provided role, a policy must be in place to establish a trust reltionship between the role and the IAM credential (for the installer and ingress operator). Because mint mode will create new credentials with non-deterministic unique identifiers in the cluster, it is impossible to generate the policy in advance.

When users bring their own hosted zone, the installer no longer requires permissions to create or destroy zones.

openshift-ci-robot · 2023-06-16T13:53:44Z

@patrickdillon: This pull request references CORS-2645 which is a valid jira issue.

Details

In response to this:

Adds/refines validations for the cross-account private hosted zone use case:

do not allow mint credential mode

do not require unnecessary permissions when using an existing hosted zone

require sts:AssumeRole permissions

Also includes some refactoring that did not make it into the previous PR.

ccing previous reviewers:
/cc @barbacbd @r4f4 @mtulio @sadasu

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.

patrickdillon · 2023-06-16T13:59:17Z

@yunjiang29 PTAL

Small refactoring of destroy code for increasing readibility for cross-account private hosted zones.

Improves wording of hostedZoneRole comment/godoc.

r4f4

/lgtm

yunjiang29 · 2023-06-20T09:43:16Z

@patrickdillon while doing AssumeRole validation tests, is the below message expected? It looks like it's a real "No Permission" error instead of a "current credentials insufficient for performing cluster installation" [2]

time="2023-06-20T17:14:00+08:00" level=info msg="Credentials loaded from the \"accountbnoassume\" profile in file \"/home/cloud-user/.aws/credentials\""
time="2023-06-20T17:14:01+08:00" level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to generate asset \"Platform Provisioning Check\": aws.hostedZone: Invalid value: \"Z034258827351BTRAIDAV\": unable to retrieve hosted zone: could not get hosted zone: Z034258827351BTRAIDAV: AccessDenied: User: arn:aws:iam::301721915996:user/yunjiang-deny-AssumeRole is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::641733028092:role/yunjiang-noassu-rol1\n\tstatus code: 403, request id: 6e9d02bc-8ddb-4c5d-8f41-4e3d5d2790ac"

[2] compare to missing ec2:CreateVpc

WARNING Action not allowed with tested creds          action=ec2:CreateVpc
WARNING Tested creds not able to perform all requested actions
FATAL failed to fetch Cluster: failed to fetch dependency of "Cluster": failed to generate asset "Platform Permissions Check": validate AWS credentials: current credentials insufficient for performing cluster installation

barbacbd · 2023-06-20T11:33:59Z

/test okd-images

patrickdillon · 2023-06-20T17:07:14Z

@patrickdillon while doing AssumeRole validation tests, is the below message expected? It looks like it's a real "No Permission" error instead of a "current credentials insufficient for performing cluster installation" [2]

I think this should now be fixed in bfd54274464407ca988cde3624d88a4e2d54043a. I would expect a Tested creds not able to perform all requested actions error/warning if the INSTALLER creds do not have the right permissions (in this case AssumeRole).

But we are not testing the provided role for permissions, so if the role is missing route53 or tagging permissions, we will get an actual error and that is expected.

I hope that makes sense! LMK if it is unclear.

r4f4 · 2023-06-20T22:25:59Z

I don't think the resource_dep.svg was generated correctly. Here's the content


 * Waiting in queue... 
The following packages have to be installed:
 graphviz-5.0.0-4.fc37.x86_64	Graph Visualization Tools
 gts-0.7.6-42.20121130.fc37.x86_64	GNU Triangulated Surface Library
 lasi-1.1.3-9.fc37.x86_64	C++ library for creating Postscript documents
 mkfontscale-1.2.2-2.fc37.x86_64	Tool to generate legacy X11 font system index files
 netpbm-11.02.00-1.fc37.x86_64	A library for handling different graphics file formats
 xorg-x11-fonts-ISO8859-1-100dpi-7.5-34.fc37.noarch	A set of 100dpi ISO-8859-1 fonts for X

 * Waiting in queue... 
 * Waiting for authentication... 
 * Waiting in queue... 
 * Downloading packages... 
 * Requesting data... 
 * Testing changes... 
 * Installing packages...

yunjiang29 · 2023-06-21T05:22:05Z

I think this should now be fixed in bfd5427.

@patrickdillon it looks like the issue is still there:

time="2023-06-21T12:23:55+08:00" level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to generate asset \"Platform Provisioning Check\": aws.hostedZone: Invalid value: \"Z0134236393RTBLQK2N75\": unable to retrieve hosted zone: could not get hosted zone: Z0134236393RTBLQK2N75: AccessDenied: User: arn:aws:iam::301721915996:user/yunjiang-deny-AssumeRole is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::641733028092:role/yunjiang-noassu5-rol1\n\tstatus code: 403, request id: bf64206b-bebd-4dcf-a79b-d1de6976d4a9"

patrickdillon · 2023-06-21T17:07:15Z

I don't think the resource_dep.svg was generated correctly. Here's the content


 * Waiting in queue... 
The following packages have to be installed:
 graphviz-5.0.0-4.fc37.x86_64	Graph Visualization Tools
 gts-0.7.6-42.20121130.fc37.x86_64	GNU Triangulated Surface Library
 lasi-1.1.3-9.fc37.x86_64	C++ library for creating Postscript documents
 mkfontscale-1.2.2-2.fc37.x86_64	Tool to generate legacy X11 font system index files
 netpbm-11.02.00-1.fc37.x86_64	A library for handling different graphics file formats
 xorg-x11-fonts-ISO8859-1-100dpi-7.5-34.fc37.noarch	A set of 100dpi ISO-8859-1 fonts for X

 * Waiting in queue... 
 * Waiting for authentication... 
 * Waiting in queue... 
 * Downloading packages... 
 * Requesting data... 
 * Testing changes... 
 * Installing packages...

🤦

In any case, I have removed the commit changing the resource dependency so this doesn't need to be updated anymore.

patrickdillon · 2023-06-21T17:12:02Z

I think this should now be fixed in bfd5427.

@patrickdillon it looks like the issue is still there:

time="2023-06-21T12:23:55+08:00" level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to generate asset \"Platform Provisioning Check\": aws.hostedZone: Invalid value: \"Z0134236393RTBLQK2N75\": unable to retrieve hosted zone: could not get hosted zone: Z0134236393RTBLQK2N75: AccessDenied: User: arn:aws:iam::301721915996:user/yunjiang-deny-AssumeRole is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::641733028092:role/yunjiang-noassu5-rol1\n\tstatus code: 403, request id: bf64206b-bebd-4dcf-a79b-d1de6976d4a9"

On further examination, it appears that there are no permissions that need to be checked in Account B to perform this assume role option: all of the permissions reside within Account A. We could potentially check the permissions for the role in Account A, but that would require expanding the permissions needed by the role in Account A (such as the IAM simulate policy role). Considering that in this case a user-friendly permissions error will appear before any resources have been created (or when attempting to destroy DNS records), it does not seem like a good idea to pursue further permissions checking. Therefore I have removed the check for sts:AssumeRole (because it does nothing), and we should expect errors like the one Yunfei posted #7253 (comment) in the case of an incorrect configuration.

openshift-ci · 2023-06-21T20:44:30Z

@patrickdillon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`7b02db3`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn-single-node	`7b02db3`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/okd-e2e-aws-ovn-upgrade	`7b02db3`	link	false	`/test okd-e2e-aws-ovn-upgrade`
ci/prow/okd-e2e-aws-ovn	`7b02db3`	link	false	`/test okd-e2e-aws-ovn`

Full PR test history. Your PR dashboard.

Details

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.

r4f4

/approve

openshift-ci · 2023-06-21T20:49:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: r4f4

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [r4f4]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

yunjiang29 · 2023-06-26T08:28:46Z

I think this should now be fixed in bfd5427.

@patrickdillon it looks like the issue is still there:
time="2023-06-21T12:23:55+08:00" level=fatal msg="failed to fetch Terraform Variables: failed to fetch dependency of \"Terraform Variables\": failed to generate asset \"Platform Provisioning Check\": aws.hostedZone: Invalid value: \"Z0134236393RTBLQK2N75\": unable to retrieve hosted zone: could not get hosted zone: Z0134236393RTBLQK2N75: AccessDenied: User: arn:aws:iam::301721915996:user/yunjiang-deny-AssumeRole is not authorized to perform: sts:AssumeRole on resource: arn:aws:iam::641733028092:role/yunjiang-noassu5-rol1\n\tstatus code: 403, request id: bf64206b-bebd-4dcf-a79b-d1de6976d4a9"
On further examination, it appears that there are no permissions that need to be checked in Account B to perform this assume role option: all of the permissions reside within Account A. We could potentially check the permissions for the role in Account A, but that would require expanding the permissions needed by the role in Account A (such as the IAM simulate policy role). Considering that in this case a user-friendly permissions error will appear before any resources have been created (or when attempting to destroy DNS records), it does not seem like a good idea to pursue further permissions checking. Therefore I have removed the check for sts:AssumeRole (because it does nothing), and we should expect errors like the one Yunfei posted #7253 (comment) in the case of an incorrect configuration.

@patrickdillon thanks for the detailed explanation, the pre-merge test gets passed.

barbacbd

/lgtm

sadasu · 2023-06-28T20:21:20Z

/lgtm

patrickdillon added 2 commits June 15, 2023 14:22

aws: adjust permisisons for hostedZone

0b7b59f

When users bring their own hosted zone, the installer no longer requires permissions to create or destroy zones.

openshift-ci bot requested review from barbacbd, mtulio, r4f4 and sadasu June 16, 2023 13:52

patrickdillon changed the title ~~AWS Cross-Account Private Hosted Zone: Add Further Validations~~ CORS-2645: AWS Cross-Account Private Hosted Zone: Add Further Validations Jun 16, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 16, 2023

patrickdillon added 2 commits June 16, 2023 12:33

aws: refactor destroy code

8e4e8c8

Small refactoring of destroy code for increasing readibility for cross-account private hosted zones.

aws: clarify hostedZoneRole description

7b02db3

Improves wording of hostedZoneRole comment/godoc.

patrickdillon force-pushed the aws-shared-phz-2 branch from 0cb1c3b to 8a7779c Compare June 16, 2023 16:46

r4f4 reviewed Jun 16, 2023

View reviewed changes

openshift-ci bot assigned r4f4 Jun 16, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2023

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2023

patrickdillon force-pushed the aws-shared-phz-2 branch from 7d77d76 to 7b02db3 Compare June 21, 2023 17:05

r4f4 reviewed Jun 21, 2023

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 21, 2023

barbacbd reviewed Jun 28, 2023

View reviewed changes

openshift-ci bot assigned barbacbd Jun 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 28, 2023

openshift-ci bot assigned sadasu Jun 28, 2023

openshift-merge-robot merged commit 156e883 into openshift:master Jun 28, 2023

Conversation

patrickdillon commented Jun 16, 2023

Uh oh!

openshift-ci-robot commented Jun 16, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickdillon commented Jun 16, 2023

Uh oh!

r4f4 left a comment

Choose a reason for hiding this comment

Uh oh!

yunjiang29 commented Jun 20, 2023

Uh oh!

barbacbd commented Jun 20, 2023

Uh oh!

patrickdillon commented Jun 20, 2023

Uh oh!

r4f4 commented Jun 20, 2023

Uh oh!

yunjiang29 commented Jun 21, 2023

Uh oh!

patrickdillon commented Jun 21, 2023

Uh oh!

patrickdillon commented Jun 21, 2023

Uh oh!

openshift-ci bot commented Jun 21, 2023

Uh oh!

r4f4 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 21, 2023

Uh oh!

yunjiang29 commented Jun 26, 2023

Uh oh!

barbacbd left a comment

Choose a reason for hiding this comment

Uh oh!

sadasu commented Jun 28, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

openshift-ci-robot commented Jun 16, 2023 •

edited by openshift-ci bot

Loading