Aad pod identity as add on #3475

kkmsft · 2018-07-13T21:16:55Z

What this PR does / why we need it:

Add deploying Aad pod Identity as a add-on for acs engine created cluster.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

kkmsft · 2018-07-13T21:17:25Z

/cc @khenidak @rite2nikhil @ritazh

ritazh · 2018-07-13T21:34:35Z

I think we should hold off merging this until pod identity works with azure cni (no podCIDR) and RBAC, per these two issues: Azure/aad-pod-identity#40 and Azure/aad-pod-identity#39

khenidak · 2018-07-17T19:29:25Z

@kkmsft can we remove the [WIP] tag and prep to merge this?
@jackfrancis @CecileRobertMichon I appreciate if you review this.

jackfrancis · 2018-07-17T19:41:08Z

parts/k8s/kubernetescustomscript.sh

@@ -486,6 +486,10 @@ configACIConnectorAddon() {
    sed -i "s|<kubernetesACIConnectorKey>|$(echo $ACI_CONNECTOR_KEY)|g" "/etc/kubernetes/addons/aci-connector-deployment.yaml"
 }

+configAADPodIdentityAddon() {
+    sed -i "s|<kubernetesAADPodIdentityCredentials>|$AAD_POD_IDENTITY|g" "/etc/kubernetes/addons/aad-pod-identity-deployment.yaml"


It looks like we aren't actually passing in AAD_POD_IDENTITY or AAD_POD_IDENTITY_ADDON env vars yet. May I suggest that we drop the "boolean" AAD_POD_IDENTITY_ADDON var, and simply check for non-empty string value of AAD_POD_IDENTITY to determine whether or not we want to do the sed replacement?

Thank you, Jack. Since we don't have anything to pass and use values from azure.json, this is not required. So have removed this.

ritazh · 2018-07-18T00:53:15Z

examples/addons/aad-pod-identity/README.md

+            {
+              "name": "aad-pod-identity",
+              "enabled" : true,
+              "config": {


I think these configs are specific to the aci-connector. I dont think config is neccessary for pod identity.

ritazh · 2018-07-18T01:02:56Z

examples/addons/aad-pod-identity/kubernetes-aad-pod-identity.json

+        "name": "agentpool",
+        "count": 3,
+        "vmSize": "Standard_DS2_v2",
+        "availabilityProfile": "VirtualMachineScaleSets"


Need to support VMSS first...hold this PR until that is added or change this to AvailabilitySet

Using AvailablitySet in this PR as the example. Will add the ScaleSet after adding the support for that in aad-pod-identity.

ritazh · 2018-07-18T01:06:24Z

parts/k8s/addons/kubernetesmasteraddons-aad-pod-identity.yaml

+apiVersion: v1
+kind: Secret
+metadata:
+  name: aad-pod-identity-secret


we dont need this for pod identity

Using azure.json directly.

ritazh · 2018-07-18T01:07:13Z

parts/k8s/kubernetesmastervars.t

@@ -103,6 +103,16 @@
    "kubernetesTillerCPULimit": "[parameters('kubernetesTillerCPULimit')]",
    "kubernetesTillerMemoryLimit": "[parameters('kubernetesTillerMemoryLimit')]",
    "kubernetesTillerMaxHistory": "[parameters('kubernetesTillerMaxHistory')]",
+    "kubernetesAADPodIdentityEnabled": "[parameters('kubernetesAADPodIdentityEnabled')]",
+    "kubernetesAADPodIdentitySpec": "[parameters('kubernetesAADPodIdentitySpec')]",
+    "kubernetesAADPodIdentityNodeName": "[parameters('kubernetesAADPodIdentityNodeName')]",


These are specific to aci-connector. I dont think we need these for pod identity.

Enabled flag remains. Rest removed.

ritazh · 2018-07-18T01:08:58Z

parts/k8s/kubernetesparams.t

+      },
+      "type": "string"
+    },
+    "kubernetesAADPodIdentityOS": {


I think these params are specific to the aci-connector. I dont think config is neccessary for pod identity.

Thanks Rita. Removed them. Remnants from following a template example :-)

khenidak · 2018-07-19T15:48:58Z

@jackfrancis @ritazh @kkmsft @CecileRobertMichon
Can we merge this?

ritazh · 2018-07-19T16:03:16Z

examples/addons/aad-pod-identity/README.md

+          "name": "agentpool",
+          "count": 3,
+          "vmSize": "Standard_DS2_v2",
+          "availabilityProfile": "AvailabilitySet"


Please add a comment to indicate currently only "AvailabilitySet" is supported to avoid users creating using "VirtualMachineScaleSets".

ritazh · 2018-07-19T16:07:47Z

examples/addons/aad-pod-identity/README.md

+
+You can validate that the add-on is running as expected with the following commands:
+
+You should see two pod names - one starting with mic and another nmi as `Running` after executing:


You should see two sets of pods - a single mic pod and as many nmi pods as there are agent nodes in Running state after executing:

ritazh · 2018-07-19T16:10:33Z

pkg/acsengine/addons.go

+			"kubernetesmasteraddons-aad-pod-identity-deployment.yaml",
+			"aad-pod-identity-deployment.yaml",
+			profile.OrchestratorProfile.KubernetesConfig.IsAADPodIdentityEnabled(),
+		},


nit: spacing

codecov · 2018-07-19T16:13:30Z

Codecov Report

Merging #3475 into master will increase coverage by 0.36%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master    #3475      +/-   ##
==========================================
+ Coverage   55.49%   55.85%   +0.36%     
==========================================
  Files         105      105              
  Lines       15990    16003      +13     
==========================================
+ Hits         8873     8938      +65     
+ Misses       6369     6316      -53     
- Partials      748      749       +1

ritazh · 2018-07-19T16:15:13Z

pkg/acsengine/defaults.go

+		Containers: []api.KubernetesContainerSpec{
+			{
+				Name:           "mic",
+				Image:          "nikhilbh/mic:1.0",


Remove image, it is not being used

ritazh · 2018-07-19T16:15:26Z

pkg/acsengine/defaults.go

+			},
+			{
+				Name:           "nmi",
+				Image:          "nikhilbh/nmi:1.0",


Remove image, it is not being used

ritazh · 2018-07-19T16:50:50Z

parts/k8s/addons/kubernetesmasteraddons-aad-pod-identity-deployment.yaml

+      - name: nmi
+        image: "nikhilbh/nmi:1.2"
+        imagePullPolicy: Always
+        args:


I think you need resource limits and requests here.

ritazh · 2018-07-19T16:51:11Z

parts/k8s/addons/kubernetesmasteraddons-aad-pod-identity-deployment.yaml

+        component: mic
+    spec:
+      serviceAccountName: aad-pod-id-mic-service-account
+      containers:


Also here. I think you need resource limits and requests here.

ritazh · 2018-07-20T05:29:44Z

LGTM @jackfrancis will add an issue to add validation test for this

jackfrancis · 2018-07-20T16:39:26Z

@ritazh In your // TODO issue add configurable resource limits

Thanks so much Rita and @kkmsft!

jackfrancis

lgtm

jackfrancis · 2018-07-20T16:47:14Z

@ritazh and E2E test coverage :)

jackfrancis suggested changes Jul 17, 2018

View reviewed changes

ritazh suggested changes Jul 18, 2018

View reviewed changes

kkmsft force-pushed the aad-pod-identity branch 6 times, most recently from d86fab9 to 9d83cbf Compare July 19, 2018 03:12

kkmsft changed the title ~~[WIP] Aad pod identity as add on~~ Aad pod identity as add on Jul 19, 2018

ritazh suggested changes Jul 19, 2018

View reviewed changes

ritazh reviewed Jul 19, 2018

View reviewed changes

pkg/acsengine/defaults.go Outdated

},

{

Name: "nmi",

Image: "nikhilbh/nmi:1.0",

Copy link

Member

ritazh Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove image, it is not being used

ritazh reviewed Jul 19, 2018

View reviewed changes

kkmsft force-pushed the aad-pod-identity branch from 9d83cbf to 5e5d58b Compare July 19, 2018 21:05

AAD pod identity add on changes.

0fe25ea

kkmsft force-pushed the aad-pod-identity branch from eeb7430 to 0fe25ea Compare July 20, 2018 02:13

ritazh approved these changes Jul 20, 2018

View reviewed changes

ritazh mentioned this pull request Jul 20, 2018

Add validation for aad-pod-identity addon to ensure only AvailabilitySet is used #3523

Closed

jackfrancis approved these changes Jul 20, 2018

View reviewed changes

jackfrancis merged commit fdcd8cb into Azure:master Jul 20, 2018

kkmsft deleted the aad-pod-identity branch October 14, 2018 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aad pod identity as add on #3475

Aad pod identity as add on #3475

kkmsft commented Jul 13, 2018 •

edited

Loading

kkmsft commented Jul 13, 2018

ritazh commented Jul 13, 2018

khenidak commented Jul 17, 2018

jackfrancis Jul 17, 2018

kkmsft Jul 19, 2018

ritazh Jul 18, 2018

kkmsft Jul 19, 2018

ritazh Jul 18, 2018

kkmsft Jul 19, 2018

ritazh Jul 18, 2018

kkmsft Jul 19, 2018

ritazh Jul 18, 2018

kkmsft Jul 19, 2018

ritazh Jul 18, 2018

kkmsft Jul 19, 2018

khenidak commented Jul 19, 2018

ritazh Jul 19, 2018

ritazh Jul 19, 2018 •

edited

Loading

ritazh Jul 19, 2018

codecov bot commented Jul 19, 2018 •

edited

Loading

ritazh Jul 19, 2018

ritazh Jul 19, 2018

ritazh Jul 19, 2018

ritazh Jul 19, 2018

ritazh commented Jul 20, 2018

jackfrancis commented Jul 20, 2018

jackfrancis left a comment

jackfrancis commented Jul 20, 2018


		You can validate that the add-on is running as expected with the following commands:

		You should see two pod names - one starting with mic and another nmi as `Running` after executing:

Aad pod identity as add on #3475

Aad pod identity as add on #3475

Conversation

kkmsft commented Jul 13, 2018 • edited Loading

kkmsft commented Jul 13, 2018

ritazh commented Jul 13, 2018

khenidak commented Jul 17, 2018

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

khenidak commented Jul 19, 2018

Choose a reason for hiding this comment

ritazh Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2018 • 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

ritazh commented Jul 20, 2018

jackfrancis commented Jul 20, 2018

jackfrancis left a comment

Choose a reason for hiding this comment

jackfrancis commented Jul 20, 2018

kkmsft commented Jul 13, 2018 •

edited

Loading

ritazh Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 19, 2018 •

edited

Loading