lib/resourcemerge: change SecurityContext reconcile

to handle securityContext changes differently. Since d9f6718, if a securityContext is not explicitly specified in the manifest the resource's securityContext will remain unchanged and it will continue to use the securityContext setting of the currently running resource (if there is one). We're not sure of the exact reason the logic was originally developed in this manner but this change joins a series of similar previous tightenings, including 02bb9ba (lib/resourcemerge/core: Clear env and envFrom if unset in manifest, 2021-04-20, #549) and ca299b8 (lib/resourcemerge: remove ports which are no longer required, 2020-02-13, #322). Reconciliation has been changed such that the entire securityContext structure, or any sub field of it, will be cleared if not specified in the manifest. This change affects Deployments, Jobs, and DaemonSets. It affects the securityContext found in both a PodSpec and a Container. Since the functions setInt64Ptr and setBoolPtr have been changed the impact is wide affecting ServiceAccounts, the PodSpec fields ShareProcessNamespace and TerminationGracePeriodSeconds, and the Job fields ActiveDeadlineSeconds and ManualSelector. For example, prior to this change assume Deployment machine-api-operator is running on the cluster with the following: securityContext: runAsNonRoot: true runAsUser: 65534 and during an upgrade the Deployment machine-api-operator no longer specifies a securityContext. The resulting upgraded Deployment machine-api-operator will still have the original securityContext: securityContext: runAsNonRoot: true runAsUser: 65534 Similarly, there is no way to remove, or clear, a securityContext field such as runAsUser. You can only modify it. After this change the above scenario will correctly result in the Deployment machine-api-operator not specifying securityContext upon upgrade completion. The changes apply to both the SecurityContext within a Container and the PodSecurityContext within a PodSpec.
openshift · openshift-merge-robot · Jul 26, 2022 · Jul 25, 2022 · Jul 26, 2022 · 619baaee9bce1ade67f27527a7bf367f1015f548
commit 619baaee9bce1ade67f27527a7bf367f1015f548
diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go
@@ -18,7 +18,7 @@ func EnsureConfigMap(modified *bool, existing *corev1.ConfigMap, required corev1
 	mergeMap(modified, &existing.Data, required.Data)
 }
 
-// EnsureServiceAccount ensures that the existing mathces the required.
+// EnsureServiceAccount ensures that the existing matches the required.
 // modified is set to true when existing had to be updated with required.
 func EnsureServiceAccount(modified *bool, existing *corev1.ServiceAccount, required corev1.ServiceAccount) {
 	EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
@@ -466,12 +466,13 @@ func ensureVolumeSourceDefaults(required *corev1.VolumeSource) {
 }
 
 func ensureSecurityContextPtr(modified *bool, existing **corev1.SecurityContext, required *corev1.SecurityContext) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return
@@ -490,12 +491,13 @@ func ensureSecurityContext(modified *bool, existing *corev1.SecurityContext, req
 }
 
 func ensureCapabilitiesPtr(modified *bool, existing **corev1.Capabilities, required *corev1.Capabilities) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return
@@ -619,12 +621,13 @@ func ensureAffinity(modified *bool, existing *corev1.Affinity, required corev1.A
 }
 
 func ensurePodSecurityContextPtr(modified *bool, existing **corev1.PodSecurityContext, required *corev1.PodSecurityContext) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return
@@ -676,12 +679,13 @@ func ensurePodSecurityContext(modified *bool, existing *corev1.PodSecurityContex
 }
 
 func ensureSELinuxOptionsPtr(modified *bool, existing **corev1.SELinuxOptions, required *corev1.SELinuxOptions) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return
@@ -734,12 +738,13 @@ func setBool(modified *bool, existing *bool, required bool) {
 }
 
 func setBoolPtr(modified *bool, existing **bool, required *bool) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return
@@ -758,6 +763,9 @@ func setInt32Ptr(modified *bool, existing **int32, required *int32) {
 	if *existing == nil && required == nil {
 		return
 	}
+
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
 	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
@@ -774,12 +782,13 @@ func setInt64(modified *bool, existing *int64, required int64) {
 }
 
 func setInt64Ptr(modified *bool, existing **int64, required *int64) {
-	// if we have no required, then we don't care what someone else has set
-	if required == nil {
+	if *existing == nil && required == nil {
 		return
 	}
 
-	if *existing == nil {
+	// Check if we can simply set to required. This can be done if existing is not set or it is set
+	// but required is not set.
+	if *existing == nil || (required == nil && *existing != nil) {
 		*modified = true
 		*existing = required
 		return

diff --git a/lib/resourcemerge/core_test.go b/lib/resourcemerge/core_test.go
@@ -30,11 +30,12 @@ func TestEnsurePodSpec(t *testing.T) {
 			expected:         corev1.PodSpec{},
 		},
 		{
-			name: "remove regular containers from existing",
+			name: "remove regular containers from existing, PodSecurityContext none",
 			existing: corev1.PodSpec{
 				Containers: []corev1.Container{
 					{Name: "test"},
-					{Name: "to-be-removed"}}},
+					{Name: "to-be-removed"}},
+				SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false)}},
 			input: corev1.PodSpec{
 				Containers: []corev1.Container{
 					{Name: "test"}}},
@@ -44,6 +45,71 @@ func TestEnsurePodSpec(t *testing.T) {
 				Containers: []corev1.Container{
 					{Name: "test"}}},
 		},
+		{
+			name: "PodSecurityContext empty",
+			existing: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
+					RunAsUser:      int64Ptr(int64(1234)),
+					RunAsGroup:     int64Ptr(int64(1234)),
+					FSGroup:        int64Ptr(int64(1234)),
+					SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
+				}},
+			input: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{}},
+
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{}},
+		},
+		{
+			name: "PodSecurityContext changes",
+			existing: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(true),
+					RunAsUser:  int64Ptr(int64(1234)),
+					RunAsGroup: int64Ptr(int64(1234))}},
+			input: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
+					RunAsGroup: int64Ptr(int64(5))}},
+
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				SecurityContext: &corev1.PodSecurityContext{RunAsNonRoot: boolPtr(false),
+					RunAsGroup: int64Ptr(int64(5))}},
+		},
+		{
+			name: "container SecurityContext none",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{SecurityContext: &corev1.SecurityContext{RunAsNonRoot: boolPtr(false),
+						RunAsUser: int64Ptr(int64(1234)),
+						Capabilities: &corev1.Capabilities{
+							Add: []corev1.Capability{"bar"}},
+						SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
+					}}}},
+			input: corev1.PodSpec{},
+
+			expectedModified: true,
+			expected:         corev1.PodSpec{},
+		},
+		{
+			name: "container SecurityContext empty",
+			existing: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{SecurityContext: &corev1.SecurityContext{RunAsNonRoot: boolPtr(false),
+						RunAsUser: int64Ptr(int64(1234)),
+						Capabilities: &corev1.Capabilities{
+							Add: []corev1.Capability{"bar"}},
+						SELinuxOptions: &corev1.SELinuxOptions{User: "foo"},
+					}}}},
+			input: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{SecurityContext: &corev1.SecurityContext{}}}},
+
+			expectedModified: true,
+			expected: corev1.PodSpec{
+				Containers: []corev1.Container{
+					{SecurityContext: &corev1.SecurityContext{}}}},
+		},
 		{
 			name: "remove regular and init containers from existing",
 			existing: corev1.PodSpec{
@@ -1559,3 +1625,11 @@ func defaultPodSpec(in *corev1.PodSpec, from corev1.PodSpec) {
 	modified := pointer.BoolPtr(false)
 	ensurePodSpec(modified, in, from)
 }
+
+func boolPtr(b bool) *bool {
+	return &b
+}
+
+func int64Ptr(i int64) *int64 {
+	return &i
+}
diff --git a/lib/resourcemerge/meta_test.go b/lib/resourcemerge/meta_test.go
@@ -14,6 +14,8 @@ import (
 
 func init() {
 	klog.InitFlags(flag.CommandLine)
+	_ = flag.CommandLine.Lookup("v").Value.Set("2")
+	_ = flag.CommandLine.Lookup("alsologtostderr").Value.Set("true")
 }
 
 func TestMergeOwnerRefs(t *testing.T) {