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.

Sign up for GitHub

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

Jump to bottom

use alternate IAM endpoints if specified #184

Merged
merged 1 commit into from Jun 9, 2020
Merged

use alternate IAM endpoints if specified #184

merged 1 commit into from Jun 9, 2020

Conversation

joelddiaz
Copy link
Contributor

Search through the cluster's Infrastructure object to see if an alternate IAM endpoint is specified, and use it when creating the AWS client.

@joelddiaz joelddiaz requested a review from dgoodwin April 16, 2020 20:25
@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 Apr 16, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 16, 2020
dgoodwin
dgoodwin reviewed Apr 17, 2020
View reviewed changes
Copy link
Contributor

@dgoodwin dgoodwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, is it feasible to unit test or excessively difficult?

@joelddiaz
Copy link
Contributor Author

Looks good, is it feasible to unit test or excessively difficult?

The best I could think of was to put some test cases to cover that we're processing the contents of the Infrastructure object correctly.

Once we receive the awsClient object, I don't see a way to inspect the client to see what the connection settings are for it.

@dgoodwin
Copy link
Contributor

Ok that's reasonable.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@joelddiaz
Copy link
Contributor Author

/test unit

@joelddiaz
Copy link
Contributor Author

/test images

@joelddiaz
Copy link
Contributor Author

/test e2e-aws

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 20, 2020
@joelddiaz joelddiaz changed the title WIP: use alternate IAM endpoints if specified use alternate IAM endpoints if specified Apr 20, 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 Apr 20, 2020
@joelddiaz
Copy link
Contributor Author

@dgoodwin this should be good to go now. I can squash and re-push once you've had a look.

@dgoodwin
Copy link
Contributor

Looks good, squash away.

@dgoodwin
Copy link
Contributor

/lgtm

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

tide Pending — Not mergeable. Needs bugzilla/valid-bug label.

@joelddiaz
Copy link
Contributor Author

did all the supporting work this to work finally make it in time for 4.5 (and we need an exception for this)? or can this wait?

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020
@dgoodwin
Copy link
Contributor

dgoodwin commented Jun 3, 2020

Need conflicts resolved here @joelddiaz

@joelddiaz
Copy link
Contributor Author

@sjenning fyi, i did mess with your oidc controller a bit to get this working. Not thrilled with having to add a controller-runtime.Client dependency on it, WDYT?

@joelddiaz
Copy link
Contributor Author

/test e2e-aws-upgrade

use alternate endpoint if specified in Infrastructure
72de52e 
Respect the cluster's serviceEndpoints specified in the Infrastructure object when building up a client to interact with AWS. If the IAM service endpoint is specified, use it.

Extend oidcdiscoveryendpoint controller to have a controller-runtime Client to simplify building AWS clients.
@joelddiaz
Copy link
Contributor Author

@dgoodwin FYI, rebased to fix merge conflicts. no functional changes.

@joelddiaz
Copy link
Contributor Author

/test e2e-aws

@dgoodwin
Copy link
Contributor

dgoodwin commented Jun 9, 2020

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgoodwin, joelddiaz

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-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 0aa1de5 into openshift:master Jun 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgoodwin dgoodwin dgoodwin left review comments

@abutcher abutcher Awaiting requested review from abutcher

Assignees

@dgoodwin dgoodwin

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joelddiaz @dgoodwin @openshift-ci-robot @openshift-bot @openshift-merge-robot