Update client go #398

kkmsft · 2019-09-28T00:50:50Z

Reason for Change:

Update client-go to avoid thread leak.
- The older go-client had thread leak which would lead to high cpu usage. Fixes by updating the client-go to 1.15.3 related client version. The following has details about the thread leak pkg/util/wait PollUntil() can leak goroutines kubernetes/kubernetes#71277.
- The newer client-go has been made case sensitive because of which we need to have our fields to be case senstivie. Hence the changes in the types.go to update the json tags. The following issue has more details - JSON decoders should be case sensitive kubernetes/kubernetes#64612.
- There has been changes in the usage for leader election. MIC code had to be changed to work with this.
Simple command to test listing and accept GOOS from env
Fix cache sync calls
Misc. logging changes

Issue Fixed:

Notes for Reviewers:
TODO

Fix the usage of exit channel
Run e2e to validate
Add e2e to validate backward compatibility with older 1.5 and 1.3-1.4

kkmsft · 2019-09-28T05:45:26Z

E2E test results. One of them failed and passed on rerun:

Summarizing 1 Failure:

[Fail] Kubernetes cluster using aad-pod-identity [It] should assign identity with init containers
/Users/kkmsft/projects/aad-pod-identity/go/src/github.com/Azure/aad-pod-identity/test/e2e/aadpodidentity_test.go:1216

Ran 17 of 19 Specs in 3898.244 seconds
FAIL! -- 16 Passed | 1 Failed | 0 Pending | 2 Skipped
--- FAIL: TestAADPodIdentity (3898.24s)
FAIL
FAIL github.com/Azure/aad-pod-identity/test/e2e 3898.289s
make: *** [e2e] Error 1

[1] should assign identity with init containers
...

Ran 1 of 19 Specs in 257.761 seconds
SUCCESS! -- 1 Passed | 0 Failed | 0 Pending | 18 Skipped
PASS

aramase

Just few small nits, otherwise lgtm

Since we don't have e2e test for SP, maybe we should validate that scenario manually too before merging the PR? Also, PR needs rebase.

cmd/simple/main.go

pkg/apis/aadpodidentity/v1/types.go

aramase

Added few comments but otherwise looks great!

For the docs, should we update the docs and examples with new case after we cut the release? If we update the examples and docs with this PR, it will not work for users trying out 1.5 releases.

aramase · 2020-03-18T05:44:54Z

cmd/mic/main.go

@@ -72,7 +75,20 @@ func main() {
 	//Identities that should be never removed from Azure AD (used defined managed identities)
 	flag.StringVar(&immutableUserMSIs, "immutable-user-msis", "", "prevent deletion of these IDs from the underlying VM/VMSS")

+	// Config map for aad-pod-identity
+	flag.StringVar(&cmConfig.Name, "cmName", "aad-pod-identity-cm", "Configmap name")


can we change this to config-map-name instead of cmName

can we use aad-pod-identity-config instead of aad-pod-identity-cm?

aramase · 2020-03-18T05:48:29Z

cmd/mic/main.go

+	// Config map for aad-pod-identity
+	flag.StringVar(&cmConfig.Name, "cmName", "aad-pod-identity-cm", "Configmap name")
+	// Config map details for the type changes in the context of client-go upgrade.
+	flag.StringVar(&typeUpgradeConfig.CMTypeUpgradeKey, "typeUpgradeCMKey", "type-upgrade-status", "Configmap key for type upgrade status")


we should use hyphen separated name instead of camel case similar to the other flags we have in MIC

aramase · 2020-03-18T05:55:35Z

cmd/simple/main.go

+	"k8s.io/client-go/rest"
+	"k8s.io/client-go/tools/clientcmd"
+	"k8s.io/klog"
+	//"context"


nit: can we remove this?

aramase · 2020-03-18T05:56:43Z

deploy/demo/aadpodidentitybinding.yaml

+  azureIdentity: "demo-aad1"
+  selector: "demo"


nit: new line

ditto as above

aramase · 2020-03-18T06:10:01Z

pkg/mic/mic.go

@@ -18,6 +18,7 @@ import (
 	"github.com/Azure/aad-pod-identity/version"
 	"golang.org/x/sync/semaphore"
 	corev1 "k8s.io/api/core/v1"
+	kubeErrors "k8s.io/apimachinery/pkg/api/errors"


can we rename this to apierrors instead of kubeErrors?

aramase · 2020-03-18T06:26:11Z

pkg/mic/mic.go

 // Start ...
 func (c *Client) Start(exit <-chan struct{}) {
 	klog.V(6).Infof("MIC client starting..")

+	if err := c.UpgradeTypeIfRequired(); err != nil {
+		klog.Fatalf("Upgrade failed with error: %v", err)


I think we should say type upgrade failed with error. I think we need to make this error message descriptive so the user what upgrade failed here

aramase · 2020-03-18T06:27:34Z

pkg/mic/mic.go

+
+		// There is a delay in data propogation to cache. It's possible that the creates performed in the previous sync cycle
+		// are not propogated before this sync cycle began. In order to avoid redoing the cycle, we sync cache again.
+		c.CRDClient.SyncCacheAll(exit, false)


Were you able to confirm that this cache sync with the go client update has no leaks?

aramase · 2020-03-18T06:28:36Z

pkg/mic/mic.go

@@ -417,19 +522,22 @@ func (c *Client) createDesiredAssignedIdentityList(
 	newAssignedIDs := make(map[string]aadpodid.AzureAssignedIdentity)

 	for _, pod := range listPods {
+		klog.V(6).Infof("checking pod: %s", pod.Name)


can we also add the pod namespace here to the log?

aramase · 2020-03-18T06:28:58Z

pkg/mic/mic.go

 		if pod.Spec.NodeName == "" {
 			//Node is not yet allocated. In that case skip the pod
 			klog.V(2).Infof("Pod %s/%s has no assigned node yet. it will be ignored", pod.Namespace, pod.Name)
 			continue
 		}
 		crdPodLabelVal := pod.Labels[aadpodid.CRDLabelKey]
+		klog.V(6).Infof("Label value: %v", crdPodLabelVal)


I think we should also add the podns and podname in this log

aramase · 2020-03-18T06:29:09Z

pkg/mic/mic.go

 		if crdPodLabelVal == "" {
 			//No binding mentioned in the label. Just continue to the next pod
 			klog.V(2).Infof("Pod %s/%s has correct %s label but with no value. it will be ignored", pod.Namespace, pod.Name, aadpodid.CRDLabelKey)
 			continue
 		}
 		var matchedBindings []aadpodid.AzureIdentityBinding
 		for _, allBinding := range *listBindings {
+			klog.V(6).Infof("Check the binding: %s", allBinding.Spec.Selector)


same as above

aramase · 2020-03-20T19:58:06Z

deploy/demo/aadpodidentity.yaml

@@ -4,5 +4,5 @@ metadata:
 name: demo-aad1
 spec:
 type: 0
- ResourceID: RESOURCE_ID
- ClientID: CLIENT_ID 
+ resourceID: RESOURCE_ID


We should update these manifests right before we cut the new release as this will not work with current release.

aramase

lgtm

@kkmsft looks like there are merge conflicts.

kkmsft requested a review from aramase September 28, 2019 00:50

kkmsft force-pushed the go-client branch from 07fd635 to 6571d64 Compare September 28, 2019 00:59

kkmsft force-pushed the go-client branch 2 times, most recently from 159219c to ee325ac Compare February 1, 2020 02:14

aramase reviewed Feb 6, 2020

View reviewed changes

cmd/simple/main.go Outdated Show resolved Hide resolved

pkg/apis/aadpodidentity/v1/types.go Outdated Show resolved Hide resolved

kkmsft force-pushed the go-client branch 2 times, most recently from 54220a3 to 90e16f9 Compare February 13, 2020 21:45

kkmsft force-pushed the go-client branch from 90e16f9 to c4aeb65 Compare March 16, 2020 19:41

aramase reviewed Mar 18, 2020

View reviewed changes

aramase reviewed Mar 20, 2020

View reviewed changes

aramase approved these changes Mar 21, 2020

View reviewed changes

kkmsft added 7 commits March 20, 2020 18:14

Client-go update and type upgrade

a0c02c3

Add simple tool to list the CRDs

153a143

Test changes for type update

28ff5f9

Remove unused rc deployment files

e31a238

Fixes from testing

d75fdfb

Address review comments

f65e05b

Quick go mod tidy

31e0422

kkmsft force-pushed the go-client branch from d23e425 to 31e0422 Compare March 21, 2020 01:15

kkmsft merged commit 96e329b into Azure:master Mar 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update client go #398

Update client go #398

kkmsft commented Sep 28, 2019

kkmsft commented Sep 28, 2019 •

edited

Loading

aramase left a comment

aramase left a comment

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 20, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 18, 2020

aramase Mar 20, 2020 •

edited

Loading

aramase left a comment

Update client go #398

Update client go #398

Conversation

kkmsft commented Sep 28, 2019

kkmsft commented Sep 28, 2019 • edited Loading

aramase left a comment

Choose a reason for hiding this comment

aramase 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

aramase Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

aramase left a comment

Choose a reason for hiding this comment

kkmsft commented Sep 28, 2019 •

edited

Loading

aramase Mar 20, 2020 •

edited

Loading