Service account reconciliation in connect cluster #3187

ranatrk · 2023-08-08T11:50:33Z

What changed?

bumb k8s.io/apimachinery from v0.26.3 to v0.27.3
Add connector package to pkg/api/cluster
Add reconcileServiceAccount which
- Creates service account given a client
- Creates cluster role and cluster role binding for service account
- Creates a Service account Secret, which when created is populated with desired token(will be used in the kubeconfig used to access the leaf cluster)

Why was this change made?
Create the resources required (Service account, Cluster Role, Cluster Role Binding, Secret) to be able to access the intended cluster from the management cluster

How was this change implemented?
New package was created that includes the reconciliation process and checks

How did you validate the change?

Explain how a reviewer can verify the change themselves
- A simple driver given an existing kind cluster created that calls the reconciliation function
- Then using kubectl the generated resources can be verified to exist correctly.
- Service Account: test-service-account
  Cluster role: test-service-account-cluster-role
  Cluster role binding: test-service-account-cluster-role-binding
  Secret: test-service-account-token

cfg := config.GetConfigOrDie()
clientSet, err := kubernetes.NewForConfig(cfg)
if err != nil {
   panic(err)
}

token, err := connector.ReconcileServiceAccount(context.Background(), clientSet, "test-service-account")
log.Printf("Error: %s", err)
log.Printf("Token :%s", token)

Unit tests -- what is covered, what cannot be covered; are
there tests that fail without the change?

Release notes

Documentation Changes

Other follow ups

bigkevmcd

Good start, mostly tidyups and simplifications.

bigkevmcd · 2023-08-09T06:29:20Z

pkg/cluster/connector/driver.go

Let's remove this?

bigkevmcd · 2023-08-09T06:30:05Z

pkg/cluster/connector/config.go

+	"k8s.io/client-go/tools/clientcmd"
+)
+
+// ConnectCluster will ensure that the cluster referenced by newContext can be


We can remove this?

And let's add a Doc comment for ConfigForContext?

pkg/cluster/connector/config_test.go

bigkevmcd · 2023-08-09T06:30:55Z

pkg/cluster/connector/config_test.go

+		t.Fatal(err)
+	}
+
+	if restCfg.Host != "https://cluster2.example.com" {


We could also check the other fields in the created config?

bigkevmcd · 2023-08-09T06:37:27Z

pkg/cluster/connector/service_account.go

+// ReconcileServiceAccount accepts a client and the name for a service account.
+// A new Service account is created, if one with same name exists that will be used
+// A new cluster role and cluster role binding are created, if already existing those will be used
+func ReconcileServiceAccount(ctx context.Context, client kubernetes.Interface, serviceAccountName string) ([]byte, error) {


We should indicate what it returns ?

bigkevmcd · 2023-08-09T07:29:59Z

pkg/cluster/connector/service_account_test.go

+		return nil
+	}
+	for _, resource := range resources {
+		switch reflect.TypeOf(resource) {


You could use a Go type switch here?

This would simplify the code a fair bit?

bigkevmcd · 2023-08-09T07:30:09Z

pkg/cluster/connector/service_account_test.go

+// Add resources of different types to the client based on the type of the resource
+// Valid resources: v1.ServiceAccount, rbacv1.ClusterRole, rbacv1.ClusterRoleBinding, v1.Secret
+func addFakeResources(client kubernetes.Interface, resources []runtime.Object) error {
+	if resources == nil {


Probably don't need to do this?

bigkevmcd · 2023-08-09T07:31:18Z

pkg/cluster/connector/service_account_test.go

+		switch reflect.TypeOf(resource) {
+
+		case reflect.TypeOf(&v1.ServiceAccount{}):
+			_, err := client.CoreV1().ServiceAccounts(corev1.NamespaceDefault).Create(context.Background(), resource.(*v1.ServiceAccount), metav1.CreateOptions{})


If you pass in a *testing.T you could just fail the test here in the event of error?

bigkevmcd · 2023-08-09T07:31:50Z

pkg/cluster/connector/service_account_test.go

+
+		}
+
+	}


Passing in a *testing.T would have another benefit, you could fail if you get an unknown type :-)

bigkevmcd · 2023-08-09T07:32:57Z

pkg/cluster/connector/service_account_test.go

+	return nil
+}
+
+func addFakeServiceAccounts(client kubernetes.Interface, serviceAccounts []string) error {


See above and https://speakerdeck.com/mitchellh/advanced-testing-with-go?slide=28 is a great talk well worth watching https://www.youtube.com/watch?v=8hQG7QlcLBk

bigkevmcd

This looks good for the first part, there are some tiny tidyups, but otherwise it looks good.

bigkevmcd · 2023-08-09T16:20:52Z

pkg/cluster/connector/service_account.go

+	"k8s.io/client-go/kubernetes"
+)
+
+type ClusterConnectionOptions struct {


Let's add a go doc for this, it's exported?
https://github.com/golang/go/wiki/CodeReviewComments#doc-comments

bigkevmcd · 2023-08-09T16:25:01Z

pkg/cluster/connector/service_account_test.go

+				t.Errorf("error adding resources: %v", err)
+			}
+		default:
+			t.Error("invalid resource type")


how about t.Fatalf("invalid resource type %s", resource)

This can be fatal, because it's during the setup, so if the setup fails, the tests likely would anyway, for unrelated reasons.

bigkevmcd · 2023-08-09T16:25:53Z

pkg/cluster/connector/service_account.go

+// ReconcileServiceAccount accepts a client and the name for a service account.
+// A new Service account is created, if one with same name exists that will be used
+// A new cluster role and cluster role binding are created, if already existing those will be used
+// returns the token of the secret created for the service account


Nit pick here, I would move this to the top of the file, so that it's the first function you see, the rest are fairly unimportant, so it makes it easier to pick up.

But this is very much a personal preference.

Add test cases for existing service account, cluster role, and cluster role binding Add helper function in test to add resources to client based on the type of the resource Add logs when existing resources found in reconcile service account instead of creating new ones Move functionality from service account test to service account

…textTimeout

Add test case for failing ConfigForContext Delete driver file Add ClusterConnectionOptions struct Simplify ReconcileServiceAccount to extract resource creation to different helper functions Cleanup service account tests Move artificially populating secret token to helper function in test

ranatrk added the enhancement New feature or request label Aug 8, 2023

ranatrk requested a review from bigkevmcd August 8, 2023 11:50

ranatrk force-pushed the cluster-connection-config branch from 83b8224 to 3a5afc4 Compare August 8, 2023 13:02

bigkevmcd requested changes Aug 9, 2023

View reviewed changes

ranatrk force-pushed the cluster-connection-config branch 2 times, most recently from 2a6a446 to 9bcf9ad Compare August 9, 2023 15:21

ranatrk requested a review from bigkevmcd August 9, 2023 15:42

bigkevmcd approved these changes Aug 9, 2023

View reviewed changes

bigkevmcd and others added 8 commits August 9, 2023 19:51

Work in progress.

1b3e090

WIP waiting for token no timeout

539c2a6

Tests passing.

a157523

Update go.sum after rebasing main

a36d795

Fix and remove deprecated PollImmediate function and use PollUntilCon…

3f64da1

…textTimeout

Cleanup reconcile service account helper functions

3ca0965

ranatrk force-pushed the cluster-connection-config branch from 74d0b67 to eb91c05 Compare August 9, 2023 16:51

Tidy up service account

7ba19b9

ranatrk force-pushed the cluster-connection-config branch from eb91c05 to 7ba19b9 Compare August 9, 2023 17:01

ranatrk merged commit fbf1ee9 into main Aug 9, 2023

ranatrk deleted the cluster-connection-config branch August 9, 2023 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service account reconciliation in connect cluster #3187

Service account reconciliation in connect cluster #3187

ranatrk commented Aug 8, 2023 •

edited

Loading

bigkevmcd left a comment

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd left a comment

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

bigkevmcd Aug 9, 2023

Service account reconciliation in connect cluster #3187

Service account reconciliation in connect cluster #3187

Conversation

ranatrk commented Aug 8, 2023 • edited Loading

bigkevmcd 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

bigkevmcd 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

ranatrk commented Aug 8, 2023 •

edited

Loading