-
Notifications
You must be signed in to change notification settings - Fork 54
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
Move all functionality for assume command into credentials/console #237
Move all functionality for assume command into credentials/console #237
Conversation
/hold I'm putting this up now for review, but we must hold off on merging until this conversation is resolved. In short - the OCM endpoint that determines a cluster's support jump role needs an update to how it makes it determination. |
74ff07b
to
b3fd866
Compare
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.
Nice! I really like that this doesn't only make it easier for SREs but also makes the code easier to understand and reason about! I have yet to test the changes locally, which is why i didn't approve the PR right now. The comments I left don't neccessarily need to be addressed as they're all pretty minor.
|
||
const ( | ||
// AwsCredentialsStringFormat format strings for printing AWS credentials as a string or as environment variables | ||
AwsCredentialsStringFormat = `Temporary Credentials: |
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.
Really Minor Nitpick: Maybe use \t
to indent the lines in the String. Just from looking at it in the code it's a bit confusing right now as it's the same indentation level as the surrounding code.
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.
This was all copy-paste from what used to be in the old console
and credentials
commands, so I wanted to keep that identical.
I also don't believe these should be indented. Right now this will all print to a terminal as such:
Temporary Credentials:
AccessKeyID: test-key
SecretAccessKey: test-secret
SessionToken: test-token
Region: test-region
Expires: test-expiration
I believe that's what we'd want.
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.
It seems we don't have a good way to make indentation look nice in raw string. golang/go#32590
Let's keep it as is, as long as the output is what we want.
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.
Non-blocking - I wonder if we could use go's text/templates here to better define these strings separately. I think "fixing" this should be a separate effort, though.
@@ -0,0 +1,33 @@ | |||
package credentials |
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.
Question: do you think it made sense to add some tests to this again?
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.
For functions like this, it feels weird to test because my tests would essentially just do exactly what the function is already doing. For example...
type fields struct {
AccessKeyID string
SecretAccessKey string
SessionToken string
Region string
Expiration string
}
tests := []struct {
name string
fields fields
want string
}{
{
name: "Contains no values",
fields: fields{},
want: fmt.Sprintf(AwsExportFormat, "", "", "", ""), <-- this is just what the function itself is doing
},
}
That said - I could still go either way. Any thoughts?
b3fd866
to
c047c23
Compare
/unhold |
We're currently processing your upload. This comment will be updated when the results are available. |
Hi @AlexVulaj , thanks for the PR to consolidate commands! I am new to the "new flow", and I wonder if there's any steps that I can try that locally? |
8d47317
to
5d278f6
Compare
Hey @feichashao , more details on the new flow can be found in the ADR, and this design doc. For steps to test locally, I'll add those to the description of this PR directly. |
5d278f6
to
f3ef69d
Compare
@AlexVulaj: all tests passed! Full PR test history. Your PR dashboard. 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. |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlexVulaj, samanthajayasinghe, wanghaoran1988 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 |
// GetAWSV2Config allows consumers to get an aws-sdk-go-v2 Config to programmatically access the AWS API | ||
func GetAWSV2Config(backplaneURL string, clusterID string) (aws.Config, error) { |
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.
This previously exported function should not have been removed, osdctl depends on it https://github.com/openshift/osdctl/blob/04c8575e83b30d0199ff0d1cfd5b81761f870556/pkg/osdCloud/aws.go#L189
This was originally added in openshift#66, but was mistakenly removed in openshift#237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>
This was originally added in openshift#66, but was mistakenly removed in openshift#237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>
This was originally added in openshift#66, but was mistakenly removed in openshift#237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>
This was originally added in openshift#66, but was mistakenly removed in openshift#237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>
This was originally added in openshift#66, but was mistakenly removed in openshift#237 Since we are now required to pass AWS calls through a proxy, this PR adds additional logic to ensure the aws-sdk-go-v2 client returned by this function enforces the use of the provided proxy_url in a user's backplane config file. Signed-off-by: Michael Shen <mshen@redhat.com>
What type of PR is this?
cleanup/feature
What this PR does / Why we need it?
takes the logic from the new assume/token commands (which are not yet in use anywhere) and consolidates them into the existing commands. this will prevent SREs from having to change anything about their existing workflows.
Which Jira/Github issue(s) does this PR fix?
OSD-19436
Pre-checks (if applicable)
Steps to test locally
As a pre-requisite, run either
backplane cloud console
orbackplane cloud credentials
against an existing, non-ROSA cluster and make sure it still works as it always has. For the new flow:Note - at this point, it is not uncommon to see a message on the screen saying "Waiting for IAM policy changes to resolve..."