Added a watcher for secret #169

Nithish555 · 2023-03-21T15:55:07Z

No description provided.

Anandkumar26

Remove the update related functions in the secret_webhook.go and secret_webhook_test.go.

Anandkumar26 · 2023-03-27T11:23:33Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"

+	"antrea.io/antrea/pkg/util/env"


Should we really import a new package to access the environment variable? Can we not access it directly?

Yeah. We should import os or this package.

pkg/controllers/cloud/cloudprovideraccount_controller.go

Anandkumar26 · 2023-03-27T13:41:01Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	// Client in controller requires reconciler for each object that are under watch. So to avoid reconciler and use only
+	// watch, that can be implemented by clientset.
+	if r.clientset, err = kubernetes.NewForConfig(ctrl.GetConfigOrDie()); err != nil {
+		r.Log.Error(err, "error while creating config")


Can you re-phrase the error, as config does not indicate which config is being referred to.?

What should we add.

may be "error while fetching cluster configuration"?

Anandkumar26 · 2023-03-27T13:41:25Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

 	r.Log.Info("Waiting for shared informer caches to be synced")
 	// Blocking call to wait till the informer caches are synced by controller run-time
 	// or the context is Done.
 	if !(*r.Mgr).GetCache().WaitForCacheSync(context.TODO()) {
 		return fmt.Errorf("failed to sync shared informer cache")
 	}
-


re-add the space.

pkg/controllers/cloud/cloudprovideraccount_controller.go

Anandkumar26 · 2023-03-27T13:49:29Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	}
+}
+
+// resetWatcher Create a watcher for secret


Suggested change

// resetWatcher Create a watcher for secret

// resetWatcher creates a watcher for Secret

Anandkumar26 · 2023-03-27T13:50:03Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	var err error
+	for {
+		if r.watcher, err = r.clientset.CoreV1().Secrets(env.GetPodNamespace()).Watch(context.Background(), metav1.ListOptions{}); err != nil {
+			r.Log.Error(err, "error while creating secret watcher")


Suggested change

r.Log.Error(err, "error while creating secret watcher")

r.Log.Error(err, "error creating Secret watcher")

Anandkumar26 · 2023-03-27T13:50:31Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+
+}
+
+// setupSecretWatcher Set up a watcher.


Suggested change

// setupSecretWatcher Set up a watcher.

// setupSecretWatcher set up a watcher for Secret objects.

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

Anandkumar26 · 2023-03-27T13:52:14Z

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

+			_ = fakeClient.Create(context.Background(), account)
+			go func() {
+				//err = reconciler.setupSecretWatcher()
+				//Expect(err).Should(HaveOccurred())


nit: remove?

Cleanup the unit-tests and remove unused code.

Anandkumar26 · 2023-03-29T05:35:43Z

go.mod

@@ -38,6 +38,7 @@ require (
 	github.com/PuerkitoBio/purell v1.1.1 // indirect
 	github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
+	github.com/blang/semver v3.5.1+incompatible // indirect


In the L42, latest version of github.com/blang/semver is already added as indirect dependency. So you can remove this. Same for go.sum file as well.

Anandkumar26 · 2023-03-29T05:53:36Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+
+// getCPABySecret returns nil only when the Secret is not used by any CloudProvideAccount CR,
+// otherwise the dependent CloudProvideAccount CR will be returned.
+func (r *CloudProviderAccountReconciler) getCPABySecret(s types.NamespacedName) (error, []cloudv1alpha1.CloudProviderAccount) {


Change the function to getCpaBySecret.

Anandkumar26 · 2023-03-29T06:40:02Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+				namespacedName := types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name}
+				err, cpaItems := r.getCPABySecret(namespacedName)
+				if err != nil {
+					r.Log.Error(err, "error while getting CPA by Secret %v", secret.Name)


For consistent logging(Similiar to L297), can you dump the secret NamespacedName instead of Name?

Anandkumar26 · 2023-03-29T06:46:39Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+					// result vpc cache will contain vpcs from old and new accounts.
+					err := r.processCreate(&namespacedName, &cpa)
+					if err != nil {
+						r.Log.Error(err, "error while adding the secret in CPAs")


Any error returned in watchSecret() will exit the controller and restart the pod.
IMO, we should not be restarting for error in L293, since only one CPA credentials update failed. May be we can just log an error and let user check the logs.

@reachjainrahul Any suggestions here?

Anandkumar26 · 2023-03-29T06:47:32Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	}
+}
+
+// setupSecretWatcher Set up a watcher for Secret objects.


Suggested change

// setupSecretWatcher Set up a watcher for Secret objects.

// setupSecretWatcher set up a watcher for Secret objects.

Anandkumar26 · 2023-03-29T06:49:01Z

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

+			// Create CPA
+			_ = fakeClient.Create(context.Background(), account)
+			go func() {
+				ch <- reconciler.setupSecretWatcher() // writing


nit: remove //comment

Anandkumar26 · 2023-03-29T06:49:36Z

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

+			err = os.Setenv("POD_NAMESPACE", testSecretNamespacedName.Namespace)
+			Expect(err).ShouldNot(HaveOccurred())
+			// Create secret
+			_, err = reconciler.clientset.CoreV1().Secrets(testSecretNamespacedName.Namespace).


Add a comment explaining why you need to created a Secret using clientset and fakeClient

Anandkumar26 · 2023-03-30T12:12:59Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+				r.mutex.Lock()
+				secret := event.Object.(*v1.Secret)
+				namespacedName := types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name}
+				r.Log.Info("Received request for update", "Secret", namespacedName)


Suggested change

r.Log.Info("Received request for update", "Secret", namespacedName)

r.Log.Info("Received request", "Secret", namespacedName, "operation", update)

Anandkumar26

Add a brief explanation in the commit body about this PR.

Anandkumar26 · 2023-03-30T13:13:30Z

config/manager/nephe-controller.yaml

+          - name: POD_NAMESPACE
+            valueFrom:
+              fieldRef:
+                fieldPath: metadata.namespace


This will require changes in the helm charts as well.
here: nephe/build/charts/nephe/templates/controller/deployment.yaml

Anandkumar26 · 2023-03-30T13:14:54Z

pkg/apiserver/webhook/secret_webhook_test.go

-			_, _ = GinkgoWriter.Write([]byte(fmt.Sprintf("Got admission response %+v\n", response)))
-			Expect(response.Allowed).To(BeTrue())
-		})
-		It("Validate Secret delete without dependent AWS CPA", func() {


Delete tests should not be removed.

Anandkumar26 · 2023-03-30T13:15:08Z

pkg/apiserver/webhook/secret_webhook_test.go

-		})
-
-		// The below set of tests validate Secret update/delete for Azure config.
-		It("Validate Azure Secret delete with dependent Azure CPA", func() {


Anandkumar26 · 2023-03-30T13:18:40Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

 	r.accountProviderType = make(map[types.NamespacedName]common.ProviderType)
+	// Client in controller requires reconciler for each object that are under watch. So to avoid reconciler and use only


When you say client, you are referring to client from controller-runtime/pkg/client right?

Anandkumar26 · 2023-03-30T13:19:49Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

 		}
 	}
 }

+// setWatcher sets the controller sync status and watcher.


nit: update function name

Anandkumar26 · 2023-03-30T13:20:18Z

pkg/controllers/cloud/cloudprovideraccount_controller.go


 	delete(r.accountProviderType, *namespacedName)
 }

 func (r *CloudProviderAccountReconciler) getAccountProviderType(namespacedName *types.NamespacedName) common.ProviderType {
-	r.mutex.Lock()
-	defer r.mutex.Unlock()



Anandkumar26 · 2023-03-30T13:20:38Z

pkg/controllers/cloud/cloudprovideraccount_controller.go


 	return r.accountProviderType[*namespacedName]
 }
+
+// getCpaBySecret returns nil only when the Secret is not used by any CloudProvideAccount CR,
+// otherwise the dependent CloudProvideAccount CR will be returned.


Suggested change

// otherwise the dependent CloudProvideAccount CR will be returned.

// otherwise the dependent CloudProvideAccount CRs will be returned.

Anandkumar26 · 2023-03-30T13:21:52Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+func (r *CloudProviderAccountReconciler) getCpaBySecret(s types.NamespacedName) (error, []crdv1alpha1.CloudProviderAccount) {
+	cpaList := &crdv1alpha1.CloudProviderAccountList{}
+	if err := r.Client.List(context.TODO(), cpaList, &client.ListOptions{}); err != nil {
+		return fmt.Errorf("failed to get CloudProviderAccount list, err:%v", err), nil


nit: for consistency

Suggested change

return fmt.Errorf("failed to get CloudProviderAccount list, err:%v", err), nil

return fmt.Errorf("failed to get CloudProviderAccount list, err %v", err), nil

Anandkumar26 · 2023-03-30T13:23:31Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	}
+}
+
+// resetWatcher create a watcher for Secret


nit: function name different

Anandkumar26 · 2023-03-30T13:27:53Z

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

+			Expect(err).ShouldNot(HaveOccurred())
+			// Create secret
+			// Created a secret using clientset because watcher is created using clientset. So inorder to know
+			// watcher if the secret is modified secret should be created using clientset. Secret should be created using


Does this convey message correctly?
Create a secret using both fakeClient and clientSet, since secret watcher uses cleintset while reconciler using client from controller run-time. Both secret watcher and reconciler should be able to retrieve the secret object.

Anandkumar26 · 2023-03-31T10:32:53Z

pkg/config/env.go

+func GetPodNamespace() string {
+	podNamespace := os.Getenv(PodNamespaceEnvKey)
+	if podNamespace == "" {
+		klog.Errorf("environment variable %v not found", PodNamespaceEnvKey)


Do not add new logger package here. We don't use klog.

What log should we add.

Its actually a FATAL error in our case. If this does not exist, then we should continue further.

The log message you added does not crash the pod. Suppose namespace is not set, what would happen to the watcher? will it throw an error? If it throws an error, then we will be in state where we are continuously calling resetting the watcher object.

In our class we handle this error.

pkg/controllers/cloud/cloudprovideraccount_controller.go

Anandkumar26 · 2023-03-31T10:36:22Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+				r.mutex.Lock()
+				secret := event.Object.(*v1.Secret)
+				namespacedName := types.NamespacedName{Namespace: secret.Namespace, Name: secret.Name}
+				r.Log.WithName("Secret").Info("Received request", "Secret", namespacedName, "operation", "update")


Instead of prefixing all the log messages with r.Log.WithName("Secret")
can we do initialize a local variable and use it?
log := Log.WithName("Secret")

Then we should initialize logger for each secret methods.

In the other functions, you can keep r.Log.WithName("Secret")

Why only this function requires variable

Is it temporary, or this is the way we will have Secret prefix ?

reachjainrahul · 2023-04-03T03:40:57Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+	// Client in controller requires reconciler for each object that are under watch. So to avoid reconciler and use only
+	// watch, that can be implemented by clientset.
+	if r.clientset, err = kubernetes.NewForConfig(ctrl.GetConfigOrDie()); err != nil {
+		r.Log.Error(err, "error creating client config")


creating config or just client?

Both right. Because ctr.GetConfigOrDie creates config and kubernetes creates clientset

reachjainrahul · 2023-04-03T03:44:37Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+				for _, cpa := range cpaItems {
+					// If the credentials are updated to new cloud account then vpc cache is not updated. As a
+					// result vpc cache will contain vpcs from old and new accounts.
+					accountNamespacedName := types.NamespacedName{Namespace: cpa.Namespace, Name: cpa.Name}


is it a bug?

@Anandkumar26 Please comment on this.

@reachjainrahul Consider we have account 1, with cred 1. We get 10 VPCs from account.
Now user updates the cred 1 to cred 2. (Cred 2 can potential point to different account, same region).

In this particular scenario, VPCs cache will have:

VPCs from account 1 (Since they never got deleted upon secret modification)

VPCs from new account (updated credentials)

@Nithish555 please remove the comment..

Please remove the entire comment

reachjainrahul · 2023-04-03T03:48:10Z

pkg/controllers/cloud/cloudprovideraccount_controller.go

+}
+
+// getPodNamespace gets the namespace of the pod.
+func (r *CloudProviderAccountReconciler) getPodNamespace() string {


I dont think this is required here.. You should check for empty ns in GetPodNamespace. not here.. Also verify if nephe is launched in default Namespace, whats the value you get.

This is required because the GetPodNamespace is generic so we should not handle it there. Because in this case we exit the pod.
The same namespace value which is configured in the pod metadata.

reachjainrahul · 2023-04-03T03:48:45Z

pkg/controllers/cloud/cloudprovideraccount_controller_test.go

+			credential := `{"accessKeyId": "keyId","accessKeySecret": "Secret"}`
+			err = os.Setenv("POD_NAMESPACE", testSecretNamespacedName.Namespace)
+			Expect(err).ShouldNot(HaveOccurred())
+			// Create a secret using both fakeClient and clientSet, since secret watcher uses cleintset


typo clientset

reachjainrahul

/LGTM

reachjainrahul · 2023-04-03T11:21:51Z

/nephe-test-e2e-kind

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

reachjainrahul

/LGTM

Nithish555 force-pushed the secret branch 5 times, most recently from 796743b to 213b67b Compare March 27, 2023 11:26

Anandkumar26 reviewed Mar 27, 2023

View reviewed changes

Nithish555 force-pushed the secret branch from 213b67b to 89a9a01 Compare March 27, 2023 15:53

Anandkumar26 reviewed Mar 29, 2023

View reviewed changes

Nithish555 force-pushed the secret branch 6 times, most recently from e03b059 to dff0991 Compare March 30, 2023 12:01

Anandkumar26 reviewed Mar 30, 2023

View reviewed changes

Nithish555 force-pushed the secret branch from dff0991 to 2f88325 Compare March 30, 2023 12:42

Anandkumar26 reviewed Mar 30, 2023

View reviewed changes

Nithish555 force-pushed the secret branch 3 times, most recently from 855e065 to cfd4867 Compare March 31, 2023 10:29

Anandkumar26 reviewed Mar 31, 2023

View reviewed changes

pkg/controllers/cloud/cloudprovideraccount_controller.go Outdated Show resolved Hide resolved

Anandkumar26 reviewed Mar 31, 2023

View reviewed changes

pkg/controllers/cloud/cloudprovideraccount_controller.go Outdated Show resolved Hide resolved

Anandkumar26 reviewed Mar 31, 2023

View reviewed changes

Nithish555 force-pushed the secret branch 3 times, most recently from ebf6687 to b9b28ed Compare March 31, 2023 11:44

reachjainrahul reviewed Apr 3, 2023

View reviewed changes

reachjainrahul linked an issue Apr 3, 2023 that may be closed by this pull request

Support updates for CPA Secrets #152

Closed

Nithish555 force-pushed the secret branch from b9b28ed to 269ff98 Compare April 3, 2023 05:45

reachjainrahul previously approved these changes Apr 3, 2023

View reviewed changes

Nithish555 dismissed reachjainrahul’s stale review via 7c04a9c April 3, 2023 07:58

Nithish555 force-pushed the secret branch 3 times, most recently from 2632dfa to a48b6f0 Compare April 3, 2023 10:19

Added a watcher for secret

2d507b2

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

reachjainrahul force-pushed the secret branch from a48b6f0 to 2d507b2 Compare April 3, 2023 12:27

reachjainrahul approved these changes Apr 3, 2023

View reviewed changes

reachjainrahul merged commit a54a589 into antrea-io:main Apr 3, 2023

reachjainrahul added this to the Nephe v0.5 release milestone Jun 2, 2023

	// resetWatcher Create a watcher for secret
	// resetWatcher creates a watcher for Secret

	r.Log.Error(err, "error while creating secret watcher")
	r.Log.Error(err, "error creating Secret watcher")

	// setupSecretWatcher Set up a watcher.
	// setupSecretWatcher set up a watcher for Secret objects.

	r.Log.Info("Received request for update", "Secret", namespacedName)
	r.Log.Info("Received request", "Secret", namespacedName, "operation", update)

		r.accountProviderType = make(map[types.NamespacedName]common.ProviderType)
		// Client in controller requires reconciler for each object that are under watch. So to avoid reconciler and use only

	// otherwise the dependent CloudProvideAccount CR will be returned.
	// otherwise the dependent CloudProvideAccount CRs will be returned.

	return fmt.Errorf("failed to get CloudProviderAccount list, err:%v", err), nil
	return fmt.Errorf("failed to get CloudProviderAccount list, err %v", err), nil

Added a watcher for secret #169

Added a watcher for secret #169

Conversation

Nithish555 commented Mar 21, 2023

Anandkumar26 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

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

Anandkumar26 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

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

Nithish555 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anandkumar26 Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nithish555 Apr 3, 2023 •

edited

Loading

Anandkumar26 Apr 3, 2023 •

edited

Loading