✨ Made changes for clusteradm accept to create role and policies on hub #819

alex0chan · 2025-01-31T16:17:48Z

Summary

This PR has the enhancement for clusteradm accept to create roles and policies on the hub

Related issue(s)

openshift-ci · 2025-01-31T16:18:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alex0chan
Once this PR has been reviewed and has the lgtm label, please assign qiujian16 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

jaswalkiranavtar · 2025-02-04T00:53:07Z

manifests/managed-cluster-policy/AccessPolicy.tmpl

+      "Effect": "Allow",
+      "Action": [
+        "eks:DescribeCluster",
+        "eks:ListClusters"


I think these permissions are not needed as we are copying cert authority data from bootstrap kubeconfig.

Shall we not have any permissions policy attached then?
Theoretically seems fine but have not tested this yet.

jaswalkiranavtar · 2025-02-04T00:57:01Z

pkg/common/helpers/helpers.go

If we have a similar method used by kluster-agent, can we reuse it?

We don't have this method anywhere else for the changes in this branch. If there will be more declarations coming up as a part of other PRs, We can con reuse there.

jaswalkiranavtar · 2025-02-04T01:15:24Z

pkg/registration/hub/managedcluster/controller.go

@@ -203,6 +210,21 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
 		}
 	}

+	// Only create new IAM roles when status is not present
+	if !meta.IsStatusConditionTrue(managedCluster.Status.Conditions, v1.ManagedClusterConditionHubAccepted) {


Would it make sense to remove this if condition and overwrite role, policy and access entry on every reconciliation? This would also ensure that the IAM resources are repaired if somebody accidentally or intentionally changes them manually.

Also suggesting to overwrite directly instead of making a get call first and them comparing, to save an api call.

This seems to be a valid comment and we can take this in an upcoming change.
We will have to add some logic to check role, trust policy and permissions policy and if they have been modified or not. It might be a little bit more complicated as compared to checking kubernetes Roles and Rolebindings created because we can simply be reapply the yaml on them where as over here we might have to check the resources using sdk.

Is the intention to only create permission once? It might not work since the condition patch might fail, and the is called again. Can we ensure that the createPermission can be called multiple times when the accept=true which is better to tolerate error.

mikeshng

Could you check and maybe address some of the workflow errors if they are valid? Thanks.

jaswalkiranavtar · 2025-02-04T23:19:52Z

Could you check and maybe address some of the workflow errors if they are valid? Thanks.

Yes I guess this is still WIP, we created PR to get early feedback.

jaswalkiranavtar · 2025-02-04T23:28:09Z

pkg/registration/hub/managedcluster/controller.go

@@ -203,6 +210,21 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
 		}
 	}

+	// Only create new IAM roles when status is not present
+	if !meta.IsStatusConditionTrue(managedCluster.Status.Conditions, v1.ManagedClusterConditionHubAccepted) {
+		clusterManager, err := c.operatorClient.OperatorV1().ClusterManagers().Get(context.TODO(), "cluster-manager", metav1.GetOptions{})


Instead of making this API server call and adding another permission to clusterrole, can we instead pass this info from clustermanager CR to registration-controller on hub through these command line options similar to this?

qiujian16 · 2025-02-06T07:22:44Z

I am wondering whether we need to import aws sdk here, or we just ask user to install aws cli as the prereq?

jaswalkiranavtar · 2025-02-10T04:53:40Z

I am wondering whether we need to import aws sdk here, or we just ask user to install aws cli as the prereq?

Yes we need to use aws sdk here to create IAM roles and policies when hub admin approves registration as part of registration controller on hub. Preferring sdk over cli as with cli, we would have to run shell commands from within go and handling errors would be complicated.

Signed-off-by: Gaurav Jaswal <[email protected]>

codecov · 2025-02-13T22:03:12Z

Codecov Report

Attention: Patch coverage is 56.68016% with 107 lines in your changes missing coverage. Please review.

Project coverage is 63.74%. Comparing base (b6c2a84) to head (78d4bcd).
Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/registration/register/aws_irsa/aws_irsa.go	65.62%	58 Missing and 8 partials ⚠️
pkg/registration/hub/managedcluster/controller.go	12.50%	12 Missing and 2 partials ⚠️
pkg/registration/register/common.go	0.00%	11 Missing ⚠️
pkg/registration/register/csr/csr.go	0.00%	6 Missing ⚠️
...agercontroller/clustermanager_runtime_reconcile.go	0.00%	4 Missing and 1 partial ⚠️
pkg/common/helpers/helpers.go	0.00%	3 Missing ⚠️
pkg/registration/hub/manager.go	75.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #819      +/-   ##
==========================================
- Coverage   63.82%   63.74%   -0.09%     
==========================================
  Files         193      194       +1     
  Lines       18667    18897     +230     
==========================================
+ Hits        11914    12045     +131     
- Misses       5772     5859      +87     
- Partials      981      993      +12

Flag	Coverage Δ
unit	`63.74% <56.68%> (-0.09%)`	⬇️

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.

Signed-off-by: Gaurav Jaswal <[email protected]>

qiujian16 · 2025-02-14T02:18:00Z

pkg/registration/hub/managedcluster/controller.go

+	// Only create new IAM roles when status is not present
+	if !meta.IsStatusConditionTrue(managedCluster.Status.Conditions, v1.ManagedClusterConditionHubAccepted) {
+		if err != nil {
+			log.Printf("Failed to get cluster manager %v", err)


we do not use log in controller. You have to use klog.FromContext to get the logger and set the correct log level. Actually, since it returns error, you might not need to output log.

qiujian16 · 2025-02-14T02:19:20Z

pkg/registration/register/interface.go

@@ -87,3 +87,9 @@ type Approver interface {
 	// deletes rolebindings for the agent, and then this is the additional operation a driver should process.
 	Cleanup(ctx context.Context, cluster *clusterv1.ManagedCluster) error
 }
+
+type RegisterDriverForHub interface {


Maybe only HubDriver, and also add some comment for this interface. And why it is not in the approver interface?

qiujian16 · 2025-02-14T02:22:26Z

pkg/registration/hub/managedcluster/controller.go

+
+		err = c.registerDriverForHub.CreatePermissions(ctx, managedCluster)
+		if err != nil {
+			log.Printf("Failed to create permissions %v", err)


ditto, do not use log here.

qiujian16 · 2025-02-14T02:23:52Z

pkg/registration/hub/manager.go

 	csrApprover, err := csr.NewCSRApprover(kubeClient, kubeInformers, m.ClusterAutoApprovalUsers, controllerContext.EventRecorder)
 	if err != nil {
 		return err
 	}

 	approver := register.NewAggregatedApprover(csrApprover)

+	awsIRSADriverForHub := awsirsa.NewAWSIRSADriverForHub(m.HubClusterArn)
+	csrDriverForHub := csr.NewCSRDriverForHub()
+	registerDriverForHub := register.NewAggregatedDriverForHub(csrDriverForHub, awsIRSADriverForHub)


shouldn't we start this based on the flags of which drivers should be enabled?

qiujian16 · 2025-02-14T02:29:22Z

pkg/registration/register/aws_irsa/aws_irsa.go

+// This function creates:
+// 1. IAM Roles and Policies in the hub cluster IAM
+// 2. Returns the hubclustername and the roleArn to be used for Access Entry creation
+func CreateIAMRoleAndPolicy(ctx context.Context, hubClusterArn string, managedCluster *v1.ManagedCluster, iamClient *iam.Client) (string, string, error) {


why it is public? do you need to call it outside of this package?

qiujian16 · 2025-02-14T02:30:40Z

pkg/registration/register/aws_irsa/aws_irsa.go

+		return nil
+	}
+	//Creating config for aws
+	cfg, err := config.LoadDefaultConfig(context.TODO())


Do we need to load the config everytime this func is called, or you can init the config when the driver is newed?

qiujian16 · 2025-02-14T08:26:32Z

manifests/managed-cluster-policy/TrustPolicy.tmpl

+            }
+        }
+    ]
+}


qiujian16 · 2025-02-14T08:29:35Z

pkg/registration/hub/managedcluster/controller.go

@@ -203,6 +210,21 @@ func (c *managedClusterController) sync(ctx context.Context, syncCtx factory.Syn
 		}
 	}

+	// Only create new IAM roles when status is not present
+	if !meta.IsStatusConditionTrue(managedCluster.Status.Conditions, v1.ManagedClusterConditionHubAccepted) {


Is the intention to only create permission once? It might not work since the condition patch might fail, and the is called again. Can we ensure that the createPermission can be called multiple times when the accept=true which is better to tolerate error.

openshift-ci bot requested review from suvaanshkumar and xuezhaojun January 31, 2025 16:18

jaswalkiranavtar reviewed Feb 4, 2025

View reviewed changes

mikeshng reviewed Feb 4, 2025

View reviewed changes

jaswalkiranavtar reviewed Feb 4, 2025

View reviewed changes

suvaanshkumar force-pushed the GWCP-65858-clusteradm-accept_bkp branch from 63faa0b to 76289f7 Compare February 5, 2025 03:38

suvaanshkumar force-pushed the GWCP-65858-clusteradm-accept_bkp branch 2 times, most recently from 9eda489 to 354054e Compare February 11, 2025 21:42

jaswalkiranavtar force-pushed the GWCP-65858-clusteradm-accept_bkp branch from a6c455a to 3ae3841 Compare February 13, 2025 21:46

Acceping AWS IRSA registraion request

6aa4d68

Signed-off-by: Gaurav Jaswal <[email protected]>

jaswalkiranavtar force-pushed the GWCP-65858-clusteradm-accept_bkp branch from 3ae3841 to 6aa4d68 Compare February 13, 2025 21:47

Running go mod tidy

78d4bcd

Signed-off-by: Gaurav Jaswal <[email protected]>

qiujian16 reviewed Feb 14, 2025

View reviewed changes

Waiting for AWS operation to complete before proceeding

eb24f6b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Made changes for clusteradm accept to create role and policies on hub #819

✨ Made changes for clusteradm accept to create role and policies on hub #819

alex0chan commented Jan 31, 2025 •

edited by mikeshng

Loading

openshift-ci bot commented Jan 31, 2025

jaswalkiranavtar Feb 4, 2025

suvaanshkumar Feb 12, 2025

jaswalkiranavtar Feb 4, 2025

suvaanshkumar Feb 12, 2025

jaswalkiranavtar Feb 4, 2025

suvaanshkumar Feb 12, 2025

qiujian16 Feb 14, 2025

mikeshng left a comment

jaswalkiranavtar commented Feb 4, 2025

jaswalkiranavtar Feb 4, 2025 •

edited

Loading

qiujian16 commented Feb 6, 2025

jaswalkiranavtar commented Feb 10, 2025

codecov bot commented Feb 13, 2025 •

edited

Loading

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

qiujian16 Feb 14, 2025

✨ Made changes for clusteradm accept to create role and policies on hub #819

Are you sure you want to change the base?

✨ Made changes for clusteradm accept to create role and policies on hub #819

Conversation

alex0chan commented Jan 31, 2025 • edited by mikeshng Loading

Summary

Related issue(s)

openshift-ci bot commented Jan 31, 2025

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

mikeshng left a comment

Choose a reason for hiding this comment

jaswalkiranavtar commented Feb 4, 2025

jaswalkiranavtar Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

qiujian16 commented Feb 6, 2025

jaswalkiranavtar commented Feb 10, 2025

codecov bot commented Feb 13, 2025 • edited Loading

Codecov Report

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

alex0chan commented Jan 31, 2025 •

edited by mikeshng

Loading

jaswalkiranavtar Feb 4, 2025 •

edited

Loading

codecov bot commented Feb 13, 2025 •

edited

Loading