HOSTEDCP-1561: Move HCP Product CLI to STS #4027

Patryk-Stefanski · 2024-05-13T17:17:54Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci · 2024-05-13T17:18:06Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2024-05-13T17:18:28Z

@Patryk-Stefanski: This pull request references HOSTEDCP-1561 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

netlify · 2024-05-13T17:19:04Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`01d10a4`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/664486bb36a72b00087493cf
😎 Deploy Preview	https://deploy-preview-4027--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

cmd/cluster/aws/create.go

cmd/cluster/aws/destroy.go

cmd/consolelogs/aws/getlogs.go

stevekuznetsov · 2024-05-13T22:40:01Z

cmd/infra/aws/create_cli_role.go

+)
+
+const (
+	cliRolePolicy = `{


Didn't @muraee just add this?

yes he did, This PR wasn't meant to be reviewed yet (but I always welcome an early review :) ) and I was using his work for testing. So this comment and the following comments should have been left on his pr. In this case Im more than happy to make any changes that @muraee approves.

OK. His PR already went in, let's get these following comments addressed in this PR then.

stevekuznetsov · 2024-05-13T22:40:58Z

cmd/infra/aws/create_cli_role.go

+	return nil
+}
+
+func assumeRoleTrustPolicy(ctx context.Context, client *sts.STS) (string, error) {


This looks like it has a lot in common with iam.go - let's refactor to share where we can, please.

same as #4027 (comment)

stevekuznetsov · 2024-05-13T22:41:26Z

cmd/infra/aws/create_cli_role.go

+	return assumeRolePolicy, nil
+}
+
+func (o *CreateCLIRoleOptions) ParseAdditionalTags() ([]*iam.Tag, error) {


This looks like a copy-pasta from iam.go - please DRY it out

same as #4027 (comment)

stevekuznetsov · 2024-05-13T22:44:05Z

cmd/infra/aws/create_iam.go

@@ -155,7 +169,17 @@ func (o *CreateIAMOptions) CreateIAM(ctx context.Context, client crclient.Client
 		return nil, err
 	}

-	awsSession := awsutil.NewSession("cli-create-iam", o.AWSCredentialsFile, o.AWSKey, o.AWSSecretKey, o.Region)
+	var awsSession *session.Session


The sheer number of places where you need to add these new flags and validation is too high to be tenable. Let's break out some

type AWSCredentialOptions struct { RoleARN string STSCreds string // whatever else } func DefaultOptions() *AWSCredentialOptions { return &AWSCredentialOptions{/* place any meaningful defaults here */} } func BindOptions(opts *AWSCredentialOptions, flags *pflag.FlagSet) { flags.StringVar(&opts.RoleArn, "role-arn", opts.RoleArn, "The ARN of the role to assume when creating the iam.") flags.StringVar(&opts.StsCredentialsFile, "sts-creds", opts.StsCredentialsFile, "Path to STS credentials file to use when assuming the role.") } func (opts *AWSCredentialOptions) Validate() error { /* add validation here */ }

Then, embed this struct into the others where needed and call BindOptions() on it where flags are being added.

same as #4027 (comment)

@stevekuznetsov because of the --secret-creds flag we sometimes need the roleArn without the need of stsCreds. I have created a new util validation function for validating the required flags here cmd/util/credentials_validation.go

Let's find a factoring where we bind the flags once in the repo and validate them in one flow. If that means that the options need parameters on creation and/or validation, so be it. For all commands that want to authenticate to AWS, let's provide a package that lets them bind the flags they need and get an AWS session out the other end. A good litmus test IMHO will be that this PR adds only one line that binds sts-creds as a flag to a struct member.

@stevekuznetsov Just reading over the PR and I agree with you but since I'm leaving for PTO today and this feature is time sensitive I think this would be more of nice to have in this scenario. @muraee I don't know if you have the time to look into this approach or maybe this can be a separate Jira in our backlog?

I can't speak to timelines but I think trending in the direction I outline here will help us immensely with maintenance of the codebase.

@Patryk-Stefanski given that you'll be out, is it ok if we get someone else to take over the PR after today? I don't think @stevekuznetsov's are that big, but agree they would help with the maintenance of the code.

@csrwng I have added @muraee as contributor and I can add anyone else aswell just let me know.

stevekuznetsov · 2024-05-13T22:44:46Z

cmd/infra/aws/iam.go

+	TrustPolicy       string
+	PermissionsPolicy string
+
+	additionalIAMTags []*iam.Tag


why is this not exported?

same as #4027 (comment)

cmd/infra/aws/util/util.go

muraee · 2024-05-15T14:53:22Z

cmd/infra/aws/util/util.go

+	}
+
+	if credentialsFile != "" {
+		stsSessionOpts.SharedConfigFiles = append(stsSessionOpts.SharedConfigFiles, credentialsFile)


this is expecting an aws credentials file, I think it should expect the output of aws sts get-session-token --output json instead and then parse it and extract necessary fields.
Otherwise, the use will have to do 1 extra step to format the sts-creds file before passing it to the cli. If we do this, we can validate that the session_token exist and throw an error/warning that the user is not using temporary credentials.

csrwng

Just some naming nits, otherwise lgtm

csrwng · 2024-05-17T13:31:42Z

cmd/infra/aws/util/sts.go

+	supportawsutil "github.com/openshift/hypershift/support/awsutil"
+)
+
+func NewStsSession(agent, rolenArn, region string, assumeRoleCreds *credentials.Credentials) (*session.Session, error) {


nit: s/Sts/STS/

csrwng · 2024-05-17T13:36:05Z

cmd/infra/aws/util/sts.go

+		Credentials Credentials `json:"Credentials"`
+	}
+
+	rawStsCreds, err := os.ReadFile(credentialsFile)


nit: s/Sts/STS/

csrwng · 2024-05-17T13:36:20Z

cmd/infra/aws/util/util.go

+	AWSCredentialsFile string
+
+	RoleArn            string
+	StsCredentialsFile string


nit: s/Sts/STS/

muraee · 2024-05-17T14:04:40Z

/label tide/merge-method-squash

csrwng · 2024-05-17T14:07:55Z

cmd/cluster/aws/destroy.go

+		return nil
+	}
+
+	if opts.AWSPlatform.AWSCredentialsOpts.AWSCredentialsFile == "" {


Is there a reason we're doing this check again? Wouldn't Validate above catch it?

Validate above is executed when the CredentialsSecret is not set.

If CredentialsSecret is set we want to only check that role-arn is required, but only if AWSCredentialsFile is not set. i.e. AWSCredentialsFile has priority over credentialSecret (this is the current behavior, I can change it if it doesn't make sense)

In some follow-up, might be good to check with whoever is using this secret flag thing and see if they can stop, it's a huge hassle to support it and asking folks to pass in the options they need verbatim would be a lot easier to reason about.

csrwng · 2024-05-17T14:21:29Z

/approve

stevekuznetsov · 2024-05-17T15:02:20Z

cmd/cluster/core/create.go

@@ -167,7 +168,7 @@ type KubevirtPlatformCreateOptions struct {
 }

 type AWSPlatformOptions struct {
-	AWSCredentialsFile      string
+	AWSCredentialsOpts      awsutil.AWSCredentialsOptions


You do not need to change this now, but FWIW if you had embedded the awsutil.AWSCredentialsOptions it would have required a much smaller diff (and a lot less typing!)

stevekuznetsov

Looks good! Couple small comments

stevekuznetsov · 2024-05-17T15:03:26Z

cmd/cluster/aws/create.go

@@ -48,11 +46,13 @@ func NewCreateCommand(opts *core.CreateOptions) *cobra.Command {
 	cmd.Flags().StringVar(&opts.AWSPlatform.EndpointAccess, "endpoint-access", opts.AWSPlatform.EndpointAccess, "Access for control plane endpoints (Public, PublicAndPrivate, Private)")
 	cmd.Flags().StringVar(&opts.AWSPlatform.EtcdKMSKeyARN, "kms-key-arn", opts.AWSPlatform.EtcdKMSKeyARN, "The ARN of the KMS key to use for Etcd encryption. If not supplied, etcd encryption will default to using a generated AESCBC key.")
 	cmd.Flags().BoolVar(&opts.AWSPlatform.EnableProxy, "enable-proxy", opts.AWSPlatform.EnableProxy, "If a proxy should be set up, rather than allowing direct internet access from the nodes")
-	cmd.Flags().StringVar(&opts.CredentialSecretName, "secret-creds", opts.CredentialSecretName, "A Kubernetes secret with needed AWS platform credentials: aws-creds, pull-secret, and a base-domain value. The secret must exist in the supplied \"--namespace\". If a value is provided through the flag '--pull-secret', that value will override the pull-secret value in 'secret-creds'.")
+	cmd.Flags().StringVar(&opts.CredentialSecretName, "secret-creds", opts.CredentialSecretName, "A Kubernetes secret with needed AWS platform credentials: sts-creds, pull-secret, and a base-domain value. The secret must exist in the supplied \"--namespace\". If a value is provided through the flag '--pull-secret', that value will override the pull-secret value in 'secret-creds'.")


Changing what we expect in this secret is breaking for any existing users of the CLI that are using the flag and secret today - if we have such users, this is not safe to do.

this is used by ACM, we already communicated the new changes with them.

What is the path for this not to be breaking, then? They will have to update the secret to contain both values before this merges and is rolled out?

we need this to merge today on our side. I don't how/when they roll out new versions.

stevekuznetsov · 2024-05-17T15:06:19Z

cmd/cluster/aws/destroy.go

+		return nil
+	}
+
+	if opts.AWSPlatform.AWSCredentialsOpts.AWSCredentialsFile == "" {


In some follow-up, might be good to check with whoever is using this secret flag thing and see if they can stop, it's a huge hassle to support it and asking folks to pass in the options they need verbatim would be a lot easier to reason about.

stevekuznetsov · 2024-05-17T15:07:25Z

cmd/cluster/aws/destroy.go

@@ -138,20 +138,26 @@ func DestroyCluster(ctx context.Context, o *core.DestroyOptions) error {
 	return core.DestroyCluster(ctx, hostedCluster, o, destroyPlatformSpecifics)
 }

-// ValidateCredentialInfo validates if the credentials secret name is empty, the aws-creds is not empty; validates if
+// ValidateCredentialInfo validates if the credentials secret name is empty, the aws-creds or sts-creds mutually exclusive and are not empty; validates if
 // the credentials secret is not empty, that it can be retrieved.
 func ValidateCredentialInfo(opts *core.DestroyOptions) error {


Is this any different from the ValidateCreateCredentialInfo(opts *core.CreateOptions) error? If not, can it be written as ValidateAWSCredentialInfo(opts *core.AWSCredentialsOpts, secret string) error and shared?

Or, share using a larger struct that has one more field for the secret and closes over binding the flag, validating, etc.

ValidateCreateCredentialInfo also validates the pull-secret while this one doesn't, so I don't see how we can share the func easily.

What about

func ValidateCreateCredentialInfo(opts *core.CreateOptions) error { if err := ValidateAWSCredentialInfo(opts.AWSPlatform.AWSCredentialsOpts, opts.CredentialSecretName); err != nil { return err } if len(opts.CredentialSecretName) == 0 { if err := util.ValidateRequiredOption("pull-secret", opts.PullSecretFile); err != nil { return err } } return nil }

stevekuznetsov · 2024-05-17T15:09:51Z

cmd/infra/aws/util/util.go

+
+func (opts *AWSCredentialsOptions) BindFlags(flags *pflag.FlagSet) {
+	flags.StringVar(&opts.AWSCredentialsFile, "aws-creds", opts.AWSCredentialsFile, "Path to an AWS credentials file")
+	flags.StringVar(&opts.RoleArn, "role-arn", opts.RoleArn, "The ARN of the role to assume.")


Can you factor one of Bind* to call the other? We do not want the flag names etc to be different.

how would you do that? one marks the flags as required, the other one doesn't

I would personally not bother with the cobra.MarkFlagRequired business - you need to have the correct logic in Validate() anyway and with the programmatic re-use of the Options struct in e2e, so it's not really buying us anything.

cobra.MarkFlagRequired also helps prioritize those fields with auto completion. But not very important.
Addressed.

stevekuznetsov · 2024-05-17T15:11:52Z

cmd/infra/aws/util/util.go

+		return NewSTSSession(agent, opts.RoleArn, region, creds)
+	}
+
+	return nil, fmt.Errorf("either --aws-creds or --sts-creds or --secret-creds flag must be set")


nit: keep the flag-related comments in the Validate(). This kind of function (GetSession(), etc) is most useful when being re-used in code, for instance how we have the e2e today creating Options structs and using them as-is. In that kind of context, there are no flags and error messages about --aws-creds from the internals of the e2e framework (or whatever other programmatic consumer there is for this) are harder to follow. This error might read something like

return nil, errors.New("could not create AWS session, no credentials were given")

stevekuznetsov

/lgtm

openshift-ci · 2024-05-20T12:19:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csrwng, Patryk-Stefanski, stevekuznetsov

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:

~~OWNERS~~ [csrwng]

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 · 2024-05-20T13:07:48Z

/retest-required

Remaining retests: 0 against base HEAD c698d1d and 2 for PR HEAD d8dd04e in total

openshift-ci-robot · 2024-05-20T17:53:28Z

/retest-required

Remaining retests: 0 against base HEAD c5401ca and 1 for PR HEAD d8dd04e in total

openshift-ci-robot · 2024-05-21T03:05:45Z

/retest-required

Remaining retests: 0 against base HEAD f91af53 and 0 for PR HEAD d8dd04e in total

openshift-ci-robot · 2024-05-21T06:05:47Z

/hold

Revision d8dd04e was retested 3 times: holding

muraee · 2024-05-21T08:14:06Z

/unhold

muraee · 2024-05-21T08:22:57Z

/override ci/prow/e2e-kubevirt-aws-ovn

openshift-ci · 2024-05-21T08:23:58Z

@muraee: Overrode contexts on behalf of muraee: ci/prow/e2e-kubevirt-aws-ovn

In response to this:

/override ci/prow/e2e-kubevirt-aws-ovn

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-sigs/prow repository.

openshift-ci · 2024-05-21T13:00:06Z

@Patryk-Stefanski: The following test 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/e2e-azure	`d8dd04e`	link	false	`/test e2e-azure`

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-sigs/prow repository. I understand the commands that are listed here.

openshift-ci-robot · 2024-05-21T13:05:50Z

/retest-required

Remaining retests: 0 against base HEAD 5934604 and 2 for PR HEAD d8dd04e in total

openshift-ci-robot · 2024-05-21T16:55:03Z

/retest-required

Remaining retests: 0 against base HEAD f20ba0d and 1 for PR HEAD d8dd04e in total

celebdor · 2024-05-22T07:46:07Z

/cherry-pick release-4.16

openshift-cherrypick-robot · 2024-05-22T07:46:53Z

@celebdor: new pull request created: #4078

In response to this:

/cherry-pick release-4.16

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-sigs/prow repository.

* HOSTEDCP-1561: Move HCP CLI to STS mode * HOSTEDCP-1561: Addressing comments * HOSTEDCP-1561: enable --secret-creds flag with sts * HOSTEDCP-1561 extract sts cred specifics from sts-creds file * refactor * addressed review comments * more refactor --------- Co-authored-by: Mulham Raee <mraee@redhat.com>

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2024

openshift-ci bot added the do-not-merge/needs-area label May 13, 2024

Patryk-Stefanski changed the title ~~Hostedcp 1561~~ HOSTEDCP-1561: Move HCP Product CLI to STS May 13, 2024

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

openshift-ci bot added area/cli Indicates the PR includes changes for CLI and removed do-not-merge/needs-area labels May 13, 2024

stevekuznetsov reviewed May 13, 2024

View reviewed changes

Patryk-Stefanski added 2 commits May 15, 2024 10:55

HOSTEDCP-1561: Move HCP CLI to STS mode

fce5bac

HOSTEDCP-1561: Addressing comments

01d10a4

Patryk-Stefanski force-pushed the HOSTEDCP-1561 branch from dce034f to 01d10a4 Compare May 15, 2024 09:56

HOSTEDCP-1561: enable --secret-creds flag with sts

f54aca9

Patryk-Stefanski force-pushed the HOSTEDCP-1561 branch from bbad7cc to f54aca9 Compare May 15, 2024 14:24

Patryk-Stefanski marked this pull request as ready for review May 15, 2024 14:30

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2024

openshift-ci bot requested review from csrwng and hasueki May 15, 2024 14:32

muraee reviewed May 15, 2024

View reviewed changes

HOSTEDCP-1561 extract sts cred specifics from sts-creds file

d376060

openshift-ci bot added the area/testing Indicates the PR includes changes for e2e testing label May 17, 2024

muraee force-pushed the HOSTEDCP-1561 branch from 9f76128 to f757b3d Compare May 17, 2024 09:07

refactor

557f5ec

muraee force-pushed the HOSTEDCP-1561 branch from f757b3d to 557f5ec Compare May 17, 2024 13:40

csrwng reviewed May 17, 2024

View reviewed changes

addressed review comments

b463663

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 17, 2024

csrwng reviewed May 17, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2024

stevekuznetsov reviewed May 17, 2024

View reviewed changes

more refactor

d8dd04e

stevekuznetsov approved these changes May 20, 2024

View reviewed changes

openshift-ci bot assigned stevekuznetsov May 20, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2024

openshift-merge-bot bot merged commit 936f82b into openshift:main May 22, 2024
12 of 13 checks passed

openshift-cherrypick-robot mentioned this pull request May 22, 2024

[release-4.16] HOSTEDCP-1561: Move HCP Product CLI to STS #4078

Closed

Patryk-Stefanski mentioned this pull request May 27, 2024

[release-4.16] HOSTEDCP-1561: Move HCP Product CLI to STS #4092

Merged

HOSTEDCP-1561: Move HCP Product CLI to STS #4027

HOSTEDCP-1561: Move HCP Product CLI to STS #4027

Conversation

Patryk-Stefanski commented May 13, 2024

openshift-ci bot commented May 13, 2024

openshift-ci-robot commented May 13, 2024 • edited by openshift-ci bot

netlify bot commented May 13, 2024 • edited

✅ Deploy Preview for hypershift-docs ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muraee commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented May 17, 2024

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 20, 2024

openshift-ci-robot commented May 20, 2024

openshift-ci-robot commented May 20, 2024

openshift-ci-robot commented May 21, 2024

openshift-ci-robot commented May 21, 2024

muraee commented May 21, 2024

muraee commented May 21, 2024

openshift-ci bot commented May 21, 2024

openshift-ci bot commented May 21, 2024 • edited

openshift-ci-robot commented May 21, 2024

openshift-ci-robot commented May 21, 2024

celebdor commented May 22, 2024

openshift-cherrypick-robot commented May 22, 2024

openshift-ci-robot commented May 13, 2024 •

edited by openshift-ci bot

netlify bot commented May 13, 2024 •

edited

openshift-ci bot commented May 21, 2024 •

edited