✨ Add AWS IAM support #677

suvaanshkumar · 2024-10-27T20:51:07Z

Summary

The PR adds changes for AWS IAM based authentication support for Open Cluster Management (OCM) for Amazon EKS clusters. This change updates the Klusterletconfig object by adding the registration driver for awsirsa when the value of registration-auth flag is set as awsirsa from clusteradm join command.

Related issue(s)

Fixes #

codecov · 2024-10-28T01:48:57Z

Codecov Report

Attention: Patch coverage is 27.10843% with 121 lines in your changes missing coverage. Please review.

Project coverage is 63.37%. Comparing base (603b405) to head (2e2bd19).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/registration/register/aws_irsa/aws_irsa.go	26.66%	33 Missing ⚠️
...kg/registration/spoke/registration/registration.go	0.00%	28 Missing ⚠️
pkg/registration/spoke/spokeagent.go	42.22%	23 Missing and 3 partials ⚠️
pkg/registration/register/aws_irsa/aws.go	0.00%	16 Missing ⚠️
pkg/registration/helpers/helpers.go	0.00%	10 Missing ⚠️
pkg/registration/spoke/options.go	0.00%	7 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #677      +/-   ##
==========================================
- Coverage   63.72%   63.37%   -0.35%     
==========================================
  Files         182      185       +3     
  Lines       17611    17765     +154     
==========================================
+ Hits        11222    11258      +36     
- Misses       5460     5576     +116     
- Partials      929      931       +2

Flag	Coverage Δ
unit	`63.37% <27.10%> (-0.35%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mikeshng

Thanks for the PR!

It's currently failing at this check. Could you please make sure it works locally make update-gofmt?

Also, could you please update the PR summary with more details?

mikeshng · 2024-11-01T01:07:19Z

@dtclxy64 @suvaanshkumar could you please take a look at the merge conflict and rebase?

I think you could try deleting the following, based on the PR #658


		reSelectChecker: &reSelectChecker{shouldReSelect: false},
		bootstrapKubeconfigEventHandler: &bootstrapKubeconfigEventHandler{
			bootstrapKubeconfigSecretName: &opts.BootstrapKubeconfigSecret,
			cancel:                        cancel,
		},

mikeshng · 2024-11-04T16:26:10Z

@dtclxy64 thanks for the rebase. It's now failing at the DCO check and make update-gofmt check.
For DCO you could try:
git commit -s --amend then force push.

mikeshng · 2024-11-06T17:04:29Z

All the PR checks seem to be ok now. Great job @dtclxy64 and @suvaanshkumar

/assign @qiujian16

Signed-off-by: EmilyL <[email protected]>

qiujian16

looks good in general, just some minor comments

qiujian16 · 2024-11-08T07:29:57Z

pkg/registration/spoke/options.go

@@ -74,6 +76,10 @@ func (o *SpokeAgentOptions) AddFlags(fs *pflag.FlagSet) {
 			"the value of --cluster-signing-duration command-line flag of the kube-controller-manager will be used.")
 	fs.StringToStringVar(&o.ClusterAnnotations, "cluster-annotations", o.ClusterAnnotations, `the annotations with the reserve
 	 prefix "agent.open-cluster-management.io" set on ManagedCluster when creating only, other actors can update it afterwards.`)
+	fs.StringVar(&o.RegistrationAuth, "registration-auth", o.RegistrationAuth,


I think we should have a drivers Options and we just need to AddFlags here?

@qiujian16 We are currently using the registration-auth flag to determine the type of registerDriver to set up. See lines 199-202 in pkg/registration/spoke/spokeagent.go.

Are you suggesting that we should add another flag to capture the type of the registration driver as well?

the flag name is fine. I think you could put all these flags adding func to the register directory with a new Option struct. And the code here just to call driverOptions.AddFlags(fs), but this is not outstanding. We could do code restructure as a followup.

qiujian16 · 2024-11-08T07:30:39Z

pkg/registration/register/aws_irsa/aws_irsa.go

+)
+
+const (
+	// TLSKeyFile is the name of tls key file in kubeconfigSecret


nit you would not need these files. This are only for mtls

These constants are currently referenced by the BuildKubeConfigFromTemplate function in the aws-irso.go, the implementation is just a placeholder for now, so we'll remove these constants in one of the upcoming PRs once we have the function fully implemented for the AWSIRSADriver.

Add a //TODO here so we do not forget to remove these later.

qiujian16 · 2024-11-08T07:32:06Z

pkg/registration/helpers/testing/testinghelpers.go

@@ -421,6 +421,9 @@ type TestCert struct {
 	Key  []byte
 }

+type TestIrsaRequest struct {
+}


might add some TODO or comment here, I am not quite sure why this is added.

We are using the struct in aws_irsa_test.go on lines 38 and 64, which is setting up a test irsa driver for each test case.

I see, I think we could add a //TODO here so we will not forget the followup task later :)

qiujian16 · 2024-11-08T07:32:53Z

pkg/registration/helpers/helpers.go

@@ -158,3 +159,12 @@ func IsCSRSupported(nativeClient kubernetes.Interface) (bool, bool, error) {
 	}
 	return v1CSRSupported, v1beta1CSRSupported, nil
 }
+
+func IsEksArnWellFormed(eksArn string) bool {


add some comment for this func, it will be easier to understand what is the format for the arn.

qiujian16 · 2024-11-08T07:36:14Z

pkg/registration/spoke/spokeagent.go

@@ -54,7 +59,8 @@ type SpokeAgentConfig struct {

 	internalHubConfigValidFunc wait.ConditionWithContextFunc

-	hubKubeConfigChecker *hubKubeConfigHealthChecker
+	hubKubeConfigChecker            *hubKubeConfigHealthChecker
+	bootstrapKubeconfigEventHandler *bootstrapKubeconfigEventHandler


why this is added? I did not see where it is used

We got the change from the main branch which was introduced as part of PR #630, but we see in the latest main branch, it's no longer used. We removed it to match the main branch as requested.

…d from the NewSpokeAgentConfig function based on code review comments. Signed-off-by: Suvaansh <[email protected]>

…mple EKS ARN. Signed-off-by: Suvaansh <[email protected]> Signed-off-by: Emily Li <[email protected]>

qiujian16 · 2024-11-13T01:40:41Z

/approve
/lgtm

openshift-ci · 2024-11-13T01:40:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, suvaanshkumar

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~~ [qiujian16]

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

openshift-ci bot requested review from elgnay and xuezhaojun October 27, 2024 20:51

suvaanshkumar changed the title ~~✨ Add AWS IRSA support~~ ✨ Add AWS IAM support Oct 27, 2024

suvaanshkumar force-pushed the aws-irsa branch from 8bc341b to 21e55ff Compare October 27, 2024 21:15

mikeshng reviewed Oct 28, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase label Oct 29, 2024

dtclxy64 force-pushed the aws-irsa branch from 0b9aa37 to 65b6193 Compare November 4, 2024 15:49

openshift-merge-robot removed the needs-rebase label Nov 4, 2024

suvaanshkumar force-pushed the aws-irsa branch 7 times, most recently from 28a0f25 to 3838d5e Compare November 4, 2024 22:15

dtclxy64 force-pushed the aws-irsa branch from 3838d5e to 2402c22 Compare November 6, 2024 16:01

openshift-ci bot assigned qiujian16 Nov 6, 2024

Adding AWS IAM authentication support

c631baf

Signed-off-by: EmilyL <[email protected]>

dtclxy64 force-pushed the aws-irsa branch from 2402c22 to c631baf Compare November 6, 2024 17:11

qiujian16 reviewed Nov 8, 2024

View reviewed changes

dtclxy64 added 2 commits November 8, 2024 17:27

Remove the bootstrapKubeconfigEventHandler field that's no longer use…

eca7d04

…d from the NewSpokeAgentConfig function based on code review comments. Signed-off-by: Suvaansh <[email protected]>

Add a comment on what the IsEksArnWellFormed function does and an exa…

2e2bd19

…mple EKS ARN. Signed-off-by: Suvaansh <[email protected]> Signed-off-by: Emily Li <[email protected]>

suvaanshkumar force-pushed the aws-irsa branch from 375462f to 2e2bd19 Compare November 12, 2024 16:54

openshift-ci bot added the lgtm label Nov 13, 2024

openshift-ci bot added the approved label Nov 13, 2024

openshift-merge-bot bot merged commit 67b4f1a into open-cluster-management-io:main Nov 13, 2024
15 checks passed

jaswalkiranavtar mentioned this pull request Jan 6, 2025

Membership request open-cluster-management-io/community#194

Closed

5 tasks

dtclxy64 mentioned this pull request Jan 7, 2025

Membership request open-cluster-management-io/community#195

Closed

5 tasks

✨ Add AWS IAM support #677

✨ Add AWS IAM support #677

Uh oh!

Conversation

suvaanshkumar commented Oct 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related issue(s)

Uh oh!

codecov bot commented Oct 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mikeshng left a comment

Choose a reason for hiding this comment

Uh oh!

mikeshng commented Nov 1, 2024

Uh oh!

mikeshng commented Nov 4, 2024

Uh oh!

mikeshng commented Nov 6, 2024

Uh oh!

qiujian16 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiujian16 Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiujian16 commented Nov 13, 2024

Uh oh!

openshift-ci bot commented Nov 13, 2024

Uh oh!

Uh oh!

Uh oh!

suvaanshkumar commented Oct 27, 2024 •

edited

Loading

codecov bot commented Oct 28, 2024 •

edited

Loading

qiujian16 Nov 11, 2024 •

edited

Loading