Enable configure-helper.sh to support two scenarios for etcd level encryption: decryption and adding encryption to existing clusters. #68379

immutableT · 2018-09-06T22:16:27Z

What this PR does / why we need it:

Enables two additional scenarios for etcd level encryption:
Adding encryption to an existing cluster
Decryption
Both of the above mentioned scenarios require that secrets be "touched" by the envelope transformer post cluster startup. This functionality is implemented in the apply-encryption-config function.
Refactor all encryption related logic into a single function - setup-etcd-encryption
Remove the responsibility of generating kms-plugin manifest from configure-helper.sh. Implementers of kms-plugins (cloud-providers) should rely on the up-stream components to generate kms-plugin manifest.

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

Special notes for your reviewer:

Release note:

NONE

immutableT · 2018-09-06T22:17:08Z

/assign @mikedanese

BenTheElder · 2018-09-08T18:17:37Z

/ok-to-test

immutableT · 2018-09-10T19:13:07Z

/assign @awly

immutableT · 2018-09-10T20:07:40Z

/test pull-kubernetes-verify

awly

It's a lot of bash and sed,. Just to make sure have you successfully started a cluster with these changes?

awly · 2018-09-10T20:30:44Z

cluster/gce/gci/apiserver_manifest_test.go

@@ -51,89 +55,138 @@ readonly DOCKER_REGISTRY="k8s.gcr.io"
 readonly ENABLE_LEGACY_ABAC=false
 readonly ETC_MANIFESTS=${KUBE_HOME}/etc/kubernetes/manifests
 readonly KUBE_API_SERVER_DOCKER_TAG=v1.11.0-alpha.0.1808_3c7452dc11645d-dirty
-readonly LOG_OWNER_USER=$(id -un)
+readonly LOG_OWNER_USER=$(whoami)


What's this change for?

awly · 2018-09-10T20:32:18Z

cluster/gce/gci/apiserver_manifest_test.go

-	kmsPluginManifestFileName     = "kms-plugin-container.manifest"
-	kubeAPIServerStartFuncName    = "start-kube-apiserver"
+	/*
+		   encryptionProviderConfigForKMS is base64 encoded yaml below


Could you base64-encode it at runtime? Makes it easier to edit.
The YAML itself can still be a string

awly · 2018-09-10T20:32:57Z

cluster/gce/gci/apiserver_manifest_test.go

 }

 func newKubeAPIServerManifestTestCase(t *testing.T) *kubeAPIServerManifestTestCase {
 	return &kubeAPIServerManifestTestCase{
-		ManifestTestCase: newManifestTestCase(t, kubeAPIServerManifestFileName, kubeAPIServerStartFuncName, []string{kmsPluginManifestFileName}),
+		ManifestTestCase: newManifestTestCase(t, kubeAPIServerManifestFileName, kubeAPIServerStartFuncName, []string{}),


nil instead of []string{}

awly · 2018-09-10T20:35:10Z

cluster/gce/gci/apiserver_manifest_test.go

+			flag := fmt.Sprintf("%s=%s", encryptionConfigFlag, e.EncryptionProviderConfigPath)
+
+			switch {
+			case tc.wantFlag && !flagIsInArg:


case tc.wantFlag != flagIsInArg:

This would save one case block, but will prevent me from providing a more informative error message.

awly · 2018-09-10T20:35:39Z

cluster/gce/gci/apiserver_manifest_test.go

+			case !tc.wantFlag && flagIsInArg:
+				t.Fatalf("Got %q,\n do not want flags to contain %q", execArgs, encryptionConfigFlag)
+			case tc.wantFlag && flagIsInArg:
+				if !strings.Contains(execArgs, flag) {


Move this check into the case condition above

awly · 2018-09-10T20:39:48Z

cluster/gce/gci/configure-helper.sh

+# Assumes vars (supplied via kube-env):
+# ENCRYPTION_PROVIDER_CONFIG
+# CLOUD_KMS_INTEGRATION
+


remove newline

awly · 2018-09-10T20:40:56Z

cluster/gce/gci/configure-helper.sh

-    # kms_socket_mnt is used by both kms_plugin and kube-apiserver - this is how these containers talk.
-    local kms_socket_mnt="{ \"name\": \"kmssocket\", \"mountPath\": \"${kms_socket_dir}\", \"readOnly\": false}"
+    local -n kube_api_server_params=$2
+    local encryption_provider_config_path=${ENCRYPTION_PROVIDER_CONFIG_PATH:-/etc/srv/kubernetes/encryption-provider-config.yml}


Mention ENCRYPTION_PROVIDER_CONFIG_PATH in the function comment above

awly · 2018-09-10T20:45:29Z

cluster/gce/gci/configure-helper.sh

+# Assumes vars (supplied via kube-env):
+# ENCRYPTION_PROVIDER_CONFIG_FORCE
+function apply-encryption-config() {
+    if [[ -z "${ENCRYPTION_PROVIDER_CONFIG_FORCE:-}" ]] || [[ "${ENCRYPTION_PROVIDER_CONFIG_FORCE:-}" == "false" ]]; then


if [[ "${ENCRYPTION_PROVIDER_CONFIG_FORCE:-false}" == "false" ]];

awly · 2018-09-10T20:47:21Z

cluster/gce/gci/configure-helper.sh

-    } " "${src_file}"
-  fi
+    # need kube-apiserver to be ready
+    until kubectl get secret


Hmm, what other startup steps are blocked by this? I wonder if this needs a timeout or maybe needs to run as the very last step of configure-helper

If this block of code fails then we can safely assume that the cluster did not come-up, so I don't see much value in time-out.
I put this function into background to allow other things to continue.

Hmm, when the parent shell exits (when rest of configure-helper is done) will it continue running?
And will there be a way to tell if/when it fails?

I believe that the parent will not exit until that function is done.
Today, this is a weak spot - cluster creation times-out and users need to login to the master to investigate. In other words, the caller would not know why kube-up or cluster create fails.

BenTheElder · 2018-09-10T21:08:02Z

It's a lot of bash and sed,. Just to make sure have you successfully started a cluster with these changes?

I'd love if you took a look at these with shellcheck, working on a PR to make this a presubmit currently.

immutableT · 2018-09-10T21:56:56Z

@awly PTAL.

immutableT · 2018-09-10T22:13:47Z

@BenTheElder ran shellcheck and addressed issues it found in my changes.

awly · 2018-09-10T22:21:15Z

/lgtm

BenTheElder · 2018-09-10T22:52:26Z

Thanks!!

immutableT · 2018-09-10T23:25:07Z

squashed

mikedanese · 2018-09-25T16:59:14Z

cluster/gce/gci/configure-helper.sh

-    params+=" --experimental-encryption-provider-config=${ENCRYPTION_PROVIDER_CONFIG_PATH}"
-  fi
+  # params is passed by reference, so no "$"
+  setup-etcd-encryption "${src_file}" params


I'm confused. Isn't this params passed as a string?

The idea is to allow setup-etcd-encryption function to modify params and the results of such modifications to be visible in start-kube-apiserver function. If I were to pass params as ${params} setup-etcd-encryption will get a copy of params.

mikedanese · 2018-09-25T16:59:30Z

cluster/gce/gci/configure-helper.sh

-EOM
-)
-  fi
+  src_file="${src_dir}/kube-apiserver.manifest"


make local?

It is used in other places (ex. start-etcd-empty-dir-clean-up-pod).

start-etcd-empty-dir-clean-up-pod declares src_file as local. Shouldn't we do the same here?

mikedanese · 2018-09-27T16:03:25Z

cluster/gce/gci/configure-helper.sh

-    # kms_socket_mnt is used by both kms_plugin and kube-apiserver - this is how these containers talk.
-    local kms_socket_mnt="{ \"name\": \"kmssocket\", \"mountPath\": \"${kms_socket_dir}\", \"readOnly\": false}"
+    kube_api_server_params=$2
+    encryption_provider_config_path=${ENCRYPTION_PROVIDER_CONFIG_PATH:-/etc/srv/kubernetes/encryption-provider-config.yml}


quote bash variable

mikedanese · 2018-09-27T16:05:42Z

cluster/gce/gci/configure-helper.sh

+# CLOUD_KMS_INTEGRATION
+# ENCRYPTION_PROVIDER_CONFIG_PATH (will default to /etc/srv/kubernetes/encryption-provider-config.yml)
+function setup-etcd-encryption {
+    local kube_apiserver_template_path


File uses two spaces for indent

mikedanese · 2018-09-27T16:09:29Z

cluster/gce/gci/configure-helper.sh

-  cp "${src_file}" "${ETC_MANIFESTS:-/etc/kubernetes/manifests}"
+    # forces all secrets to be re-written to etcd, and in the process either encrypting or decrypting them
+    # https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/
+    kubectl get secrets --all-namespaces -o json | kubectl replace -f -


This seems like it will lose data if secrets are being modified while it runs.

immutableT · 2018-09-27T22:11:01Z

@mikedanese PTAL.

mikedanese · 2018-10-04T04:17:20Z

/retest

mikedanese · 2018-10-04T04:18:00Z

cluster/gce/gci/configure-helper.sh

+  # need kube-apiserver to be ready
+  until kubectl get secret
+  do
+      sleep ${ENCRYPTION_PROVIDER_CONFIG_FORCE_DELAY:-5}


fix indentation

mikedanese · 2018-10-04T04:18:09Z

cluster/gce/gci/configure-helper.sh

+# ENCRYPTION_PROVIDER_CONFIG_FORCE
+function apply-encryption-config() {
+  if [[ "${ENCRYPTION_PROVIDER_CONFIG_FORCE:-false}" == "false" ]]; then
+      return


fix indentation

mikedanese · 2018-10-04T04:19:18Z

cluster/gce/gci/configure-helper.sh

-  cp "${src_file}" "${ETC_MANIFESTS:-/etc/kubernetes/manifests}"
+  # need kube-apiserver to be ready
+  until kubectl get secret
+  do


use single line until

https://google.github.io/styleguide/shell.xml?showone=Loops#Loops

mikedanese · 2018-10-04T04:19:28Z

cluster/gce/gci/configure-helper.sh

+  # else updated the secret in the middle of our update).
+  # TODO: Retry only on errors caused by a conflict.
+  until (( retries == 0 ))
+  do


https://google.github.io/styleguide/shell.xml?showone=Loops#Loops

mikedanese · 2018-10-04T04:20:00Z

/lgtm
/hold

mikedanese · 2018-10-04T16:24:39Z

/lgtm

k8s-ci-robot · 2018-10-04T16:24:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awly, immutableT, mikedanese

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:

~~build/OWNERS~~ [mikedanese]
~~cluster/gce/OWNERS~~ [mikedanese]

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

immutableT · 2018-10-04T18:22:21Z

/test pull-kubernetes-integration
/test pull-kubernetes-e2e-kops-aws

immutableT · 2018-10-04T19:43:08Z

/test pull-kubernetes-e2e-kops-aws

mikedanese · 2018-10-04T21:51:59Z

/hold cancel

fejta-bot · 2018-10-05T01:28:59Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-10-05T03:56:01Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

immutableT · 2018-10-05T05:45:40Z

/test pull-kubernetes-e2e-gke

k8s-ci-robot requested review from MaciekPytel and spiffxp September 6, 2018 22:16

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 6, 2018

k8s-ci-robot assigned mikedanese Sep 6, 2018

immutableT force-pushed the kms-plugin-via-gke branch 2 times, most recently from 70bc500 to 6ee40cc Compare September 7, 2018 00:48

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 8, 2018

immutableT force-pushed the kms-plugin-via-gke branch from 6ee40cc to bfda078 Compare September 10, 2018 18:52

k8s-ci-robot assigned awly Sep 10, 2018

awly reviewed Sep 10, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2018

immutableT force-pushed the kms-plugin-via-gke branch from 5574218 to 34bc600 Compare September 10, 2018 23:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 10, 2018

mikedanese reviewed Sep 25, 2018

View reviewed changes

mikedanese reviewed Sep 27, 2018

View reviewed changes

immutableT force-pushed the kms-plugin-via-gke branch 2 times, most recently from 33628ec to cffa2b9 Compare September 27, 2018 22:08

immutableT force-pushed the kms-plugin-via-gke branch from cffa2b9 to ff89856 Compare September 28, 2018 06:46

mikedanese reviewed Oct 4, 2018

View reviewed changes

Run kms-plugin in its own POD.

4bc62be

immutableT force-pushed the kms-plugin-via-gke branch from e7d6909 to 4bc62be Compare October 4, 2018 16:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2018

k8s-ci-robot merged commit 5602ab7 into kubernetes:master Oct 5, 2018

Enable configure-helper.sh to support two scenarios for etcd level encryption: decryption and adding encryption to existing clusters. #68379

Enable configure-helper.sh to support two scenarios for etcd level encryption: decryption and adding encryption to existing clusters. #68379

Conversation

immutableT commented Sep 6, 2018

immutableT commented Sep 6, 2018

BenTheElder commented Sep 8, 2018

immutableT commented Sep 10, 2018

immutableT commented Sep 10, 2018

awly 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

BenTheElder commented Sep 10, 2018

immutableT commented Sep 10, 2018

immutableT commented Sep 10, 2018

awly commented Sep 10, 2018

BenTheElder commented Sep 10, 2018

immutableT commented Sep 10, 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

immutableT commented Sep 27, 2018

mikedanese commented Oct 4, 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

mikedanese commented Oct 4, 2018

mikedanese commented Oct 4, 2018

k8s-ci-robot commented Oct 4, 2018

immutableT commented Oct 4, 2018

immutableT commented Oct 4, 2018

mikedanese commented Oct 4, 2018

fejta-bot commented Oct 5, 2018

fejta-bot commented Oct 5, 2018

immutableT commented Oct 5, 2018