Skip to content
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

aws: update the bootstrap ignition fetching to use custom region endpoints #2854

Conversation

jhixson74
Copy link
Member

Update the S3 bucket that stores the ignition config to use a presigned URL.
This allows the S3 bucket to be accesseed via HTTP(s) similar to Azure and GCP
thus allowing the installer to pick the correct endpoint based on region/user
specification.

https://issues.redhat.com/browse/CORS-1322

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2019
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from a7c5da9 to 82f112f Compare December 20, 2019 01:24
@jhixson74 jhixson74 changed the title [WIP] aws: update the bootstrap ignition fetching to use custom region endpoints aws: update the bootstrap ignition fetching to use custom region endpoints Dec 20, 2019
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2019
@abhinavdahiya
Copy link
Contributor

With move to presigned URL, do we have to change anything wrt to permissions?

  • should the user credentials that's being used to create presigned URL have certain things?
  • did the bootstrap host have special permissions assigned to it using instance profiles that we no longer needed?

also is there improvement in the UPI flow that we can make to improve that for users?

data/data/aws/bootstrap/main.tf Outdated Show resolved Hide resolved
pkg/asset/installconfig/aws/metadata.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 30, 2020
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from 78ced0b to 260318c Compare January 30, 2020 03:55
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 30, 2020
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from 260318c to 6c66604 Compare January 30, 2020 03:59
@jhixson74
Copy link
Member Author

With move to presigned URL, do we have to change anything wrt to permissions?

No permissions should need changing. Using a presigned URL will use the same permissions of the role that created it.

* should the user credentials that's being used to create presigned URL have certain things?

It should have the same access as it currently does. If a presigned URL were to be manually configured for elsewhere, additional permissions might need to be configured.

* did the bootstrap host have special permissions assigned to it using instance profiles that we no longer needed?

I don't believe so.

also is there improvement in the UPI flow that we can make to improve that for users?

It looks like we can change the UPI flow to use a presigned URL also.

@abhinavdahiya
Copy link
Contributor

With move to presigned URL, do we have to change anything wrt to permissions?

No permissions should need changing. Using a presigned URL will use the same permissions of the role that created it.

Previously the permissions of the bootstrap host were used to fetch the ignition file from S3, but now the credentials of the user invoking the installer will be used to fetch it.

so maybe permission on bootstrap like https://github.com/openshift/installer/pull/2854/files#diff-8af9be4a478f6109c2539e6abc13acaeR104-R110 are no longer required?? @jhixson74

I think we need to take another look at required changes in permissions with this move..

* should the user credentials that's being used to create presigned URL have certain things?

It should have the same access as it currently does. If a presigned URL were to be manually configured for elsewhere, additional permissions might need to be configured.

* did the bootstrap host have special permissions assigned to it using instance profiles that we no longer needed?

I don't believe so.

also is there improvement in the UPI flow that we can make to improve that for users?

It looks like we can change the UPI flow to use a presigned URL also.

@jhixson74 jhixson74 changed the title aws: update the bootstrap ignition fetching to use custom region endpoints [WIP] aws: update the bootstrap ignition fetching to use custom region endpoints Feb 17, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2020
@jhixson74
Copy link
Member Author

With move to presigned URL, do we have to change anything wrt to permissions?

No permissions should need changing. Using a presigned URL will use the same permissions of the role that created it.

Previously the permissions of the bootstrap host were used to fetch the ignition file from S3, but now the credentials of the user invoking the installer will be used to fetch it.

Can you explain to me how the credentials of the user invoking the installer are used here? My understanding here is everything remains the same as it was, but now uses a presigned URL. Since the bucket name and filename contents are known before it is created, the presigned URL is created using them. These values are passed into the terraform where they are created with the same roles and permissions as they previously were. What am I missing?

so maybe permission on bootstrap like https://github.com/openshift/installer/pull/2854/files#diff-8af9be4a478f6109c2539e6abc13acaeR104-R110 are no longer required?? @jhixson74

I think we need to take another look at required changes in permissions with this move..

* should the user credentials that's being used to create presigned URL have certain things?

It should have the same access as it currently does. If a presigned URL were to be manually configured for elsewhere, additional permissions might need to be configured.

* did the bootstrap host have special permissions assigned to it using instance profiles that we no longer needed?

I don't believe so.

also is there improvement in the UPI flow that we can make to improve that for users?

It looks like we can change the UPI flow to use a presigned URL also.

@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from 6c66604 to afe506b Compare April 15, 2020 02:40
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from afe506b to a505b01 Compare May 1, 2020 00:44
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch 3 times, most recently from f82e6dc to cc56d4b Compare May 6, 2020 01:33
@jhixson74 jhixson74 changed the title [WIP] aws: update the bootstrap ignition fetching to use custom region endpoints aws: update the bootstrap ignition fetching to use custom region endpoints May 6, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 6, 2020
@abhinavdahiya
Copy link
Contributor

/test e2e-aws

@abhinavdahiya
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2020
@abhinavdahiya
Copy link
Contributor

we can merge this when master opens for 4.6, until then this can stay open.

…oints

Update the S3 bucket that stores the ignition config to use a presigned URL.
This allows the S3 bucket to be accesseed via HTTP(s) similar to Azure and GCP
thus allowing the installer to pick the correct endpoint based on region/user
specification.

https://issues.redhat.com/browse/CORS-1322
@jhixson74 jhixson74 force-pushed the master_aws_bootstrap_ign_custom_endpoints branch from cc56d4b to dfd34eb Compare May 6, 2020 19:03
@abhinavdahiya
Copy link
Contributor

/lgtm

@abhinavdahiya
Copy link
Contributor

/skip

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 7, 2020
@abhinavdahiya
Copy link
Contributor

/retest

@abhinavdahiya abhinavdahiya added the version/4.6 Tracking changes that should end up in 4.6 release label May 8, 2020
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@abhinavdahiya
Copy link
Contributor

/retest

1 similar comment
@abhinavdahiya
Copy link
Contributor

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jun 2, 2020

@jhixson74: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-fips dfd34eb link /test e2e-aws-fips
ci/prow/e2e-ovirt dfd34eb link /test e2e-ovirt
ci/prow/e2e-openstack dfd34eb link /test e2e-openstack
ci/prow/e2e-libvirt dfd34eb link /test e2e-libvirt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@openshift-ci-robot
Copy link
Contributor

@abhinavdahiya: Overrode contexts on behalf of abhinavdahiya: ci/prow/e2e-aws

In response to this:

https://deck-ci.apps.ci.l2s4.p1.openshiftapps.com/view/gcs/origin-ci-test/pr-logs/pull/openshift_installer/2854/pull-ci-openshift-installer-master-e2e-aws/11955#1:build-log.txt%3A57359

the failing test is an e2e, known broken https://coreos.slack.com/archives/CNHC2DK2M/p1591025728087100

/override ci/prow/e2e-aws

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. version/4.6 Tracking changes that should end up in 4.6 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants