Implement ManagedServiceAccount resources reconciliation and tests by yxun · Pull Request #55 · stolostron/multicluster-mesh-addon

yxun · 2026-04-29T16:18:49Z

This PR adds implementations of ManagedServiceAccount reconciliation and tests in the controller.

It implements creation and cleanup of the ManagedServiceAccount related resources for each managed cluster in the ClusterSet.

It builds istio remote Secret resources and creates ManifestWork resources for distributing those Secrets so that istiod control plane can discover remote endpoints from remote cluster API server(s)

References:

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci · 2026-04-29T16:18:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: yxun

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [yxun]

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

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

mkolesnik

I see this only handles creating MSA but not updating or deleting it.
Is this planned to be handled later, or will you be updating this PR?

mkolesnik · 2026-05-07T12:44:24Z

 	"sigs.k8s.io/controller-runtime/pkg/reconcile"

 	meshv1alpha1 "github.com/stolostron/multicluster-mesh-addon/pkg/apis/mesh/v1alpha1"
+	msav1alpha1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1alpha1"


We should use v1beta1.

I know you were worried about the rotation.enabled field being deprecated, but the rotation.validity is still exposed AFAIK so there's no reason to use the older API.

I have updated this with the v1beta1 version.

mkolesnik · 2026-05-07T12:52:50Z

+// CreateManagedServiceAccount creates a ManagedServiceAccount resource for a cluster in the ClusterSet
+func (r *Reconciler) CreateManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh *meshv1alpha1.MultiClusterMesh) error {
+	klog.Infof("Creating a ManagedServiceAccount for a managed cluster: %s", cluster.Name)
+	validity, err := time.ParseDuration(mesh.Spec.Security.Discovery.TokenValidity) // this is derived from the MultiClusterMesh spec.security.discovery.tokenValidity


Hmm right now in our API the TokenValidity field is defined as a sting that

// Supports hours (h), days (d), weeks (w), or months (m)

This parsing won't work well, but I wonder if we should just change the type of TokenValidity to be metav1.Duration?

I think so. Changing that TokenValidity type to be metav1.Duration will help aligning it with the following struct field.
https://pkg.go.dev/open-cluster-management.io/managed-serviceaccount@v0.10.0/apis/authentication/v1beta1#ManagedServiceAccountRotation

I will update this in the mesh/v1alpha1 types.

mkolesnik · 2026-05-07T12:53:58Z

+	msa := &msav1alpha1.ManagedServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s-%s", cluster.Name, mesh.Name), // Naming convention: <cluster-name>-<mesh-name>
+			Namespace: cluster.Name,


You should add labels to the resource so it can be properly deleted.
If you're adding deletion later, then I'm fine with not adding them right now.

mkolesnik · 2026-05-07T12:54:25Z

+		},
+		Spec: msav1alpha1.ManagedServiceAccountSpec{
+			Rotation: msav1alpha1.ManagedServiceAccountRotation{
+				Enabled:  false,


Why false?

// Enabled prescribes whether the ServiceAccount token will be rotated before it expires.
// Deprecated: All ServiceAccount tokens will be rotated before they expire regardless of this field.

I thought we don't want to rotate that ServiceAccount token. Anyway, I can set that to true or not set it. The default value is true.

mkolesnik · 2026-05-07T12:55:26Z


+	// Create ManagedServiceAccount resources for each managed cluster
+	for _, cluster := range clusters {
+		if err := r.CreateManagedServiceAccount(ctx, cluster, mesh); err != nil {


What if it already exists?
I reckon this'll fail?

Yes, the CreateManagedServiceAccount call would fail if the resource already exists. I have updated this part by checking an existing resource first.

mkolesnik · 2026-05-07T12:58:03Z

@@ -0,0 +1,314 @@
+---


This filed shouldn't be checked in

I have updated the Makefile target and removed this additional crd yaml file in this PR.

mkolesnik · 2026-05-07T13:01:28Z

 		CRDDirectoryPaths: []string{
 			filepath.Join("..", "..", "config", "crd"),                      // Custom MultiClusterMesh CRD
 			filepath.Join("..", "..", "test", "integration", "crds", "ocm"), // OCM CRDs
+			filepath.Join("..", "..", "test", "integration", "external-crds", "ocm"), // OCM ManagedServiceAccount CRD


I reckon we can just keep them in the same directory as other OCM test crds, or at most in a subdirectory there

right, the Makefile update-test-crds target is pulling that crd yaml in the same ocm test crds directory now.

mkolesnik · 2026-05-07T14:21:26Z

+				Version: "v1alpha1",
+			})
+
+			Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster1))).To(Succeed())


You should add test that validity gets passed if set by user

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun · 2026-05-18T20:43:41Z

I see this only handles creating MSA but not updating or deleting it. Is this planned to be handled later, or will you be updating this PR?

I have updated the part of the controller code. If the cluster has an existing ManagedServiceAccount resource with the naming convention , it will skip creating or updating it.

I can add a TODO and will implement updating existing resources in a follow up PR.

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

mkolesnik · 2026-05-20T07:22:38Z

 	// Supports hours (h), days (d), weeks (w), or months (m)
+	// It is a wrapper around time.Duration which supports correct marshaling to YAML and JSON
 	// +optional
 	// +kubebuilder:default="1m"


Shouldn't default be updated?

(1m == 1 month, but feel free to use a different default if you think its more sane)

mkolesnik · 2026-05-20T07:25:07Z

 	"sigs.k8s.io/controller-runtime/pkg/reconcile"

 	meshv1alpha1 "github.com/stolostron/multicluster-mesh-addon/pkg/apis/mesh/v1alpha1"
+	msav1v1beta1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1beta1"


Suggested change

msav1v1beta1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1beta1"

msav1beta1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1beta1"

Did you mean this?

mkolesnik · 2026-05-20T07:29:55Z

 	}

+	// Create ManagedServiceAccount resources for each managed cluster
+	msaList := &unstructured.UnstructuredList{}


Why use unstructured?

You should register the scheme (in main.go) and use the typed structure directly

mkolesnik · 2026-05-20T07:32:16Z

+	})
+
+	for _, cluster := range clusters {
+		if err := r.List(ctx, msaList, client.InNamespace(cluster.Name),


Why not simply use client.Get?

e.g.

existing := &msav1beta1.ManagedServiceAccount{} if err := r.Get(ctx, types.NamespacedName{Name: msaName, Namespace: cluster.Name}, existing) ...

mkolesnik · 2026-05-20T07:40:26Z

 }
+
+// CreateManagedServiceAccount creates a ManagedServiceAccount resource for a cluster in the ClusterSet
+func (r *Reconciler) CreateManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh *meshv1alpha1.MultiClusterMesh) error {


Suggested change

func (r *Reconciler) CreateManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh *meshv1alpha1.MultiClusterMesh) error {

func (r *Reconciler) createManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh *meshv1alpha1.MultiClusterMesh) error {

No reason to make it public

mkolesnik · 2026-05-20T07:46:04Z

+			util.CreateMultiClusterMesh(ctx, k8sClient, meshName, testNs, testClusterSet, meshv1alpha1.OperatorConfig{})
+			awaitReconcileFinished()
+
+			list := &unstructured.UnstructuredList{}


Here also, no need to use unstructured, especially since you registered the type in the test schema

mkolesnik · 2026-05-20T07:47:27Z

+
+			specData := map[string]interface{}{
+				"rotation": map[string]interface{}{
+					"validity": "24h0m",


You set it but didn't check it.

Also I think setting custom fields is better checked in a separate test (It)

mkolesnik · 2026-05-20T08:03:59Z

+
+	msa := &msav1v1beta1.ManagedServiceAccount{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("%s-%s", cluster.Name, mesh.Name), // Naming convention: <cluster-name>-<mesh-name>


I think a better convention would be something like <mesh-namespace>-<mesh-name>-istio-reader

Either way cluster name is stuttering since the ns is already the cluster name in this case.

We can further discuss naming convention on #72 as well.

mkolesnik · 2026-05-20T08:12:19Z

+			Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster2))).To(Succeed())
+		})
+
+		When("the ManagedServiceAccount resource exists", func() {


I don't think this makes sense as an integration test.

We don't expect MSA resource to be created by the user externally, which is what this test implies.

We expect them to exist in concurrent reconciliation of the same mesh, in which case the correct tests should test that mesh is updated.

mkolesnik · 2026-05-20T08:14:26Z

+			Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster1))).To(Succeed())
+			Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster2))).To(Succeed())


These don't really test that the MSA were created as expected.

For all we know, they might've been created by something else, or by the controller but not as we expected them

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun · 2026-06-15T20:24:35Z

/hold

yxun · 2026-06-15T20:27:13Z

This PR has been broken into the following four small PRs. All following comments will be addressed in the small PRs.
I will close this original one when all the small PRs are merged.

https://github.com/stolostron/multicluster-mesh-addon/pull/ ? (pending for resolving pr 130 first)

openshift-ci · 2026-06-16T08:49:36Z

PR needs rebase.

Details

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.

Implement ManagedServiceAccount resources creation

9a9aec8

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot added approved needs-rebase labels Apr 29, 2026

Merge conflict from main

92c364b

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot removed the needs-rebase label Apr 29, 2026

fix test error

07d21db

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun marked this pull request as draft April 29, 2026 17:03

openshift-ci Bot added the do-not-merge/work-in-progress label Apr 29, 2026

Add external CRDs in integ test

4d10e71

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun marked this pull request as ready for review May 2, 2026 15:18

openshift-ci Bot removed the do-not-merge/work-in-progress label May 2, 2026

update the integ test

6c84473

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

mkolesnik reviewed May 7, 2026

View reviewed changes

openshift-ci Bot added the needs-rebase label May 15, 2026

Merge from main

5d24bb3

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot removed the needs-rebase label May 18, 2026

review updates

761b348

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun added 2 commits May 19, 2026 08:43

review updates

f0b0afc

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

Update controller test

7fbbe45

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

mkolesnik reviewed May 20, 2026

View reviewed changes

yxun added 2 commits May 21, 2026 07:06

review updates

c270b8c

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

Merge from main

0ebff08

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot added the dco-signoff: yes label May 21, 2026

yxun marked this pull request as draft May 21, 2026 11:19

openshift-ci Bot added the do-not-merge/work-in-progress label May 21, 2026

yxun force-pushed the msa branch from 5563f6f to be99c8f Compare May 21, 2026 11:26

yxun added 2 commits May 26, 2026 09:24

review updates

01727b3

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

Merge branch 'main' into msa

f0b6d64

yxun marked this pull request as draft June 1, 2026 12:36

openshift-ci Bot added the do-not-merge/work-in-progress label Jun 1, 2026

review updates

1187146

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot added the needs-rebase label Jun 3, 2026

Merge from main

6ac2b51

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot removed the needs-rebase label Jun 4, 2026

yxun added 2 commits June 4, 2026 14:59

Merge branch 'main' into msa

2fe673d

review updates

4746185

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

yxun marked this pull request as ready for review June 8, 2026 20:42

openshift-ci Bot removed the do-not-merge/work-in-progress label Jun 8, 2026

yxun changed the title ~~Implement ManagedServiceAccount resources creation~~ Implement ManagedServiceAccount resources reconciliation and tests Jun 8, 2026

yxun added 5 commits June 8, 2026 17:04

fix lint error

302620a

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

fix test errors

9cf6ab7

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

review updates

8866412

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

fix verify job error

e89acd1

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

update tests

287ec22

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot added the needs-rebase label Jun 12, 2026

Merge from main

d49e45f

Signed-off-by: Yuanlin Xu <yuanlin.xu@redhat.com>

openshift-ci Bot removed the needs-rebase label Jun 15, 2026

openshift-ci Bot added the do-not-merge/hold label Jun 15, 2026

yxun marked this pull request as draft June 15, 2026 22:51

openshift-ci Bot added do-not-merge/work-in-progress needs-rebase labels Jun 15, 2026

	msav1v1beta1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1beta1"
	msav1beta1 "open-cluster-management.io/managed-serviceaccount/apis/authentication/v1beta1"

	func (r Reconciler) CreateManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh meshv1alpha1.MultiClusterMesh) error {
	func (r Reconciler) createManagedServiceAccount(ctx context.Context, cluster clusterv1.ManagedCluster, mesh meshv1alpha1.MultiClusterMesh) error {

		Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster1))).To(Succeed())
		Expect(k8sClient.List(context.Background(), list, client.InNamespace(cluster2))).To(Succeed())

Uh oh!

Conversation

yxun commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Apr 29, 2026

Uh oh!

mkolesnik 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yxun commented May 18, 2026

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!

yxun commented Jun 15, 2026

Uh oh!

yxun commented Jun 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Jun 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yxun commented Apr 29, 2026 •

edited

Loading

yxun commented Jun 15, 2026 •

edited

Loading