feat: create kms key as part of cluster bootstrap #4170

aramase · 2021-01-15T01:06:21Z

Reason for Change:

Create KMS key as part of cluster boostrap
- This is the first step to making the kms key version mandatory for the kms plugin pod.
- This will also allow BYOK configuration with KMS with the next KMS release.
- Deterministic behavior instead of relying on runtime for key creation.
Fixes using premium sku with user-assigned managed identity

Issue Fixed:

Credit Where Due:

Does this change contain code from or inspired by another project?

No
Yes

If "Yes," did you notify that project's maintainers and provide attribution?

No
Yes

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

cc @ritazh

codecov · 2021-01-15T23:20:22Z

Codecov Report

Merging #4170 (07f3123) into master (bcc8a73) will increase coverage by 0.03%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master    #4170      +/-   ##
==========================================
+ Coverage   73.27%   73.31%   +0.03%     
==========================================
  Files         135      135              
  Lines       20720    20765      +45     
==========================================
+ Hits        15183    15224      +41     
- Misses       4562     4566       +4     
  Partials      975      975

Impacted Files	Coverage Δ
pkg/engine/masterarmresources.go	`67.16% <0.00%> (-1.02%)`	⬇️
pkg/engine/params_k8s.go	`80.13% <0.00%> (-0.56%)`	⬇️
pkg/engine/templates_generated.go	`44.19% <71.42%> (+0.49%)`	⬆️
pkg/engine/armvariables.go	`86.13% <100.00%> (+0.69%)`	⬆️
pkg/engine/keyvaults.go	`100.00% <100.00%> (ø)`
pkg/engine/template_generator.go	`68.30% <100.00%> (+0.29%)`	⬆️
pkg/api/common/versions.go	`96.37% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcc8a73...07f3123. Read the comment docs.

aramase · 2021-01-16T00:22:00Z

CI failures are because of cloud-init payload limit:

$ az deployment group create --name kubernetes-southeastasia-63191 --resource-group kubernetes-southeastasia-63191 --template-file /__w/1/s/_output/kubernetes-southeastasia-63191/azuredeploy.json --parameters /__w/1/s/_output/kubernetes-southeastasia-63191/azuredeploy.parameters.json
..........2021/01/16 00:19:51 
Error from deployment for kubernetes-southeastasia-63191 in resource group kubernetes-southeastasia-63191:exit status 1
2021/01/16 00:19:51 Command Output: ValidationError: Deployment failed. Correlation ID: 959b9fbd-41f9-4e00-a67f-7218e9a6f9b2. {
  "error": {
    "code": "InvalidParameter",
    "message": "Custom data in OSProfile must be in Base64 encoding and with a maximum length of 87380 characters.",
    "target": "customData"
  }
}

parts/k8s/cloud-init/artifacts/cse_main.sh

jackfrancis · 2021-01-19T20:56:01Z

pkg/engine/masterarmresources.go

@@ -192,9 +193,11 @@ func createKubernetesMasterResourcesVMSS(cs *api.ContainerService) []interface{}
 	}

 	if isKMSEnabled {
+		// TODO (aramase) remove storage account creation as part of kms plugin v0.0.11


should we do this in this PR?

The current release v0.0.10 is still dependent on the storage account to exist. I'll remove it when we cut a kms release v0.0.11 and update the version in aks-engine.

jackfrancis

/lgtm

ritazh · 2021-01-20T01:04:17Z

.pipelines/pr-e2e.yaml

@@ -64,7 +64,7 @@ jobs:
    k8sRelease: '1.18'
    apimodel: 'examples/e2e-tests/kubernetes/release/default/definition.json'
    createVNET: true
-    enableKMSEncryption: true
+    enableKMSEncryption: false


We don't want to test this as part of PR gate anymore?

The new implementation brings excess cloud-init data overhead, which exceeds the allowed (64k bytes) limit when incorporated into the cluster config we're testing.

So we've added a discrete KMS-enabled cluster config to the set of jenkins-enabled cluster configs we test out of band.

Let's document how to run the kms-enabled jenkins job.

parts/k8s/cloud-init/artifacts/kms-keyvault-key.sh

aramase · 2021-01-20T02:07:54Z

Tried to get the key version by using outputs and reference in arm-template but reference can't be used with variables or parameters.

Example arm snippet used:

    "resources": [
        {
            "type": "Microsoft.KeyVault/vaults/keys",
            "apiVersion": "2019-09-01",
            "name": "[concat(parameters('vaultName'), '/', parameters('keyName'))]",
            "location": "southcentralus",
            "properties": {
                "kty": "[parameters('keyType')]",
                "keyOps": "[parameters('keyOps')]",
                "keySize": "[parameters('keySize')]",
                "curveName": "[parameters('curveName')]"
            }
        }
    ],
    "variables": {
        "kmsEncryptionKey": "[resourceId('Microsoft.KeyVault/vaults/keys', parameters('vaultName'), parameters('keyName'))]"
    },
    "outputs": {
        "kmsKey": {
            "type": "string",
            "value": "[reference(resourceId('Microsoft.KeyVault/vaults/keys', parameters('vaultName'), parameters('keyName'))).keyUriWithVersion]"
        }
    }

https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-outputs?tabs=azure-powershell
https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-functions-resource?tabs=json#reference

ritazh · 2021-01-20T02:28:07Z

parts/k8s/cloud-init/artifacts/kms-keyvault-key.sh

+SERVICE_PRINCIPAL_CLIENT_SECRET=$(jq -r '.aadClientSecret' ${AZURE_JSON_PATH})
+TENANT_ID=$(jq -r '.tenantId' ${AZURE_JSON_PATH})
+KMS_KEYVAULT_NAME=$(jq -r '.providerVaultName' ${AZURE_JSON_PATH})
+KMS_KEY_NAME=$(jq -r '.providerKeyName' ${AZURE_JSON_PATH})


where do you set the vault name and the key name in azure.json?

These 2 values are already set when azure.json is created the first time - https://github.com/Azure/aks-engine/blob/master/parts/k8s/cloud-init/artifacts/cse_config.sh#L212-L215

I was referring to how the keyname is passed into the cse_config.sh. But I see you are hardcoding it in the ARM template:
[concat(variables('clusterKeyVaultName'), '/', 'k8s')]",

aramase · 2021-01-21T17:21:41Z

@jackfrancis The pr-e2e (k8s_1_19_containerd_e2e) failure seems to be a flake. This PR is ready for final review and merge.

@ritazh Addressed comments. PTAL!

acs-bot · 2021-01-25T07:03:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aramase, jackfrancis, ritazh

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis,ritazh]

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

aramase force-pushed the kms-v2 branch 6 times, most recently from 83bd5b4 to c142de6 Compare January 15, 2021 23:05

aramase force-pushed the kms-v2 branch from c142de6 to 3e0c665 Compare January 15, 2021 23:50

aramase marked this pull request as ready for review January 16, 2021 00:06

feat: create kms key as part of cluster bootstrap

2137400

aramase force-pushed the kms-v2 branch from 3e0c665 to 2137400 Compare January 19, 2021 17:41

remove external kms encryption from e2e config

10c53a6

jackfrancis reviewed Jan 19, 2021

View reviewed changes

parts/k8s/cloud-init/artifacts/cse_main.sh Outdated Show resolved Hide resolved

jackfrancis reviewed Jan 19, 2021

View reviewed changes

add kms api model in test/e2e/test_cluster_configs

3972994

aramase force-pushed the kms-v2 branch from 799c687 to 3972994 Compare January 19, 2021 22:43

jackfrancis previously approved these changes Jan 19, 2021

View reviewed changes

set enableKMSEncryption to false in pr-e2e

e94b620

aramase dismissed jackfrancis’s stale review via e94b620 January 19, 2021 23:37

ritazh reviewed Jan 20, 2021

View reviewed changes

parts/k8s/cloud-init/artifacts/kms-keyvault-key.sh Outdated Show resolved Hide resolved

ritazh reviewed Jan 20, 2021

View reviewed changes

set endpoint based on cloud env

07f3123

aramase force-pushed the kms-v2 branch from 40e9853 to 07f3123 Compare January 20, 2021 18:18

ritazh approved these changes Jan 25, 2021

View reviewed changes

jackfrancis merged commit 5d932f1 into Azure:master Jan 25, 2021

aramase deleted the kms-v2 branch January 25, 2021 17:33

fmotrifork mentioned this pull request Feb 4, 2021

aks-engine 0.60.0 fishworks/fish-food#1203

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create kms key as part of cluster bootstrap #4170

feat: create kms key as part of cluster bootstrap #4170

aramase commented Jan 15, 2021 •

edited

codecov bot commented Jan 15, 2021 •

edited

aramase commented Jan 16, 2021

jackfrancis Jan 19, 2021

aramase Jan 19, 2021

jackfrancis left a comment

ritazh Jan 20, 2021

jackfrancis Jan 20, 2021

ritazh Jan 25, 2021

aramase commented Jan 20, 2021

ritazh Jan 20, 2021 •

edited

aramase Jan 20, 2021

ritazh Jan 25, 2021

aramase commented Jan 21, 2021

acs-bot commented Jan 25, 2021

feat: create kms key as part of cluster bootstrap #4170

feat: create kms key as part of cluster bootstrap #4170

Conversation

aramase commented Jan 15, 2021 • edited

codecov bot commented Jan 15, 2021 • edited

Codecov Report

aramase commented Jan 16, 2021

jackfrancis Jan 19, 2021

Choose a reason for hiding this comment

aramase Jan 19, 2021

Choose a reason for hiding this comment

jackfrancis left a comment

Choose a reason for hiding this comment

ritazh Jan 20, 2021

Choose a reason for hiding this comment

jackfrancis Jan 20, 2021

Choose a reason for hiding this comment

ritazh Jan 25, 2021

Choose a reason for hiding this comment

aramase commented Jan 20, 2021

ritazh Jan 20, 2021 • edited

Choose a reason for hiding this comment

aramase Jan 20, 2021

Choose a reason for hiding this comment

ritazh Jan 25, 2021

Choose a reason for hiding this comment

aramase commented Jan 21, 2021

acs-bot commented Jan 25, 2021

aramase commented Jan 15, 2021 •

edited

codecov bot commented Jan 15, 2021 •

edited

ritazh Jan 20, 2021 •

edited