OCPBUGS-22198: Reconcile Volumes in SCCs

lib/resourceapply/security.go

@@ -4,13 +4,14 @@ import (
 	"context"
 
 	"github.com/google/go-cmp/cmp"
-	securityv1 "github.com/openshift/api/security/v1"
-	securityclientv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
-	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
+
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/klog/v2"
-	"k8s.io/utils/pointer"
+
+	securityv1 "github.com/openshift/api/security/v1"
+	securityclientv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
+	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
 )
 
 // ApplySecurityContextConstraintsv1 applies the required SecurityContextConstraints to the cluster.
@@ -29,16 +30,24 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien
 		return nil, false, nil
 	}
 
-	modified := pointer.BoolPtr(false)
-	resourcemerge.EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta)
-	if !*modified {
+	reconcile, requiredMerges := resourcemerge.EnsureSecurityContextConstraints(*existing, *required)
+
+	if reconciling && requiredMerges {
+		klog.Warningf("System SCC %s is modified: future OCP versions will stop tolerating modifying system SCCs (https://access.redhat.com/solutions/7033949)", required.Name)
+	}
+
+	if reconcile == nil {
 		return existing, false, nil
 	}
 
 	if reconciling {
-		klog.V(2).Infof("Updating SCC %s due to diff: %v", required.Name, cmp.Diff(existing, required))
+		if diff := cmp.Diff(existing, reconcile); diff != "" {
+			klog.V(2).Infof("Updating SCC %s/%s due to diff: %v", required.Namespace, required.Name, diff)
+		} else {
+			klog.V(2).Infof("Updating SCC %s/%s with empty diff: possible hotloop after wrong comparison", required.Namespace, required.Name)
+		}
 	}
 
-	actual, err := client.SecurityContextConstraints().Update(ctx, existing, metav1.UpdateOptions{})
+	actual, err := client.SecurityContextConstraints().Update(ctx, reconcile, metav1.UpdateOptions{})
 	return actual, true, err
 }

lib/resourceapply/security_test.go

@@ -0,0 +1,181 @@
+package resourceapply
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kubetesting "k8s.io/client-go/testing"
+	"k8s.io/utils/pointer"
+
+	securityv1 "github.com/openshift/api/security/v1"
+	securityfake "github.com/openshift/client-go/security/clientset/versioned/fake"
+)
+
+var restrictedv2 = securityv1.SecurityContextConstraints{
+	TypeMeta: metav1.TypeMeta{Kind: "SecurityContextConstraints", APIVersion: securityv1.GroupVersion.String()},
+	ObjectMeta: metav1.ObjectMeta{
+		Name: "restricted-v2",
+		Annotations: map[string]string{
+			"include.release.openshift.io/ibm-cloud-managed":              "true",
+			"include.release.openshift.io/self-managed-high-availability": "true",
+			"include.release.openshift.io/single-node-developer":          "true",
+			"kubernetes.io/description":                                   "restricted-v2 denies access...",
+		},
+	},
+	RequiredDropCapabilities: []corev1.Capability{"ALL"},
+	AllowedCapabilities:      []corev1.Capability{"NET_BIND_SERVICE"},
+	Volumes:                  []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "persistentVolumeClaim", "projected", "secret"},
+	SELinuxContext:           securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs},
+	RunAsUser:                securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsRange},
+	SupplementalGroups:       securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny},
+	FSGroup:                  securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs},
+	SeccompProfiles:          []string{"runtime/default"},
+}
+
+func defaulted(get func() *securityv1.SecurityContextConstraints) func() *securityv1.SecurityContextConstraints {
+	return func() *securityv1.SecurityContextConstraints {
+		scc := get()
+		if scc.AllowPrivilegeEscalation == nil {
+			scc.AllowPrivilegeEscalation = pointer.Bool(true)
+		}
+		return scc
+	}
+}
+
+func getSCCAction(name string) kubetesting.Action {
+	return kubetesting.NewGetAction(securityv1.GroupVersion.WithResource("securitycontextconstraints"), "", name)
+}
+
+func createSCCAction(scc *securityv1.SecurityContextConstraints) kubetesting.Action {
+	return kubetesting.NewCreateAction(securityv1.GroupVersion.WithResource("securitycontextconstraints"), "", scc)
+}
+
+func updateSCCAction(scc *securityv1.SecurityContextConstraints) kubetesting.Action {
+	return kubetesting.NewUpdateAction(securityv1.GroupVersion.WithResource("securitycontextconstraints"), "", scc)
+}
+
+func TestApplySecurityContextConstraintsv1(t *testing.T) {
+	tests := []struct {
+		name     string
+		existing func() *securityv1.SecurityContextConstraints
+		required func() *securityv1.SecurityContextConstraints
+
+		expected         func() *securityv1.SecurityContextConstraints
+		expectedAPICalls []kubetesting.Action
+		expectedModified bool
+		expectedErr      error
+	}{
+		{
+			name:     "create nonexistent, expect modified",
+			required: restrictedv2.DeepCopy,
+			expected: restrictedv2.DeepCopy,
+			expectedAPICalls: []kubetesting.Action{
+				getSCCAction("restricted-v2"),
+				createSCCAction(&restrictedv2),
+			},
+			expectedModified: true,
+		},
+		{
+			name:     "no modification when existing is equal to required",
+			existing: defaulted(restrictedv2.DeepCopy),
+			required: restrictedv2.DeepCopy,
+			expected: defaulted(restrictedv2.DeepCopy),
+			expectedAPICalls: []kubetesting.Action{
+				getSCCAction("restricted-v2"),
+			},
+			expectedModified: false,
+		},
+		{
+			name:     "no modified when existing differs but there's release.openshift.io/create-only: true annotation",
+			existing: restrictedv2.DeepCopy,
+			required: func() *securityv1.SecurityContextConstraints {
+				scc := restrictedv2.DeepCopy()
+				scc.Annotations = map[string]string{CreateOnlyAnnotation: "true"}
+				scc.Volumes = nil
+				scc.AllowHostDirVolumePlugin = true
+				return scc
+			},
+			expected: func() *securityv1.SecurityContextConstraints { return nil },
+			expectedAPICalls: []kubetesting.Action{
+				getSCCAction("restricted-v2"),
+			},
+			expectedModified: false,
+		},
+		{
+			name:     "enforce missing volumes from required, modified is true",
+			existing: defaulted(restrictedv2.DeepCopy),
+			required: func() *securityv1.SecurityContextConstraints {
+				scc := restrictedv2.DeepCopy()
+				scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "ephemeral", "persistentVolumeClaim", "projected", "secret"}
+				return scc
+			},
+			expected: defaulted(func() *securityv1.SecurityContextConstraints {
+				scc := restrictedv2.DeepCopy()
+				scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "persistentVolumeClaim", "projected", "secret", "ephemeral"}
+				return scc
+			}),
+			expectedAPICalls: []kubetesting.Action{
+				getSCCAction("restricted-v2"),
+				updateSCCAction(defaulted(func() *securityv1.SecurityContextConstraints {
+					scc := restrictedv2.DeepCopy()
+					scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "persistentVolumeClaim", "projected", "secret", "ephemeral"}
+					return scc
+				})()),
+			},
+			expectedModified: true,
+		},
+		{
+			name:     "keep additional volumes from existing because we want merge semantics, modified is false",
+			existing: defaulted(restrictedv2.DeepCopy),
+			required: func() *securityv1.SecurityContextConstraints {
+				scc := restrictedv2.DeepCopy()
+				scc.Volumes = []securityv1.FSType{"configMap"}
+				return scc
+			},
+			expected: defaulted(restrictedv2.DeepCopy),
+			expectedAPICalls: []kubetesting.Action{
+				getSCCAction("restricted-v2"),
+			},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			newClient := securityfake.NewSimpleClientset()
+			// We cannot simply initialize the client because initialization is broken for SCC
+			// https://github.com/openshift/client-go/issues/244
+			if tc.existing != nil {
+				if _, err := newClient.SecurityV1().SecurityContextConstraints().Create(context.Background(), tc.existing(), metav1.CreateOptions{}); err != nil {
+					t.Fatalf("failed to setup fake client with existing SCCs: %v", err)
+				}
+				newClient.ClearActions()
+			}
+			actual, modified, err := ApplySecurityContextConstraintsv1(context.Background(), newClient.SecurityV1(), tc.required(), true)
+			if tc.expectedErr != nil {
+				if err == nil {
+					t.Fatalf("expected error %v, got nil", tc.expectedErr)
+				}
+				if err.Error() != tc.expectedErr.Error() {
+					t.Fatalf("expected error %v, got %v", tc.expectedErr, err)
+				}
+			}
+			if tc.expectedErr == nil && err != nil {
+				t.Fatalf("expected no error, got %v", err)
+			}
+
+			if diff := cmp.Diff(tc.expectedAPICalls, newClient.Actions()); diff != "" {
+				t.Errorf("API calls differ from expected:\n%s", diff)
+			}
+
+			if tc.expectedModified != modified {
+				t.Errorf("expected modified %v, got %v", tc.expectedModified, modified)
+			}
+			if diff := cmp.Diff(tc.expected(), actual); diff != "" {
+				t.Errorf("SCC differs from expected:\n%s", diff)
+			}
+		})
+	}
+}

lib/resourcemerge/core.go

@@ -554,22 +554,6 @@ func setStringSlice(modified *bool, existing *[]string, required []string) {
 	}
 }
 
-func mergeStringSlice(modified *bool, existing *[]string, required []string) {
-	for _, required := range required {
-		found := false
-		for _, curr := range *existing {
-			if required == curr {
-				found = true
-				break
-			}
-		}
-		if !found {
-			*modified = true
-			*existing = append(*existing, required)
-		}
-	}
-}
-
 func ensureTolerations(modified *bool, existing *[]corev1.Toleration, required []corev1.Toleration) {
 	exists := struct{}{}
 	found := make(map[int]struct{}, len(required))