fixed conflict resolution behavior while apply podpresets #47864

droot · 2017-06-21T18:57:24Z

What this PR does / why we need it:
This fixes the PodPreset application behavior in case of conflicts occur during the merging of Pod's information with PodPreset's. More details are in issue #47861

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

Special notes for your reviewer:
We are splitting the PodPreset application logic in two phases. In first phase, we try to detect the conflicts in information merging without modifying the Pod at all. If conflict occurs, then we reject the PodPresets injection. Incase of no conflicts, we apply the PodPresets and merge the information.

Release note:

PodPreset is not injected if conflict occurs while applying PodPresets to a Pod.

k8s-ci-robot · 2017-06-21T18:57:32Z

Hi @droot. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

cblecker · 2017-06-23T05:33:11Z

hack/local-up-cluster.sh

@@ -27,6 +27,8 @@ ALLOW_SECURITY_CONTEXT=${ALLOW_SECURITY_CONTEXT:-""}
 PSP_ADMISSION=${PSP_ADMISSION:-""}
 NODE_ADMISSION=${NODE_ADMISSION:-""}
 RUNTIME_CONFIG=${RUNTIME_CONFIG:-""}
+# RUNTIME_CONFIG=${RUNTIME_CONFIG:-"settingsio/v1alpha1=true,settings.k8s.io/v1alpha1/podpreset=true"}
+RUNTIME_CONFIG=${RUNTIME_CONFIG:-"api/all=true"}


What's your intended goal for "RUNTIME_CONFIG"? You're attempting to declare it on both lines 29 and 31.

This file got checked in by mistake. I have cleaned it up and enabled "api/all" by default. PTAL.

@droot I don't see any changes to this file.

droot · 2017-06-26T17:53:15Z

/assign @pmorie @jpeeler

k8s-ci-robot · 2017-06-26T17:53:17Z

@droot: GitHub didn't allow me to assign the following users: jpeeler.

Note that only kubernetes members can be assigned.

In response to this:

/assign @pmorie @jpeeler

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

droot · 2017-06-26T17:53:38Z

/assign @jpeeler

k8s-ci-robot · 2017-06-26T17:53:38Z

@droot: GitHub didn't allow me to assign the following users: jpeeler.

Note that only kubernetes members can be assigned.

In response to this:

/assign @jpeeler

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pmorie · 2017-06-26T18:39:19Z

hack/local-up-cluster.sh

@@ -26,7 +26,7 @@ ALLOW_PRIVILEGED=${ALLOW_PRIVILEGED:-""}
 ALLOW_SECURITY_CONTEXT=${ALLOW_SECURITY_CONTEXT:-""}
 PSP_ADMISSION=${PSP_ADMISSION:-""}
 NODE_ADMISSION=${NODE_ADMISSION:-""}
-RUNTIME_CONFIG=${RUNTIME_CONFIG:-""}
+RUNTIME_CONFIG=${RUNTIME_CONFIG:-"api/all=true"}


Are these debugging changes?

Took it out. It is not needed. I thought it was needed to enable "settings..../v1alpha1" API endpoint, but I was wrong, so took it out. PTAL.

cblecker · 2017-06-27T17:04:34Z

/unassign

pmorie · 2017-06-28T02:44:18Z

hack/local-up-cluster.sh

@@ -215,7 +215,7 @@ RKT_PATH=${RKT_PATH:-""}
 RKT_STAGE1_IMAGE=${RKT_STAGE1_IMAGE:-""}
 CHAOS_CHANCE=${CHAOS_CHANCE:-0.0}
 CPU_CFS_QUOTA=${CPU_CFS_QUOTA:-true}
-ENABLE_HOSTPATH_PROVISIONER=${ENABLE_HOSTPATH_PROVISIONER:-"false"}
+ENABLE_HOSTPATH_PROVISIONER=${ENABLE_HOSTPATH_PROVISIONER:-"true"}


This is also a debugging change, right? Def out of scpe for this PR

Good catch. Not scoped for this PR, took it out.

pmorie · 2017-06-28T02:46:29Z

plugin/pkg/admission/podpreset/admission.go

-		pod.ObjectMeta.Annotations[fmt.Sprintf("%s/podpreset-%s", annotationPrefix, pip.GetName())] = pip.GetResourceVersion()
+func safeToApplyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) error {
+	var errs []error
+	if _, err := MergeEnv(ctr.Env, podPresets); err != nil {


Seems like mergeEnv is probably unintentionally exported; we should make it private unless it's used elsewhere

Good pt. Made it private.

pmorie · 2017-06-28T02:46:46Z

plugin/pkg/admission/podpreset/admission.go

-	// if there were no original envvar just return the pip envvar
-	if original == nil {
-		return pip.Spec.Env, nil
+func MergeEnv(envVars []api.EnvVar, podPresets []*settings.PodPreset) ([]api.EnvVar, error) {


Hrm, maybe it is intentional?

Don't think so. Made it private.

pmorie · 2017-06-28T02:49:36Z

plugin/pkg/admission/podpreset/admission.go

+
+	var mergedEnvFrom []api.EnvFromSource
+
+	mergedEnvFrom = append(mergedEnvFrom, envSources...)


Is there a reason you chose a different form of allocate and copy here vs mergeenv?

I followed the original code and didn't change it. I am trying to dig more in to why EnvFrom are treated differently. Quick look suggests, they should also be handled in the similar ways as Env, but I need to confirm that.

EDIT: Took a look at EnvFrom. So EnvFrom is used by envVars to source the value from one of Sources (ConfigMap, Secret, Pod Resource or Container Resource basically downward API). So it is list of those objects which are referred by EnvVars. So there is not question of conflict here, so these lists just need to be merged. However, we can have duplicates in the list referring to the same source, but is harmless.

pmorie · 2017-06-28T02:54:38Z

plugin/pkg/admission/podpreset/admission.go

+
+			// make sure they are identical or throw an error
+			if !reflect.DeepEqual(found, v) {
+				errs = append(errs, fmt.Errorf("merging volume mounts for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.Name, v, found))


It might be helpful if this identified the 'for' as a podpreset

Didn't understand it. Can you explain it.

pmorie · 2017-06-28T02:55:46Z

plugin/pkg/admission/podpreset/admission.go

+				continue
+			}
+
+			// make sure they are identical or throw an error


I'm wondering if identical but duplicated mounts is an error - use case?

hmm... good point. Have to think more about it. Made a note of it.

pmorie · 2017-06-28T02:57:54Z

plugin/pkg/admission/podpreset/admission.go

-	for _, v := range original {
-		orig[v.MountPath] = v
-	}
+	for _, pp := range podPresets {


It seems like we can do all of this in a single nested loop, since this is orthogonal to the above loop but covers the same space

Good suggestion, refactored it.

pmorie · 2017-06-28T02:58:34Z

plugin/pkg/admission/podpreset/admission.go

+				continue
+			}
+
+			// make sure they are identical or throw an error


Also not sure about duplicate but equal volumes.

Yeah.. thinking more about it.

pmorie · 2017-06-28T03:00:02Z

plugin/pkg/admission/podpreset/admission.go

+	}
+}
+
+func applyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) {


I wiuld add a comment explaining that it shiuld be safe to ignore these errors

droot

Thanks @pmorie for the review. Have addressed all comments except one related to handling merge of EnvFrom. Will be investigating more about it today. PTAL.

droot · 2017-06-28T21:29:44Z

hack/local-up-cluster.sh

@@ -215,7 +215,7 @@ RKT_PATH=${RKT_PATH:-""}
 RKT_STAGE1_IMAGE=${RKT_STAGE1_IMAGE:-""}
 CHAOS_CHANCE=${CHAOS_CHANCE:-0.0}
 CPU_CFS_QUOTA=${CPU_CFS_QUOTA:-true}
-ENABLE_HOSTPATH_PROVISIONER=${ENABLE_HOSTPATH_PROVISIONER:-"false"}
+ENABLE_HOSTPATH_PROVISIONER=${ENABLE_HOSTPATH_PROVISIONER:-"true"}


Good catch. Not scoped for this PR, took it out.

droot · 2017-06-28T21:31:57Z

hack/local-up-cluster.sh

@@ -403,7 +403,7 @@ function start_apiserver {
    fi

    # Admission Controllers to invoke prior to persisting objects in cluster
-    ADMISSION_CONTROL=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount${security_admission},ResourceQuota,DefaultStorageClass,DefaultTolerationSeconds
+    ADMISSION_CONTROL=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount${security_admission},ResourceQuota,DefaultStorageClass,DefaultTolerationSeconds,PodPreset


@pmorie Changed the order here. Moved PodPreset before the Pod Security Policy plugin because we want PSP to evaluate the modified PodSpec instead of original PodSpec.

droot · 2017-06-28T21:34:24Z

plugin/pkg/admission/podpreset/admission.go

-		pod.ObjectMeta.Annotations[fmt.Sprintf("%s/podpreset-%s", annotationPrefix, pip.GetName())] = pip.GetResourceVersion()
+func safeToApplyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) error {
+	var errs []error
+	if _, err := MergeEnv(ctr.Env, podPresets); err != nil {


Good pt. Made it private.

droot · 2017-06-28T21:45:05Z

plugin/pkg/admission/podpreset/admission.go

-	// if there were no original envvar just return the pip envvar
-	if original == nil {
-		return pip.Spec.Env, nil
+func MergeEnv(envVars []api.EnvVar, podPresets []*settings.PodPreset) ([]api.EnvVar, error) {


Don't think so. Made it private.

droot · 2017-06-28T21:55:22Z

plugin/pkg/admission/podpreset/admission.go

-	for _, v := range original {
-		orig[v.MountPath] = v
-	}
+	for _, pp := range podPresets {


Good suggestion, refactored it.

droot · 2017-06-28T21:56:38Z

plugin/pkg/admission/podpreset/admission.go

+				continue
+			}
+
+			// make sure they are identical or throw an error


hmm... good point. Have to think more about it. Made a note of it.

droot · 2017-06-28T21:58:53Z

plugin/pkg/admission/podpreset/admission.go

+
+			// make sure they are identical or throw an error
+			if !reflect.DeepEqual(found, v) {
+				errs = append(errs, fmt.Errorf("merging volume mounts for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.Name, v, found))


Didn't understand it. Can you explain it.

droot · 2017-06-28T21:59:21Z

plugin/pkg/admission/podpreset/admission.go

+				continue
+			}
+
+			// make sure they are identical or throw an error


Yeah.. thinking more about it.

droot · 2017-06-28T22:02:36Z

plugin/pkg/admission/podpreset/admission.go

+	}
+}
+
+func applyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) {


droot · 2017-06-29T16:52:53Z

plugin/pkg/admission/podpreset/admission.go

+
+	var mergedEnvFrom []api.EnvFromSource
+
+	mergedEnvFrom = append(mergedEnvFrom, envSources...)


I followed the original code and didn't change it. I am trying to dig more in to why EnvFrom are treated differently. Quick look suggests, they should also be handled in the similar ways as Env, but I need to confirm that.

EDIT: Took a look at EnvFrom. So EnvFrom is used by envVars to source the value from one of Sources (ConfigMap, Secret, Pod Resource or Container Resource basically downward API). So it is list of those objects which are referred by EnvVars. So there is not question of conflict here, so these lists just need to be merged. However, we can have duplicates in the list referring to the same source, but is harmless.

droot · 2017-06-29T22:35:21Z

@pmorie addressed all the comments. Also updated the comment about EnvFrom. PTAL. (Also, I am not able to see review comment threads, thats very strange).

pmorie · 2017-06-30T15:52:20Z

@k8s-bot ok to test

pmorie · 2017-06-30T15:52:38Z

/release-note

droot · 2017-06-30T22:16:50Z

/test pull-kubernetes-verify

droot · 2017-06-30T23:28:17Z

/test pull-kubernetes-node-e2e

droot · 2017-07-07T17:53:00Z

/assign @shiywang

pmorie

Just a couple nits; I also think godoc for each method would be fantastic to have.

pmorie · 2017-07-11T17:30:32Z

hack/local-up-cluster.sh

@@ -406,8 +406,7 @@ function start_apiserver {
    fi

    # Admission Controllers to invoke prior to persisting objects in cluster
-    ADMISSION_CONTROL=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount${security_admission},ResourceQuota,DefaultStorageClass,DefaultTolerationSeconds
-
+    ADMISSION_CONTROL=Initializers,NamespaceLifecycle,LimitRanger,PodPreset,ServiceAccount${security_admission},ResourceQuota,DefaultStorageClass,DefaultTolerationSeconds


If we were to do it again, I would say to make this change in another PR.

Makes sense. Took it out. Will make a separate PR for this.

pmorie · 2017-07-11T17:31:15Z

plugin/pkg/admission/podpreset/admission.go

-				if err != nil {
-					// add event to pod
-					c.addEvent(pod, pip, err.Error())
+	glog.Infof("applied podpresets successfully on Pod: %s ", pod.GetGenerateName())


Is this always going to yield the right name?

Changed it to pod.Name. Also improved the logging a bit by making it more informative (added applied podpreset names as well)

pod.Name or pod.GetName() is empty, so kept pod.GenerateName(). I guess each pod instance will get a generated name.

droot · 2017-07-11T18:55:15Z

@pmorie addressed the comments. Please take a look.

pmorie · 2017-07-14T18:42:28Z

@k8s-bot test this

pmorie · 2017-07-14T19:36:48Z

/test pull-kubernetes-kubemark-e2e-gce

pmorie · 2017-07-18T16:58:42Z

/lgtm

pwittrock · 2017-07-18T17:01:10Z

/approve

k8s-cherrypick-bot · 2017-07-18T17:04:41Z

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

derekwaynecarr · 2017-07-18T17:04:41Z

/approve

droot · 2017-07-21T20:27:34Z

rebased with the master, so will need LGTM.

pmorie · 2017-07-24T17:36:49Z

/lgtm

k8s-github-robot · 2017-07-24T17:37:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, droot, pmorie, pwittrock

Associated issue: 47861

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

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [pwittrock]
~~plugin/pkg/admission/OWNERS~~ [derekwaynecarr]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-24T20:52:32Z

Automatic merge from submit-queue (batch tested with PRs 49444, 47864, 48584, 49395, 49118)

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 21, 2017

k8s-github-robot assigned derekwaynecarr and cblecker Jun 21, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 21, 2017

cblecker reviewed Jun 23, 2017

View reviewed changes

k8s-ci-robot assigned pmorie Jun 26, 2017

droot force-pushed the podpreset-conflict-fix branch 3 times, most recently from a8ce205 to 78f7974 Compare June 26, 2017 18:11

pmorie reviewed Jun 26, 2017

View reviewed changes

droot force-pushed the podpreset-conflict-fix branch from 78f7974 to 51cd10a Compare June 26, 2017 23:39

k8s-ci-robot unassigned cblecker Jun 27, 2017

pmorie reviewed Jun 28, 2017

View reviewed changes

droot force-pushed the podpreset-conflict-fix branch from 51cd10a to 09bfe6f Compare June 29, 2017 16:56

droot commented Jun 29, 2017

View reviewed changes

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 Jun 30, 2017

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jun 30, 2017

droot force-pushed the podpreset-conflict-fix branch from 09bfe6f to 7e7c3e9 Compare June 30, 2017 22:07

k8s-ci-robot assigned shiywang Jul 7, 2017

pmorie reviewed Jul 11, 2017

View reviewed changes

droot force-pushed the podpreset-conflict-fix branch from 7e7c3e9 to 91f804d Compare July 11, 2017 18:53

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 18, 2017

pmorie added the cherrypick-candidate label Jul 18, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 18, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2017

fixed conflict resolution behavior while apply podpresets

4d5b96f

droot force-pushed the podpreset-conflict-fix branch from 91f804d to 4d5b96f Compare July 21, 2017 20:22

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 24, 2017

k8s-github-robot merged commit 633079e into kubernetes:master Jul 24, 2017


		var mergedEnvFrom []api.EnvFromSource

		mergedEnvFrom = append(mergedEnvFrom, envSources...)

fixed conflict resolution behavior while apply podpresets #47864

fixed conflict resolution behavior while apply podpresets #47864

Conversation

droot commented Jun 21, 2017 • edited Loading

k8s-ci-robot commented Jun 21, 2017

Choose a reason for hiding this comment

droot Jun 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot commented Jun 26, 2017

k8s-ci-robot commented Jun 26, 2017

droot commented Jun 26, 2017

k8s-ci-robot commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Jun 27, 2017

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

droot Jun 29, 2017 • edited Loading

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

droot 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

droot Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

droot commented Jun 29, 2017

pmorie commented Jun 30, 2017

pmorie commented Jun 30, 2017

droot commented Jun 30, 2017

droot commented Jun 30, 2017

droot commented Jul 7, 2017

pmorie 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

droot commented Jul 11, 2017

pmorie commented Jul 14, 2017

pmorie commented Jul 14, 2017

pmorie commented Jul 18, 2017

pwittrock commented Jul 18, 2017

k8s-cherrypick-bot commented Jul 18, 2017

derekwaynecarr commented Jul 18, 2017

droot commented Jul 21, 2017

pmorie commented Jul 24, 2017

k8s-github-robot commented Jul 24, 2017

k8s-github-robot commented Jul 24, 2017

droot commented Jun 21, 2017 •

edited

Loading

droot Jun 26, 2017 •

edited

Loading

droot Jun 29, 2017 •

edited

Loading

droot Jun 29, 2017 •

edited

Loading