From 23a5d333dd2f0e3271440ec3f3b81230b40561f3 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Tue, 19 Sep 2023 17:58:28 +0200 Subject: [PATCH 1/7] resourcemerge: refactor EnsureSecurityContextConstraint Clearly separate input/output. Do not use side effects on structures passed as input, pass output only via return value. Cover both `EnsureSecurityContextConstraint` and `ApplySecurityContextConstraintv1` with tests, including the fact that `Apply...` does *not* call `Ensure...` as expected, which is a bug reported as OCPBUGS-18386. The new tests need additional files to be vendored, which is done in a separate followup commit. --- lib/resourceapply/security_test.go | 160 +++++++++++++++++++++++++ lib/resourcemerge/security.go | 100 +++++++++------- lib/resourcemerge/security_test.go | 186 +++++++++++++++++++++++++++++ 3 files changed, 400 insertions(+), 46 deletions(-) create mode 100644 lib/resourceapply/security_test.go create mode 100644 lib/resourcemerge/security_test.go diff --git a/lib/resourceapply/security_test.go b/lib/resourceapply/security_test.go new file mode 100644 index 000000000..1c3191c90 --- /dev/null +++ b/lib/resourceapply/security_test.go @@ -0,0 +1,160 @@ +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" + + 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 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 modified when existing is equal to required", + existing: restrictedv2.DeepCopy, + required: restrictedv2.DeepCopy, + expected: 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 DOES NOT WORK because OCPBUGS-18386, modified is false", + existing: restrictedv2.DeepCopy, + required: func() *securityv1.SecurityContextConstraints { + scc := restrictedv2.DeepCopy() + scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "ephemeral", "persistentVolumeClaim", "projected", "secret"} + return scc + }, + expected: restrictedv2.DeepCopy, + expectedAPICalls: []kubetesting.Action{ + getSCCAction("restricted-v2"), + }, + }, + { + name: "do not keep additional volumes from existing DOES NOT WORK because OCPBUGS-18386, modified is false", + existing: restrictedv2.DeepCopy, + required: func() *securityv1.SecurityContextConstraints { + scc := restrictedv2.DeepCopy() + scc.Volumes = []securityv1.FSType{"configMap"} + return scc + }, + expected: 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) + } + }) + } +} diff --git a/lib/resourcemerge/security.go b/lib/resourcemerge/security.go index 9ad63dddf..59ec7007b 100644 --- a/lib/resourcemerge/security.go +++ b/lib/resourcemerge/security.go @@ -5,104 +5,112 @@ import ( "k8s.io/apimachinery/pkg/api/equality" ) -// EnsureSecurityContextConstraints ensures that the existing matches the required. -// modified is set to true when existing had to be updated with required. -func EnsureSecurityContextConstraints(modified *bool, existing *securityv1.SecurityContextConstraints, required securityv1.SecurityContextConstraints) { - EnsureObjectMeta(modified, &existing.ObjectMeta, required.ObjectMeta) - setInt32Ptr(modified, &existing.Priority, required.Priority) - setBool(modified, &existing.AllowPrivilegedContainer, required.AllowPrivilegedContainer) +// EnsureSecurityContextConstraints ensures that the result matches the required. +// modified is set to true when result had to be updated with required. +func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstraints, required securityv1.SecurityContextConstraints) *securityv1.SecurityContextConstraints { + var modified bool + result := existing.DeepCopy() + + EnsureObjectMeta(&modified, &result.ObjectMeta, required.ObjectMeta) + setInt32Ptr(&modified, &result.Priority, required.Priority) + setBool(&modified, &result.AllowPrivilegedContainer, required.AllowPrivilegedContainer) for _, required := range required.DefaultAddCapabilities { found := false - for _, curr := range existing.DefaultAddCapabilities { + for _, curr := range result.DefaultAddCapabilities { if equality.Semantic.DeepEqual(required, curr) { found = true break } } if !found { - *modified = true - existing.DefaultAddCapabilities = append(existing.DefaultAddCapabilities, required) + modified = true + result.DefaultAddCapabilities = append(result.DefaultAddCapabilities, required) } } for _, required := range required.RequiredDropCapabilities { found := false - for _, curr := range existing.RequiredDropCapabilities { + for _, curr := range result.RequiredDropCapabilities { if equality.Semantic.DeepEqual(required, curr) { found = true break } } if !found { - *modified = true - existing.RequiredDropCapabilities = append(existing.RequiredDropCapabilities, required) + modified = true + result.RequiredDropCapabilities = append(result.RequiredDropCapabilities, required) } } for _, required := range required.AllowedCapabilities { found := false - for _, curr := range existing.AllowedCapabilities { + for _, curr := range result.AllowedCapabilities { if equality.Semantic.DeepEqual(required, curr) { found = true break } } if !found { - *modified = true - existing.AllowedCapabilities = append(existing.AllowedCapabilities, required) + modified = true + result.AllowedCapabilities = append(result.AllowedCapabilities, required) } } - setBool(modified, &existing.AllowHostDirVolumePlugin, required.AllowHostDirVolumePlugin) + setBool(&modified, &result.AllowHostDirVolumePlugin, required.AllowHostDirVolumePlugin) for _, required := range required.Volumes { found := false - for _, curr := range existing.Volumes { + for _, curr := range result.Volumes { if equality.Semantic.DeepEqual(required, curr) { found = true break } } if !found { - *modified = true - existing.Volumes = append(existing.Volumes, required) + modified = true + result.Volumes = append(result.Volumes, required) } } for _, required := range required.AllowedFlexVolumes { found := false - for _, curr := range existing.AllowedFlexVolumes { + for _, curr := range result.AllowedFlexVolumes { if equality.Semantic.DeepEqual(required.Driver, curr.Driver) { found = true break } } if !found { - *modified = true - existing.AllowedFlexVolumes = append(existing.AllowedFlexVolumes, required) + modified = true + result.AllowedFlexVolumes = append(result.AllowedFlexVolumes, required) } } - setBool(modified, &existing.AllowHostNetwork, required.AllowHostNetwork) - setBool(modified, &existing.AllowHostPorts, required.AllowHostPorts) - setBool(modified, &existing.AllowHostPID, required.AllowHostPID) - setBool(modified, &existing.AllowHostIPC, required.AllowHostIPC) - setBoolPtr(modified, &existing.DefaultAllowPrivilegeEscalation, required.DefaultAllowPrivilegeEscalation) - setBoolPtr(modified, &existing.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) - if !equality.Semantic.DeepEqual(existing.SELinuxContext, required.SELinuxContext) { - *modified = true - existing.SELinuxContext = required.SELinuxContext + setBool(&modified, &result.AllowHostNetwork, required.AllowHostNetwork) + setBool(&modified, &result.AllowHostPorts, required.AllowHostPorts) + setBool(&modified, &result.AllowHostPID, required.AllowHostPID) + setBool(&modified, &result.AllowHostIPC, required.AllowHostIPC) + setBoolPtr(&modified, &result.DefaultAllowPrivilegeEscalation, required.DefaultAllowPrivilegeEscalation) + setBoolPtr(&modified, &result.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) + if !equality.Semantic.DeepEqual(result.SELinuxContext, required.SELinuxContext) { + modified = true + result.SELinuxContext = required.SELinuxContext + } + if !equality.Semantic.DeepEqual(result.RunAsUser, required.RunAsUser) { + modified = true + result.RunAsUser = required.RunAsUser } - if !equality.Semantic.DeepEqual(existing.RunAsUser, required.RunAsUser) { - *modified = true - existing.RunAsUser = required.RunAsUser + if !equality.Semantic.DeepEqual(result.FSGroup, required.FSGroup) { + modified = true + result.FSGroup = required.FSGroup } - if !equality.Semantic.DeepEqual(existing.FSGroup, required.FSGroup) { - *modified = true - existing.FSGroup = required.FSGroup + if !equality.Semantic.DeepEqual(result.SupplementalGroups, required.SupplementalGroups) { + modified = true + result.SupplementalGroups = required.SupplementalGroups } - if !equality.Semantic.DeepEqual(existing.SupplementalGroups, required.SupplementalGroups) { - *modified = true - existing.SupplementalGroups = required.SupplementalGroups + setBool(&modified, &result.ReadOnlyRootFilesystem, required.ReadOnlyRootFilesystem) + mergeStringSlice(&modified, &result.Users, required.Users) + mergeStringSlice(&modified, &result.Groups, required.Groups) + mergeStringSlice(&modified, &result.SeccompProfiles, required.SeccompProfiles) + mergeStringSlice(&modified, &result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls) + mergeStringSlice(&modified, &result.ForbiddenSysctls, required.ForbiddenSysctls) + + if modified { + return result } - setBool(modified, &existing.ReadOnlyRootFilesystem, required.ReadOnlyRootFilesystem) - mergeStringSlice(modified, &existing.Users, required.Users) - mergeStringSlice(modified, &existing.Groups, required.Groups) - mergeStringSlice(modified, &existing.SeccompProfiles, required.SeccompProfiles) - mergeStringSlice(modified, &existing.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls) - mergeStringSlice(modified, &existing.ForbiddenSysctls, required.ForbiddenSysctls) + return nil } diff --git a/lib/resourcemerge/security_test.go b/lib/resourcemerge/security_test.go new file mode 100644 index 000000000..9402557e4 --- /dev/null +++ b/lib/resourcemerge/security_test.go @@ -0,0 +1,186 @@ +package resourcemerge + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + securityv1 "github.com/openshift/api/security/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" +) + +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 TestEnsureSecurityContextConstraints(t *testing.T) { + testCases := []struct { + name string + existing securityv1.SecurityContextConstraints + required securityv1.SecurityContextConstraints + + expected *securityv1.SecurityContextConstraints + }{ + { + name: "no modified when existing is equal to required", + existing: *restrictedv2.DeepCopy(), + required: *restrictedv2.DeepCopy(), + expected: nil, + }, + { + + name: "enforce primitive fields", + existing: securityv1.SecurityContextConstraints{ + Priority: pointer.Int32(123), + AllowPrivilegedContainer: true, + AllowHostDirVolumePlugin: false, + AllowHostNetwork: true, + AllowHostPorts: false, + AllowHostPID: true, + AllowHostIPC: false, + DefaultAllowPrivilegeEscalation: pointer.Bool(true), + AllowPrivilegeEscalation: pointer.Bool(false), + ReadOnlyRootFilesystem: true, + }, + required: securityv1.SecurityContextConstraints{ + Priority: pointer.Int32(42), + AllowPrivilegedContainer: false, + AllowHostDirVolumePlugin: true, + AllowHostNetwork: false, + AllowHostPorts: true, + AllowHostPID: false, + AllowHostIPC: true, + DefaultAllowPrivilegeEscalation: pointer.Bool(false), + AllowPrivilegeEscalation: pointer.Bool(true), + ReadOnlyRootFilesystem: false, + }, + expected: &securityv1.SecurityContextConstraints{ + Priority: pointer.Int32(42), + AllowPrivilegedContainer: false, + AllowHostDirVolumePlugin: true, + AllowHostNetwork: false, + AllowHostPorts: true, + AllowHostPID: false, + AllowHostIPC: true, + DefaultAllowPrivilegeEscalation: pointer.Bool(false), + AllowPrivilegeEscalation: pointer.Bool(true), + ReadOnlyRootFilesystem: false, + }, + }, + { + name: "enforce capabilities", + existing: securityv1.SecurityContextConstraints{ + DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B"}, + RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D"}, + AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F"}, + }, + required: securityv1.SecurityContextConstraints{ + DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "REQUIRED_A", "REQUIRED_B"}, + RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "REQUIRED_C", "REQUIRED_D"}, + AllowedCapabilities: []corev1.Capability{"SHARED_C", "REQUIRED_E", "REQUIRED_F"}, + }, + expected: &securityv1.SecurityContextConstraints{ + DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B", "REQUIRED_A", "REQUIRED_B"}, + RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D", "REQUIRED_C", "REQUIRED_D"}, + AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F", "REQUIRED_E", "REQUIRED_F"}, + }, + }, + { + name: "enforce volumes", + existing: securityv1.SecurityContextConstraints{ + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret}, + AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}}, + }, + required: securityv1.SecurityContextConstraints{ + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, + AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "required-1"}, {Driver: "required-2"}}, + }, + expected: &securityv1.SecurityContextConstraints{ + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, + AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}, {Driver: "required-1"}, {Driver: "required-2"}}, + }, + }, + { + name: "enforce strategy options", + existing: securityv1.SecurityContextConstraints{ + SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs, SELinuxOptions: &corev1.SELinuxOptions{User: "ooser"}}, + RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyRunAsAny}, + SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyMustRunAs}, + FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyRunAsAny}, + }, + required: securityv1.SecurityContextConstraints{ + SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, + RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, + SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, + FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, + }, + expected: &securityv1.SecurityContextConstraints{ + SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, + RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, + SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, + FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, + }, + }, + { + name: "enforce users, groups, seccomp profiles, sysctls", + existing: securityv1.SecurityContextConstraints{ + Users: []string{"shared-u", "existing-user-1", "existing-user-2"}, + Groups: []string{"shared-g", "existing-group-1", "existing-group-2"}, + SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2"}, + AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2"}, + ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2"}, + }, + required: securityv1.SecurityContextConstraints{ + Users: []string{"shared-u", "required-user-1", "required-user-2"}, + Groups: []string{"shared-g", "required-group-1", "required-group-2"}, + SeccompProfiles: []string{"shared-s", "required-seccomp-1", "required-seccomp-2"}, + AllowedUnsafeSysctls: []string{"shared-a", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, + ForbiddenSysctls: []string{"shared-f", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, + }, + expected: &securityv1.SecurityContextConstraints{ + Users: []string{"shared-u", "existing-user-1", "existing-user-2", "required-user-1", "required-user-2"}, + Groups: []string{"shared-g", "existing-group-1", "existing-group-2", "required-group-1", "required-group-2"}, + SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2", "required-seccomp-1", "required-seccomp-2"}, + AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, + ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := EnsureSecurityContextConstraints(tc.existing, tc.required) + + origExisting, origRequired := tc.existing.DeepCopy(), tc.required.DeepCopy() + + if diff := cmp.Diff(tc.expected, result); diff != "" { + t.Errorf("SCC differs from expected:\n%s", diff) + } + + if diff := cmp.Diff(origExisting, &tc.existing); diff != "" { + t.Errorf("Unexpected side effect on existing state:\n%s", diff) + } + if diff := cmp.Diff(origRequired, &tc.required); diff != "" { + t.Errorf("Unexpected side effect on required state:\n%s", diff) + } + }) + } +} From 460caaf16801a66bdf9ab55718cb8372d73ee4fa Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 20 Oct 2023 15:40:45 +0200 Subject: [PATCH 2/7] `go mod vendor`: vendor more o/client-go files needed in tests --- .../security/clientset/versioned/clientset.go | 105 ++++++++++++++ .../security/clientset/versioned/doc.go | 4 + .../versioned/fake/clientset_generated.go | 69 ++++++++++ .../security/clientset/versioned/fake/doc.go | 4 + .../clientset/versioned/fake/register.go | 40 ++++++ .../versioned/typed/security/v1/fake/doc.go | 4 + .../v1/fake/fake_podsecuritypolicyreview.go | 33 +++++ ...fake_podsecuritypolicyselfsubjectreview.go | 33 +++++ .../fake_podsecuritypolicysubjectreview.go | 33 +++++ .../security/v1/fake/fake_rangeallocation.go | 130 ++++++++++++++++++ .../security/v1/fake/fake_security_client.go | 40 ++++++ .../fake/fake_securitycontextconstraints.go | 130 ++++++++++++++++++ vendor/modules.txt | 3 + 13 files changed, 628 insertions(+) create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/clientset.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/doc.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/fake/clientset_generated.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/fake/doc.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/fake/register.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/doc.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyreview.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyselfsubjectreview.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicysubjectreview.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_rangeallocation.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_security_client.go create mode 100644 vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_securitycontextconstraints.go diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/clientset.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/clientset.go new file mode 100644 index 000000000..f6890257e --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/clientset.go @@ -0,0 +1,105 @@ +// Code generated by client-gen. DO NOT EDIT. + +package versioned + +import ( + "fmt" + "net/http" + + securityv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1" + discovery "k8s.io/client-go/discovery" + rest "k8s.io/client-go/rest" + flowcontrol "k8s.io/client-go/util/flowcontrol" +) + +type Interface interface { + Discovery() discovery.DiscoveryInterface + SecurityV1() securityv1.SecurityV1Interface +} + +// Clientset contains the clients for groups. Each group has exactly one +// version included in a Clientset. +type Clientset struct { + *discovery.DiscoveryClient + securityV1 *securityv1.SecurityV1Client +} + +// SecurityV1 retrieves the SecurityV1Client +func (c *Clientset) SecurityV1() securityv1.SecurityV1Interface { + return c.securityV1 +} + +// Discovery retrieves the DiscoveryClient +func (c *Clientset) Discovery() discovery.DiscoveryInterface { + if c == nil { + return nil + } + return c.DiscoveryClient +} + +// NewForConfig creates a new Clientset for the given config. +// If config's RateLimiter is not set and QPS and Burst are acceptable, +// NewForConfig will generate a rate-limiter in configShallowCopy. +// NewForConfig is equivalent to NewForConfigAndClient(c, httpClient), +// where httpClient was generated with rest.HTTPClientFor(c). +func NewForConfig(c *rest.Config) (*Clientset, error) { + configShallowCopy := *c + + if configShallowCopy.UserAgent == "" { + configShallowCopy.UserAgent = rest.DefaultKubernetesUserAgent() + } + + // share the transport between all clients + httpClient, err := rest.HTTPClientFor(&configShallowCopy) + if err != nil { + return nil, err + } + + return NewForConfigAndClient(&configShallowCopy, httpClient) +} + +// NewForConfigAndClient creates a new Clientset for the given config and http client. +// Note the http client provided takes precedence over the configured transport values. +// If config's RateLimiter is not set and QPS and Burst are acceptable, +// NewForConfigAndClient will generate a rate-limiter in configShallowCopy. +func NewForConfigAndClient(c *rest.Config, httpClient *http.Client) (*Clientset, error) { + configShallowCopy := *c + if configShallowCopy.RateLimiter == nil && configShallowCopy.QPS > 0 { + if configShallowCopy.Burst <= 0 { + return nil, fmt.Errorf("burst is required to be greater than 0 when RateLimiter is not set and QPS is set to greater than 0") + } + configShallowCopy.RateLimiter = flowcontrol.NewTokenBucketRateLimiter(configShallowCopy.QPS, configShallowCopy.Burst) + } + + var cs Clientset + var err error + cs.securityV1, err = securityv1.NewForConfigAndClient(&configShallowCopy, httpClient) + if err != nil { + return nil, err + } + + cs.DiscoveryClient, err = discovery.NewDiscoveryClientForConfigAndClient(&configShallowCopy, httpClient) + if err != nil { + return nil, err + } + return &cs, nil +} + +// NewForConfigOrDie creates a new Clientset for the given config and +// panics if there is an error in the config. +func NewForConfigOrDie(c *rest.Config) *Clientset { + cs, err := NewForConfig(c) + if err != nil { + panic(err) + } + return cs +} + +// New creates a new Clientset for the given RESTClient. +func New(c rest.Interface) *Clientset { + var cs Clientset + cs.securityV1 = securityv1.New(c) + + cs.DiscoveryClient = discovery.NewDiscoveryClient(c) + return &cs +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/doc.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/doc.go new file mode 100644 index 000000000..0e0c2a890 --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/doc.go @@ -0,0 +1,4 @@ +// Code generated by client-gen. DO NOT EDIT. + +// This package has the automatically generated clientset. +package versioned diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/clientset_generated.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/clientset_generated.go new file mode 100644 index 000000000..9bcd5d024 --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/clientset_generated.go @@ -0,0 +1,69 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + clientset "github.com/openshift/client-go/security/clientset/versioned" + securityv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1" + fakesecurityv1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/watch" + "k8s.io/client-go/discovery" + fakediscovery "k8s.io/client-go/discovery/fake" + "k8s.io/client-go/testing" +) + +// NewSimpleClientset returns a clientset that will respond with the provided objects. +// It's backed by a very simple object tracker that processes creates, updates and deletions as-is, +// without applying any validations and/or defaults. It shouldn't be considered a replacement +// for a real clientset and is mostly useful in simple unit tests. +func NewSimpleClientset(objects ...runtime.Object) *Clientset { + o := testing.NewObjectTracker(scheme, codecs.UniversalDecoder()) + for _, obj := range objects { + if err := o.Add(obj); err != nil { + panic(err) + } + } + + cs := &Clientset{tracker: o} + cs.discovery = &fakediscovery.FakeDiscovery{Fake: &cs.Fake} + cs.AddReactor("*", "*", testing.ObjectReaction(o)) + cs.AddWatchReactor("*", func(action testing.Action) (handled bool, ret watch.Interface, err error) { + gvr := action.GetResource() + ns := action.GetNamespace() + watch, err := o.Watch(gvr, ns) + if err != nil { + return false, nil, err + } + return true, watch, nil + }) + + return cs +} + +// Clientset implements clientset.Interface. Meant to be embedded into a +// struct to get a default implementation. This makes faking out just the method +// you want to test easier. +type Clientset struct { + testing.Fake + discovery *fakediscovery.FakeDiscovery + tracker testing.ObjectTracker +} + +func (c *Clientset) Discovery() discovery.DiscoveryInterface { + return c.discovery +} + +func (c *Clientset) Tracker() testing.ObjectTracker { + return c.tracker +} + +var ( + _ clientset.Interface = &Clientset{} + _ testing.FakeClient = &Clientset{} +) + +// SecurityV1 retrieves the SecurityV1Client +func (c *Clientset) SecurityV1() securityv1.SecurityV1Interface { + return &fakesecurityv1.FakeSecurityV1{Fake: &c.Fake} +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/doc.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/doc.go new file mode 100644 index 000000000..3630ed1cd --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/doc.go @@ -0,0 +1,4 @@ +// Code generated by client-gen. DO NOT EDIT. + +// This package has the automatically generated fake clientset. +package fake diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/register.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/register.go new file mode 100644 index 000000000..ef2411341 --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/fake/register.go @@ -0,0 +1,40 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + securityv1 "github.com/openshift/api/security/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + runtime "k8s.io/apimachinery/pkg/runtime" + schema "k8s.io/apimachinery/pkg/runtime/schema" + serializer "k8s.io/apimachinery/pkg/runtime/serializer" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" +) + +var scheme = runtime.NewScheme() +var codecs = serializer.NewCodecFactory(scheme) + +var localSchemeBuilder = runtime.SchemeBuilder{ + securityv1.AddToScheme, +} + +// AddToScheme adds all types of this clientset into the given scheme. This allows composition +// of clientsets, like in: +// +// import ( +// "k8s.io/client-go/kubernetes" +// clientsetscheme "k8s.io/client-go/kubernetes/scheme" +// aggregatorclientsetscheme "k8s.io/kube-aggregator/pkg/client/clientset_generated/clientset/scheme" +// ) +// +// kclientset, _ := kubernetes.NewForConfig(c) +// _ = aggregatorclientsetscheme.AddToScheme(clientsetscheme.Scheme) +// +// After this, RawExtensions in Kubernetes types will serialize kube-aggregator types +// correctly. +var AddToScheme = localSchemeBuilder.AddToScheme + +func init() { + v1.AddToGroupVersion(scheme, schema.GroupVersion{Version: "v1"}) + utilruntime.Must(AddToScheme(scheme)) +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/doc.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/doc.go new file mode 100644 index 000000000..2b5ba4c8e --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/doc.go @@ -0,0 +1,4 @@ +// Code generated by client-gen. DO NOT EDIT. + +// Package fake has the automatically generated clients. +package fake diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyreview.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyreview.go new file mode 100644 index 000000000..85d015d8a --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyreview.go @@ -0,0 +1,33 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + "context" + + v1 "github.com/openshift/api/security/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + schema "k8s.io/apimachinery/pkg/runtime/schema" + testing "k8s.io/client-go/testing" +) + +// FakePodSecurityPolicyReviews implements PodSecurityPolicyReviewInterface +type FakePodSecurityPolicyReviews struct { + Fake *FakeSecurityV1 + ns string +} + +var podsecuritypolicyreviewsResource = schema.GroupVersionResource{Group: "security.openshift.io", Version: "v1", Resource: "podsecuritypolicyreviews"} + +var podsecuritypolicyreviewsKind = schema.GroupVersionKind{Group: "security.openshift.io", Version: "v1", Kind: "PodSecurityPolicyReview"} + +// Create takes the representation of a podSecurityPolicyReview and creates it. Returns the server's representation of the podSecurityPolicyReview, and an error, if there is any. +func (c *FakePodSecurityPolicyReviews) Create(ctx context.Context, podSecurityPolicyReview *v1.PodSecurityPolicyReview, opts metav1.CreateOptions) (result *v1.PodSecurityPolicyReview, err error) { + obj, err := c.Fake. + Invokes(testing.NewCreateAction(podsecuritypolicyreviewsResource, c.ns, podSecurityPolicyReview), &v1.PodSecurityPolicyReview{}) + + if obj == nil { + return nil, err + } + return obj.(*v1.PodSecurityPolicyReview), err +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyselfsubjectreview.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyselfsubjectreview.go new file mode 100644 index 000000000..846ea9dae --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicyselfsubjectreview.go @@ -0,0 +1,33 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + "context" + + v1 "github.com/openshift/api/security/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + schema "k8s.io/apimachinery/pkg/runtime/schema" + testing "k8s.io/client-go/testing" +) + +// FakePodSecurityPolicySelfSubjectReviews implements PodSecurityPolicySelfSubjectReviewInterface +type FakePodSecurityPolicySelfSubjectReviews struct { + Fake *FakeSecurityV1 + ns string +} + +var podsecuritypolicyselfsubjectreviewsResource = schema.GroupVersionResource{Group: "security.openshift.io", Version: "v1", Resource: "podsecuritypolicyselfsubjectreviews"} + +var podsecuritypolicyselfsubjectreviewsKind = schema.GroupVersionKind{Group: "security.openshift.io", Version: "v1", Kind: "PodSecurityPolicySelfSubjectReview"} + +// Create takes the representation of a podSecurityPolicySelfSubjectReview and creates it. Returns the server's representation of the podSecurityPolicySelfSubjectReview, and an error, if there is any. +func (c *FakePodSecurityPolicySelfSubjectReviews) Create(ctx context.Context, podSecurityPolicySelfSubjectReview *v1.PodSecurityPolicySelfSubjectReview, opts metav1.CreateOptions) (result *v1.PodSecurityPolicySelfSubjectReview, err error) { + obj, err := c.Fake. + Invokes(testing.NewCreateAction(podsecuritypolicyselfsubjectreviewsResource, c.ns, podSecurityPolicySelfSubjectReview), &v1.PodSecurityPolicySelfSubjectReview{}) + + if obj == nil { + return nil, err + } + return obj.(*v1.PodSecurityPolicySelfSubjectReview), err +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicysubjectreview.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicysubjectreview.go new file mode 100644 index 000000000..dbe5d308e --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_podsecuritypolicysubjectreview.go @@ -0,0 +1,33 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + "context" + + v1 "github.com/openshift/api/security/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + schema "k8s.io/apimachinery/pkg/runtime/schema" + testing "k8s.io/client-go/testing" +) + +// FakePodSecurityPolicySubjectReviews implements PodSecurityPolicySubjectReviewInterface +type FakePodSecurityPolicySubjectReviews struct { + Fake *FakeSecurityV1 + ns string +} + +var podsecuritypolicysubjectreviewsResource = schema.GroupVersionResource{Group: "security.openshift.io", Version: "v1", Resource: "podsecuritypolicysubjectreviews"} + +var podsecuritypolicysubjectreviewsKind = schema.GroupVersionKind{Group: "security.openshift.io", Version: "v1", Kind: "PodSecurityPolicySubjectReview"} + +// Create takes the representation of a podSecurityPolicySubjectReview and creates it. Returns the server's representation of the podSecurityPolicySubjectReview, and an error, if there is any. +func (c *FakePodSecurityPolicySubjectReviews) Create(ctx context.Context, podSecurityPolicySubjectReview *v1.PodSecurityPolicySubjectReview, opts metav1.CreateOptions) (result *v1.PodSecurityPolicySubjectReview, err error) { + obj, err := c.Fake. + Invokes(testing.NewCreateAction(podsecuritypolicysubjectreviewsResource, c.ns, podSecurityPolicySubjectReview), &v1.PodSecurityPolicySubjectReview{}) + + if obj == nil { + return nil, err + } + return obj.(*v1.PodSecurityPolicySubjectReview), err +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_rangeallocation.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_rangeallocation.go new file mode 100644 index 000000000..d05477811 --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_rangeallocation.go @@ -0,0 +1,130 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + "context" + json "encoding/json" + "fmt" + + securityv1 "github.com/openshift/api/security/v1" + applyconfigurationssecurityv1 "github.com/openshift/client-go/security/applyconfigurations/security/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + labels "k8s.io/apimachinery/pkg/labels" + schema "k8s.io/apimachinery/pkg/runtime/schema" + types "k8s.io/apimachinery/pkg/types" + watch "k8s.io/apimachinery/pkg/watch" + testing "k8s.io/client-go/testing" +) + +// FakeRangeAllocations implements RangeAllocationInterface +type FakeRangeAllocations struct { + Fake *FakeSecurityV1 +} + +var rangeallocationsResource = schema.GroupVersionResource{Group: "security.openshift.io", Version: "v1", Resource: "rangeallocations"} + +var rangeallocationsKind = schema.GroupVersionKind{Group: "security.openshift.io", Version: "v1", Kind: "RangeAllocation"} + +// Get takes name of the rangeAllocation, and returns the corresponding rangeAllocation object, and an error if there is any. +func (c *FakeRangeAllocations) Get(ctx context.Context, name string, options v1.GetOptions) (result *securityv1.RangeAllocation, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootGetAction(rangeallocationsResource, name), &securityv1.RangeAllocation{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.RangeAllocation), err +} + +// List takes label and field selectors, and returns the list of RangeAllocations that match those selectors. +func (c *FakeRangeAllocations) List(ctx context.Context, opts v1.ListOptions) (result *securityv1.RangeAllocationList, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootListAction(rangeallocationsResource, rangeallocationsKind, opts), &securityv1.RangeAllocationList{}) + if obj == nil { + return nil, err + } + + label, _, _ := testing.ExtractFromListOptions(opts) + if label == nil { + label = labels.Everything() + } + list := &securityv1.RangeAllocationList{ListMeta: obj.(*securityv1.RangeAllocationList).ListMeta} + for _, item := range obj.(*securityv1.RangeAllocationList).Items { + if label.Matches(labels.Set(item.Labels)) { + list.Items = append(list.Items, item) + } + } + return list, err +} + +// Watch returns a watch.Interface that watches the requested rangeAllocations. +func (c *FakeRangeAllocations) Watch(ctx context.Context, opts v1.ListOptions) (watch.Interface, error) { + return c.Fake. + InvokesWatch(testing.NewRootWatchAction(rangeallocationsResource, opts)) +} + +// Create takes the representation of a rangeAllocation and creates it. Returns the server's representation of the rangeAllocation, and an error, if there is any. +func (c *FakeRangeAllocations) Create(ctx context.Context, rangeAllocation *securityv1.RangeAllocation, opts v1.CreateOptions) (result *securityv1.RangeAllocation, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootCreateAction(rangeallocationsResource, rangeAllocation), &securityv1.RangeAllocation{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.RangeAllocation), err +} + +// Update takes the representation of a rangeAllocation and updates it. Returns the server's representation of the rangeAllocation, and an error, if there is any. +func (c *FakeRangeAllocations) Update(ctx context.Context, rangeAllocation *securityv1.RangeAllocation, opts v1.UpdateOptions) (result *securityv1.RangeAllocation, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootUpdateAction(rangeallocationsResource, rangeAllocation), &securityv1.RangeAllocation{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.RangeAllocation), err +} + +// Delete takes name of the rangeAllocation and deletes it. Returns an error if one occurs. +func (c *FakeRangeAllocations) Delete(ctx context.Context, name string, opts v1.DeleteOptions) error { + _, err := c.Fake. + Invokes(testing.NewRootDeleteActionWithOptions(rangeallocationsResource, name, opts), &securityv1.RangeAllocation{}) + return err +} + +// DeleteCollection deletes a collection of objects. +func (c *FakeRangeAllocations) DeleteCollection(ctx context.Context, opts v1.DeleteOptions, listOpts v1.ListOptions) error { + action := testing.NewRootDeleteCollectionAction(rangeallocationsResource, listOpts) + + _, err := c.Fake.Invokes(action, &securityv1.RangeAllocationList{}) + return err +} + +// Patch applies the patch and returns the patched rangeAllocation. +func (c *FakeRangeAllocations) Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts v1.PatchOptions, subresources ...string) (result *securityv1.RangeAllocation, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootPatchSubresourceAction(rangeallocationsResource, name, pt, data, subresources...), &securityv1.RangeAllocation{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.RangeAllocation), err +} + +// Apply takes the given apply declarative configuration, applies it and returns the applied rangeAllocation. +func (c *FakeRangeAllocations) Apply(ctx context.Context, rangeAllocation *applyconfigurationssecurityv1.RangeAllocationApplyConfiguration, opts v1.ApplyOptions) (result *securityv1.RangeAllocation, err error) { + if rangeAllocation == nil { + return nil, fmt.Errorf("rangeAllocation provided to Apply must not be nil") + } + data, err := json.Marshal(rangeAllocation) + if err != nil { + return nil, err + } + name := rangeAllocation.Name + if name == nil { + return nil, fmt.Errorf("rangeAllocation.Name must be provided to Apply") + } + obj, err := c.Fake. + Invokes(testing.NewRootPatchSubresourceAction(rangeallocationsResource, *name, types.ApplyPatchType, data), &securityv1.RangeAllocation{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.RangeAllocation), err +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_security_client.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_security_client.go new file mode 100644 index 000000000..33240c41c --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_security_client.go @@ -0,0 +1,40 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + v1 "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1" + rest "k8s.io/client-go/rest" + testing "k8s.io/client-go/testing" +) + +type FakeSecurityV1 struct { + *testing.Fake +} + +func (c *FakeSecurityV1) PodSecurityPolicyReviews(namespace string) v1.PodSecurityPolicyReviewInterface { + return &FakePodSecurityPolicyReviews{c, namespace} +} + +func (c *FakeSecurityV1) PodSecurityPolicySelfSubjectReviews(namespace string) v1.PodSecurityPolicySelfSubjectReviewInterface { + return &FakePodSecurityPolicySelfSubjectReviews{c, namespace} +} + +func (c *FakeSecurityV1) PodSecurityPolicySubjectReviews(namespace string) v1.PodSecurityPolicySubjectReviewInterface { + return &FakePodSecurityPolicySubjectReviews{c, namespace} +} + +func (c *FakeSecurityV1) RangeAllocations() v1.RangeAllocationInterface { + return &FakeRangeAllocations{c} +} + +func (c *FakeSecurityV1) SecurityContextConstraints() v1.SecurityContextConstraintsInterface { + return &FakeSecurityContextConstraints{c} +} + +// RESTClient returns a RESTClient that is used to communicate +// with API server by this client implementation. +func (c *FakeSecurityV1) RESTClient() rest.Interface { + var ret *rest.RESTClient + return ret +} diff --git a/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_securitycontextconstraints.go b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_securitycontextconstraints.go new file mode 100644 index 000000000..2b570a498 --- /dev/null +++ b/vendor/github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake/fake_securitycontextconstraints.go @@ -0,0 +1,130 @@ +// Code generated by client-gen. DO NOT EDIT. + +package fake + +import ( + "context" + json "encoding/json" + "fmt" + + securityv1 "github.com/openshift/api/security/v1" + applyconfigurationssecurityv1 "github.com/openshift/client-go/security/applyconfigurations/security/v1" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + labels "k8s.io/apimachinery/pkg/labels" + schema "k8s.io/apimachinery/pkg/runtime/schema" + types "k8s.io/apimachinery/pkg/types" + watch "k8s.io/apimachinery/pkg/watch" + testing "k8s.io/client-go/testing" +) + +// FakeSecurityContextConstraints implements SecurityContextConstraintsInterface +type FakeSecurityContextConstraints struct { + Fake *FakeSecurityV1 +} + +var securitycontextconstraintsResource = schema.GroupVersionResource{Group: "security.openshift.io", Version: "v1", Resource: "securitycontextconstraints"} + +var securitycontextconstraintsKind = schema.GroupVersionKind{Group: "security.openshift.io", Version: "v1", Kind: "SecurityContextConstraints"} + +// Get takes name of the securityContextConstraints, and returns the corresponding securityContextConstraints object, and an error if there is any. +func (c *FakeSecurityContextConstraints) Get(ctx context.Context, name string, options v1.GetOptions) (result *securityv1.SecurityContextConstraints, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootGetAction(securitycontextconstraintsResource, name), &securityv1.SecurityContextConstraints{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.SecurityContextConstraints), err +} + +// List takes label and field selectors, and returns the list of SecurityContextConstraints that match those selectors. +func (c *FakeSecurityContextConstraints) List(ctx context.Context, opts v1.ListOptions) (result *securityv1.SecurityContextConstraintsList, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootListAction(securitycontextconstraintsResource, securitycontextconstraintsKind, opts), &securityv1.SecurityContextConstraintsList{}) + if obj == nil { + return nil, err + } + + label, _, _ := testing.ExtractFromListOptions(opts) + if label == nil { + label = labels.Everything() + } + list := &securityv1.SecurityContextConstraintsList{ListMeta: obj.(*securityv1.SecurityContextConstraintsList).ListMeta} + for _, item := range obj.(*securityv1.SecurityContextConstraintsList).Items { + if label.Matches(labels.Set(item.Labels)) { + list.Items = append(list.Items, item) + } + } + return list, err +} + +// Watch returns a watch.Interface that watches the requested securityContextConstraints. +func (c *FakeSecurityContextConstraints) Watch(ctx context.Context, opts v1.ListOptions) (watch.Interface, error) { + return c.Fake. + InvokesWatch(testing.NewRootWatchAction(securitycontextconstraintsResource, opts)) +} + +// Create takes the representation of a securityContextConstraints and creates it. Returns the server's representation of the securityContextConstraints, and an error, if there is any. +func (c *FakeSecurityContextConstraints) Create(ctx context.Context, securityContextConstraints *securityv1.SecurityContextConstraints, opts v1.CreateOptions) (result *securityv1.SecurityContextConstraints, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootCreateAction(securitycontextconstraintsResource, securityContextConstraints), &securityv1.SecurityContextConstraints{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.SecurityContextConstraints), err +} + +// Update takes the representation of a securityContextConstraints and updates it. Returns the server's representation of the securityContextConstraints, and an error, if there is any. +func (c *FakeSecurityContextConstraints) Update(ctx context.Context, securityContextConstraints *securityv1.SecurityContextConstraints, opts v1.UpdateOptions) (result *securityv1.SecurityContextConstraints, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootUpdateAction(securitycontextconstraintsResource, securityContextConstraints), &securityv1.SecurityContextConstraints{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.SecurityContextConstraints), err +} + +// Delete takes name of the securityContextConstraints and deletes it. Returns an error if one occurs. +func (c *FakeSecurityContextConstraints) Delete(ctx context.Context, name string, opts v1.DeleteOptions) error { + _, err := c.Fake. + Invokes(testing.NewRootDeleteActionWithOptions(securitycontextconstraintsResource, name, opts), &securityv1.SecurityContextConstraints{}) + return err +} + +// DeleteCollection deletes a collection of objects. +func (c *FakeSecurityContextConstraints) DeleteCollection(ctx context.Context, opts v1.DeleteOptions, listOpts v1.ListOptions) error { + action := testing.NewRootDeleteCollectionAction(securitycontextconstraintsResource, listOpts) + + _, err := c.Fake.Invokes(action, &securityv1.SecurityContextConstraintsList{}) + return err +} + +// Patch applies the patch and returns the patched securityContextConstraints. +func (c *FakeSecurityContextConstraints) Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts v1.PatchOptions, subresources ...string) (result *securityv1.SecurityContextConstraints, err error) { + obj, err := c.Fake. + Invokes(testing.NewRootPatchSubresourceAction(securitycontextconstraintsResource, name, pt, data, subresources...), &securityv1.SecurityContextConstraints{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.SecurityContextConstraints), err +} + +// Apply takes the given apply declarative configuration, applies it and returns the applied securityContextConstraints. +func (c *FakeSecurityContextConstraints) Apply(ctx context.Context, securityContextConstraints *applyconfigurationssecurityv1.SecurityContextConstraintsApplyConfiguration, opts v1.ApplyOptions) (result *securityv1.SecurityContextConstraints, err error) { + if securityContextConstraints == nil { + return nil, fmt.Errorf("securityContextConstraints provided to Apply must not be nil") + } + data, err := json.Marshal(securityContextConstraints) + if err != nil { + return nil, err + } + name := securityContextConstraints.Name + if name == nil { + return nil, fmt.Errorf("securityContextConstraints.Name must be provided to Apply") + } + obj, err := c.Fake. + Invokes(testing.NewRootPatchSubresourceAction(securitycontextconstraintsResource, *name, types.ApplyPatchType, data), &securityv1.SecurityContextConstraints{}) + if obj == nil { + return nil, err + } + return obj.(*securityv1.SecurityContextConstraints), err +} diff --git a/vendor/modules.txt b/vendor/modules.txt index aaf7bd914..d3dc4265a 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -142,8 +142,11 @@ github.com/openshift/client-go/image/clientset/versioned/scheme github.com/openshift/client-go/image/clientset/versioned/typed/image/v1 github.com/openshift/client-go/security/applyconfigurations/internal github.com/openshift/client-go/security/applyconfigurations/security/v1 +github.com/openshift/client-go/security/clientset/versioned +github.com/openshift/client-go/security/clientset/versioned/fake github.com/openshift/client-go/security/clientset/versioned/scheme github.com/openshift/client-go/security/clientset/versioned/typed/security/v1 +github.com/openshift/client-go/security/clientset/versioned/typed/security/v1/fake # github.com/openshift/library-go v0.0.0-20221012165547-f859132ee700 ## explicit; go 1.18 github.com/openshift/library-go/pkg/config/clusterstatus From 756276e5870fd66c0973910bccd5c402f4e7d6ef Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Tue, 19 Sep 2023 18:12:13 +0200 Subject: [PATCH 3/7] Properly reconcile SCC resources, tolerate cluster modifications Previously CVO only created SCC resources when they did not exist, but did not enforce any further state from the manifests afterwards, so e.g. SCC changes on updates were never propagated to the cluster. There is an existing `EnsureSecurityContextConstraint` method in the code, but it was not used anywhere. It is possible that calling it from `ApplySecurityContextConstraint` is all that is missing. The method has a "merge" semantics on various lists though (so the resulting state is the union of what is in the cluster and in the manifest), which plays nice and tolerates user changes. This behavior will be removed in a followup commit because we want CVO to strictly reconcile to the state desired in the manifest. --- lib/resourceapply/security.go | 14 ++++++++------ lib/resourceapply/security_test.go | 16 +++++++++++++--- 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index ec839e6ba..81a552571 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -10,7 +10,6 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" - "k8s.io/utils/pointer" ) // ApplySecurityContextConstraintsv1 applies the required SecurityContextConstraints to the cluster. @@ -29,16 +28,19 @@ 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 := resourcemerge.EnsureSecurityContextConstraints(*existing, *required) + 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 } diff --git a/lib/resourceapply/security_test.go b/lib/resourceapply/security_test.go index 1c3191c90..e5755ba15 100644 --- a/lib/resourceapply/security_test.go +++ b/lib/resourceapply/security_test.go @@ -95,20 +95,30 @@ func TestApplySecurityContextConstraintsv1(t *testing.T) { expectedModified: false, }, { - name: "enforce missing volumes from required DOES NOT WORK because OCPBUGS-18386, modified is false", + name: "enforce missing volumes from required, modified is true", existing: restrictedv2.DeepCopy, required: func() *securityv1.SecurityContextConstraints { scc := restrictedv2.DeepCopy() scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "ephemeral", "persistentVolumeClaim", "projected", "secret"} return scc }, - expected: restrictedv2.DeepCopy, + expected: 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(func() *securityv1.SecurityContextConstraints { + scc := restrictedv2.DeepCopy() + scc.Volumes = []securityv1.FSType{"configMap", "downwardAPI", "emptyDir", "persistentVolumeClaim", "projected", "secret", "ephemeral"} + return scc + }()), }, + expectedModified: true, }, { - name: "do not keep additional volumes from existing DOES NOT WORK because OCPBUGS-18386, modified is false", + name: "keep additional volumes from existing because we want merge semantics, modified is false", existing: restrictedv2.DeepCopy, required: func() *securityv1.SecurityContextConstraints { scc := restrictedv2.DeepCopy() From 9eeb0eed6608da590e88e805c25fad5da7b8f868 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Wed, 20 Sep 2023 17:26:51 +0200 Subject: [PATCH 4/7] resourcemerge: handle defaulting of allowPrivilegeEscalation in SCCs ```console $ oc explain scc.allowPrivilegeEscalation KIND: SecurityContextConstraints VERSION: security.openshift.io/v1 DESCRIPTION: AllowPrivilegeEscalation determines if a pod can request to allow privilege escalation. If unspecified, defaults to true. ``` --- lib/resourceapply/security_test.go | 31 +++++++++----- lib/resourcemerge/security.go | 11 ++++- lib/resourcemerge/security_test.go | 68 +++++++++++++++++++++--------- 3 files changed, 80 insertions(+), 30 deletions(-) diff --git a/lib/resourceapply/security_test.go b/lib/resourceapply/security_test.go index e5755ba15..913130fe4 100644 --- a/lib/resourceapply/security_test.go +++ b/lib/resourceapply/security_test.go @@ -9,6 +9,7 @@ import ( 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" @@ -35,6 +36,16 @@ var restrictedv2 = securityv1.SecurityContextConstraints{ 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) } @@ -69,10 +80,10 @@ func TestApplySecurityContextConstraintsv1(t *testing.T) { expectedModified: true, }, { - name: "no modified when existing is equal to required", - existing: restrictedv2.DeepCopy, + name: "no modification when existing is equal to required", + existing: defaulted(restrictedv2.DeepCopy), required: restrictedv2.DeepCopy, - expected: restrictedv2.DeepCopy, + expected: defaulted(restrictedv2.DeepCopy), expectedAPICalls: []kubetesting.Action{ getSCCAction("restricted-v2"), }, @@ -96,36 +107,36 @@ func TestApplySecurityContextConstraintsv1(t *testing.T) { }, { name: "enforce missing volumes from required, modified is true", - existing: restrictedv2.DeepCopy, + 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: func() *securityv1.SecurityContextConstraints { + 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(func() *securityv1.SecurityContextConstraints { + 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: restrictedv2.DeepCopy, + existing: defaulted(restrictedv2.DeepCopy), required: func() *securityv1.SecurityContextConstraints { scc := restrictedv2.DeepCopy() scc.Volumes = []securityv1.FSType{"configMap"} return scc }, - expected: restrictedv2.DeepCopy, + expected: defaulted(restrictedv2.DeepCopy), expectedAPICalls: []kubetesting.Action{ getSCCAction("restricted-v2"), }, diff --git a/lib/resourcemerge/security.go b/lib/resourcemerge/security.go index 59ec7007b..8a6fe748a 100644 --- a/lib/resourcemerge/security.go +++ b/lib/resourcemerge/security.go @@ -3,6 +3,7 @@ package resourcemerge import ( securityv1 "github.com/openshift/api/security/v1" "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/utils/pointer" ) // EnsureSecurityContextConstraints ensures that the result matches the required. @@ -85,7 +86,15 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra setBool(&modified, &result.AllowHostPID, required.AllowHostPID) setBool(&modified, &result.AllowHostIPC, required.AllowHostIPC) setBoolPtr(&modified, &result.DefaultAllowPrivilegeEscalation, required.DefaultAllowPrivilegeEscalation) - setBoolPtr(&modified, &result.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) + + // AllowPrivilegeEscalation is optional and defaults to true if not specified, + // so we enforce default if manifest does not specify it. + if required.AllowPrivilegeEscalation == nil { + setBoolPtr(&modified, &result.AllowPrivilegeEscalation, pointer.Bool(true)) + } else { + setBoolPtr(&modified, &result.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) + } + if !equality.Semantic.DeepEqual(result.SELinuxContext, required.SELinuxContext) { modified = true result.SELinuxContext = required.SELinuxContext diff --git a/lib/resourcemerge/security_test.go b/lib/resourcemerge/security_test.go index 9402557e4..c6ecb040f 100644 --- a/lib/resourcemerge/security_test.go +++ b/lib/resourcemerge/security_test.go @@ -4,10 +4,11 @@ import ( "testing" "github.com/google/go-cmp/cmp" - securityv1 "github.com/openshift/api/security/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/pointer" + + securityv1 "github.com/openshift/api/security/v1" ) var restrictedv2 = securityv1.SecurityContextConstraints{ @@ -31,6 +32,13 @@ var restrictedv2 = securityv1.SecurityContextConstraints{ SeccompProfiles: []string{"runtime/default"}, } +func defaulted(scc securityv1.SecurityContextConstraints) *securityv1.SecurityContextConstraints { + if scc.AllowPrivilegeEscalation == nil { + scc.AllowPrivilegeEscalation = pointer.Bool(true) + } + return &scc +} + func TestEnsureSecurityContextConstraints(t *testing.T) { testCases := []struct { name string @@ -41,8 +49,8 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { }{ { name: "no modified when existing is equal to required", - existing: *restrictedv2.DeepCopy(), - required: *restrictedv2.DeepCopy(), + existing: *defaulted(*restrictedv2.DeepCopy()), + required: *defaulted(*restrictedv2.DeepCopy()), expected: nil, }, { @@ -86,68 +94,90 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { }, }, { - name: "enforce capabilities", + name: "enforce allowPrivilegeEscalation as default true when not specified", existing: securityv1.SecurityContextConstraints{ + AllowPrivilegeEscalation: pointer.Bool(false), + }, + required: securityv1.SecurityContextConstraints{ + AllowPrivilegeEscalation: nil, + }, + expected: &securityv1.SecurityContextConstraints{ + AllowPrivilegeEscalation: pointer.Bool(true), + }, + }, + { + name: "allowPrivilegeEscalation does not need to be reconciled when existing state is true (default)", + existing: securityv1.SecurityContextConstraints{ + AllowPrivilegeEscalation: pointer.Bool(true), + }, + required: securityv1.SecurityContextConstraints{ + AllowPrivilegeEscalation: nil, + }, + expected: nil, + }, + { + name: "enforce capabilities", + existing: *defaulted(securityv1.SecurityContextConstraints{ DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B"}, RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D"}, AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F"}, - }, + }), required: securityv1.SecurityContextConstraints{ DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "REQUIRED_A", "REQUIRED_B"}, RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "REQUIRED_C", "REQUIRED_D"}, AllowedCapabilities: []corev1.Capability{"SHARED_C", "REQUIRED_E", "REQUIRED_F"}, }, - expected: &securityv1.SecurityContextConstraints{ + expected: defaulted(securityv1.SecurityContextConstraints{ DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B", "REQUIRED_A", "REQUIRED_B"}, RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D", "REQUIRED_C", "REQUIRED_D"}, AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F", "REQUIRED_E", "REQUIRED_F"}, - }, + }), }, { name: "enforce volumes", - existing: securityv1.SecurityContextConstraints{ + existing: *defaulted(securityv1.SecurityContextConstraints{ Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret}, AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}}, - }, + }), required: securityv1.SecurityContextConstraints{ Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "required-1"}, {Driver: "required-2"}}, }, - expected: &securityv1.SecurityContextConstraints{ + expected: defaulted(securityv1.SecurityContextConstraints{ Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}, {Driver: "required-1"}, {Driver: "required-2"}}, - }, + }), }, { name: "enforce strategy options", - existing: securityv1.SecurityContextConstraints{ + existing: *defaulted(securityv1.SecurityContextConstraints{ SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs, SELinuxOptions: &corev1.SELinuxOptions{User: "ooser"}}, RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyRunAsAny}, SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyMustRunAs}, FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyRunAsAny}, - }, + }), required: securityv1.SecurityContextConstraints{ SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, }, - expected: &securityv1.SecurityContextConstraints{ + expected: defaulted(securityv1.SecurityContextConstraints{ SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, - }, + }), }, { name: "enforce users, groups, seccomp profiles, sysctls", - existing: securityv1.SecurityContextConstraints{ + existing: *defaulted(securityv1.SecurityContextConstraints{ Users: []string{"shared-u", "existing-user-1", "existing-user-2"}, Groups: []string{"shared-g", "existing-group-1", "existing-group-2"}, SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2"}, AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2"}, ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2"}, - }, + }), required: securityv1.SecurityContextConstraints{ Users: []string{"shared-u", "required-user-1", "required-user-2"}, Groups: []string{"shared-g", "required-group-1", "required-group-2"}, @@ -155,13 +185,13 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { AllowedUnsafeSysctls: []string{"shared-a", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, ForbiddenSysctls: []string{"shared-f", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, }, - expected: &securityv1.SecurityContextConstraints{ + expected: defaulted(securityv1.SecurityContextConstraints{ Users: []string{"shared-u", "existing-user-1", "existing-user-2", "required-user-1", "required-user-2"}, Groups: []string{"shared-g", "existing-group-1", "existing-group-2", "required-group-1", "required-group-2"}, SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2", "required-seccomp-1", "required-seccomp-2"}, AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, - }, + }), }, } From 2fb153ad0698b3c2821d566d9300466060c18d4b Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 15 Sep 2023 11:52:24 +0200 Subject: [PATCH 5/7] resourcemerge: detect modified SCCs and set Upgradeable=False on them Implemented following the same pattern like `clusterManifestDeleteInProgressUpgradeable`, unfortunately uses a global but trying to weave some communication channel through the builder hiearchy would result in a more invasive, and hence risky, change. --- lib/resourceapply/security.go | 66 ++++++++++++++++++++++++++++-- lib/resourcemerge/security.go | 64 +++++++++++++++++++++-------- lib/resourcemerge/security_test.go | 36 ++++++++++------ pkg/cvo/upgradeable.go | 24 ++++++++++- 4 files changed, 156 insertions(+), 34 deletions(-) diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index 81a552571..69675b5e4 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -2,16 +2,69 @@ package resourceapply import ( "context" + "sort" + "sync" + "time" "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" + + 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" ) +type modifiedSccTracker struct { + sync.Mutex + lastUpdated time.Time + names map[string]time.Time +} + +var modifiedSccs modifiedSccTracker + +func init() { + modifiedSccs = modifiedSccTracker{ + lastUpdated: time.Now().UTC(), + names: make(map[string]time.Time), + } +} + +// ModifiedSCCs returns the list of SCCs that have been modified in the cluster +func ModifiedSCCs() []string { + modifiedSccs.Lock() + defer modifiedSccs.Unlock() + names := make([]string, 0, len(modifiedSccs.names)) + + // discard any records older than 10 minutes before last cache write, they are + // likely stale (this should not happen, but we are being defensive) + staleThreshold := modifiedSccs.lastUpdated.Add(-10 * time.Minute) + for name, saw := range modifiedSccs.names { + if saw.Before(staleThreshold) { + delete(modifiedSccs.names, name) + } else { + names = append(names, name) + } + } + + sort.Strings(names) + return names +} + +// markSccModifiedInCluster marks the SCC as modified in the cluster +func trackModifiedScc(name string, modified bool) { + modifiedSccs.Lock() + defer modifiedSccs.Unlock() + modifiedSccs.lastUpdated = time.Now().UTC() + if modified { + modifiedSccs.names[name] = modifiedSccs.lastUpdated + } else { + delete(modifiedSccs.names, name) + } +} + // ApplySecurityContextConstraintsv1 applies the required SecurityContextConstraints to the cluster. func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclientv1.SecurityContextConstraintsGetter, required *securityv1.SecurityContextConstraints, reconciling bool) (*securityv1.SecurityContextConstraints, bool, error) { existing, err := client.SecurityContextConstraints().Get(ctx, required.Name, metav1.GetOptions{}) @@ -28,7 +81,12 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien return nil, false, nil } - reconcile := resourcemerge.EnsureSecurityContextConstraints(*existing, *required) + reconcile, requiredMerges := resourcemerge.EnsureSecurityContextConstraints(*existing, *required) + + if reconciling { + trackModifiedScc(required.Name, requiredMerges) + } + if reconcile == nil { return existing, false, nil } diff --git a/lib/resourcemerge/security.go b/lib/resourcemerge/security.go index 8a6fe748a..343165f45 100644 --- a/lib/resourcemerge/security.go +++ b/lib/resourcemerge/security.go @@ -1,15 +1,24 @@ package resourcemerge import ( - securityv1 "github.com/openshift/api/security/v1" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/utils/pointer" + + securityv1 "github.com/openshift/api/security/v1" ) -// EnsureSecurityContextConstraints ensures that the result matches the required. -// modified is set to true when result had to be updated with required. -func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstraints, required securityv1.SecurityContextConstraints) *securityv1.SecurityContextConstraints { +// EnsureSecurityContextConstraints compares the existing state with the required states and returns +// SCC to be applied to reconcile the state. If there were items in the existing state that were not +// present in the required state, the items are kept and the returned SCC contains items from both +// the existing and required state. If this happens then the boolean return value is set to true. +// +// If no reconciliation is needed, the method returns nil. Note that the boolean can be still true +// indicating that existing state has additional items not present in the required state, but the +// existing state is already properly reconciled to desired state. +func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstraints, required securityv1.SecurityContextConstraints) (*securityv1.SecurityContextConstraints, bool) { var modified bool + var mergedClusterState bool result := existing.DeepCopy() EnsureObjectMeta(&modified, &result.ObjectMeta, required.ObjectMeta) @@ -18,8 +27,7 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra for _, required := range required.DefaultAddCapabilities { found := false for _, curr := range result.DefaultAddCapabilities { - if equality.Semantic.DeepEqual(required, curr) { - found = true + if found = equality.Semantic.DeepEqual(required, curr); found { break } } @@ -31,8 +39,7 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra for _, required := range required.RequiredDropCapabilities { found := false for _, curr := range result.RequiredDropCapabilities { - if equality.Semantic.DeepEqual(required, curr) { - found = true + if found = equality.Semantic.DeepEqual(required, curr); found { break } } @@ -44,8 +51,7 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra for _, required := range required.AllowedCapabilities { found := false for _, curr := range result.AllowedCapabilities { - if equality.Semantic.DeepEqual(required, curr) { - found = true + if found = equality.Semantic.DeepEqual(required, curr); found { break } } @@ -54,12 +60,22 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra result.AllowedCapabilities = append(result.AllowedCapabilities, required) } } + + for _, capabilityLists := range [][][]corev1.Capability{ + {result.DefaultAddCapabilities, required.DefaultAddCapabilities}, + {result.RequiredDropCapabilities, required.RequiredDropCapabilities}, + {result.AllowedCapabilities, required.AllowedCapabilities}, + } { + if result, required := len(capabilityLists[0]), len(capabilityLists[1]); result > required { + mergedClusterState = true + } + } + setBool(&modified, &result.AllowHostDirVolumePlugin, required.AllowHostDirVolumePlugin) for _, required := range required.Volumes { found := false for _, curr := range result.Volumes { - if equality.Semantic.DeepEqual(required, curr) { - found = true + if found = equality.Semantic.DeepEqual(required, curr); found { break } } @@ -71,8 +87,7 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra for _, required := range required.AllowedFlexVolumes { found := false for _, curr := range result.AllowedFlexVolumes { - if equality.Semantic.DeepEqual(required.Driver, curr.Driver) { - found = true + if found = equality.Semantic.DeepEqual(required.Driver, curr.Driver); found { break } } @@ -81,6 +96,11 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra result.AllowedFlexVolumes = append(result.AllowedFlexVolumes, required) } } + + if len(result.Volumes) > len(required.Volumes) || len(result.AllowedFlexVolumes) > len(required.AllowedFlexVolumes) { + mergedClusterState = true + } + setBool(&modified, &result.AllowHostNetwork, required.AllowHostNetwork) setBool(&modified, &result.AllowHostPorts, required.AllowHostPorts) setBool(&modified, &result.AllowHostPID, required.AllowHostPID) @@ -118,8 +138,20 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra mergeStringSlice(&modified, &result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls) mergeStringSlice(&modified, &result.ForbiddenSysctls, required.ForbiddenSysctls) + for _, itemLists := range [][][]string{ + {result.Users, required.Users}, + {result.Groups, required.Groups}, + {result.SeccompProfiles, required.SeccompProfiles}, + {result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls}, + {result.ForbiddenSysctls, required.ForbiddenSysctls}, + } { + if result, required := len(itemLists[0]), len(itemLists[1]); result > required { + mergedClusterState = true + } + } + if modified { - return result + return result, mergedClusterState } - return nil + return nil, mergedClusterState } diff --git a/lib/resourcemerge/security_test.go b/lib/resourcemerge/security_test.go index c6ecb040f..32de03a35 100644 --- a/lib/resourcemerge/security_test.go +++ b/lib/resourcemerge/security_test.go @@ -45,17 +45,17 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { existing securityv1.SecurityContextConstraints required securityv1.SecurityContextConstraints - expected *securityv1.SecurityContextConstraints + expected *securityv1.SecurityContextConstraints + expectedMergedClusterState bool }{ { - name: "no modified when existing is equal to required", - existing: *defaulted(*restrictedv2.DeepCopy()), - required: *defaulted(*restrictedv2.DeepCopy()), - expected: nil, + name: "no reconcile needed when existing is equal to required", + existing: *defaulted(*restrictedv2.DeepCopy()), + required: *defaulted(*restrictedv2.DeepCopy()), + expectedMergedClusterState: false, }, { - - name: "enforce primitive fields", + name: "enforce primitive fields, primitives cannot be merged", existing: securityv1.SecurityContextConstraints{ Priority: pointer.Int32(123), AllowPrivilegedContainer: true, @@ -92,6 +92,7 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { AllowPrivilegeEscalation: pointer.Bool(true), ReadOnlyRootFilesystem: false, }, + expectedMergedClusterState: false, }, { name: "enforce allowPrivilegeEscalation as default true when not specified", @@ -104,6 +105,7 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { expected: &securityv1.SecurityContextConstraints{ AllowPrivilegeEscalation: pointer.Bool(true), }, + expectedMergedClusterState: false, }, { name: "allowPrivilegeEscalation does not need to be reconciled when existing state is true (default)", @@ -113,10 +115,10 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { required: securityv1.SecurityContextConstraints{ AllowPrivilegeEscalation: nil, }, - expected: nil, + expectedMergedClusterState: false, }, { - name: "enforce capabilities", + name: "enforce capabilities, merges are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B"}, RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D"}, @@ -132,9 +134,10 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D", "REQUIRED_C", "REQUIRED_D"}, AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F", "REQUIRED_E", "REQUIRED_F"}, }), + expectedMergedClusterState: true, }, { - name: "enforce volumes", + name: "enforce volumes, merges are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret}, AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}}, @@ -147,9 +150,10 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}, {Driver: "required-1"}, {Driver: "required-2"}}, }), + expectedMergedClusterState: true, }, { - name: "enforce strategy options", + name: "enforce strategy options, cannot be merged", existing: *defaulted(securityv1.SecurityContextConstraints{ SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs, SELinuxOptions: &corev1.SELinuxOptions{User: "ooser"}}, RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyRunAsAny}, @@ -168,9 +172,10 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, }), + expectedMergedClusterState: false, }, { - name: "enforce users, groups, seccomp profiles, sysctls", + name: "enforce users, groups, seccomp profiles, sysctls, merges are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ Users: []string{"shared-u", "existing-user-1", "existing-user-2"}, Groups: []string{"shared-g", "existing-group-1", "existing-group-2"}, @@ -192,12 +197,13 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, }), + expectedMergedClusterState: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result := EnsureSecurityContextConstraints(tc.existing, tc.required) + result, mergedClusterState := EnsureSecurityContextConstraints(tc.existing, tc.required) origExisting, origRequired := tc.existing.DeepCopy(), tc.required.DeepCopy() @@ -211,6 +217,10 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { if diff := cmp.Diff(origRequired, &tc.required); diff != "" { t.Errorf("Unexpected side effect on required state:\n%s", diff) } + + if mergedClusterState != tc.expectedMergedClusterState { + t.Errorf("expected mergedClusterState %v, got %v", tc.expectedMergedClusterState, mergedClusterState) + } }) } } diff --git a/pkg/cvo/upgradeable.go b/pkg/cvo/upgradeable.go index 0074e2a54..e08954074 100644 --- a/pkg/cvo/upgradeable.go +++ b/pkg/cvo/upgradeable.go @@ -9,6 +9,7 @@ import ( configv1 "github.com/openshift/api/config/v1" configlistersv1 "github.com/openshift/client-go/config/listers/config/v1" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" @@ -20,6 +21,7 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" + "github.com/openshift/cluster-version-operator/lib/resourceapply" "github.com/openshift/cluster-version-operator/lib/resourcedelete" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "github.com/openshift/cluster-version-operator/pkg/internal" @@ -182,7 +184,7 @@ func (optr *Operator) getUpgradeable() *upgradeable { } type upgradeableCheck interface { - // returns a not-nil condition when the check fails. + // Check returns a not-nil condition when the check fails. Check() *configv1.ClusterOperatorStatusCondition } @@ -426,6 +428,7 @@ func (optr *Operator) defaultUpgradeableChecks() []upgradeableCheck { }, &clusterOperatorsUpgradeable{coLister: optr.coLister}, &clusterManifestDeleteInProgressUpgradeable{}, + &modifiedSccDetectedUpgradeable{}, } } @@ -493,3 +496,22 @@ func (intervals *upgradeableCheckIntervals) throttlePeriod(cv *configv1.ClusterV } return intervals.min } + +type modifiedSccDetectedUpgradeable struct{} + +func (m modifiedSccDetectedUpgradeable) Check() *configv1.ClusterOperatorStatusCondition { + modifiedSccs := resourceapply.ModifiedSCCs() + if len(modifiedSccs) == 0 { + return nil + } + + resources := strings.Join(modifiedSccs, ",") + klog.V(2).Infof("Found SCC resources in the cluster with elements not present in payload manifest; resources=%s", resources) + + return &configv1.ClusterOperatorStatusCondition{ + Type: configv1.ClusterStatusConditionType("ModifiedSccResources"), + Reason: "DetectedModifiedSccResources", + Status: configv1.ConditionTrue, + Message: fmt.Sprintf("Detected modified SecurityContextConstraints: %s. These modifications would be removed by the update. Please ensure all cluster workloads are able to run without these usupported modifications, then delete the modified SCC resources (they will be recreated without the modifications). See https://access.redhat.com/solutions/7033949 for more information.", resources), + } +} From 69f8745343783446cc9a7163d5adcc3f9bc6a9b7 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Tue, 17 Oct 2023 17:27:17 +0200 Subject: [PATCH 6/7] resourcemerge: only reconcile `Volumes` in SCCs but track all changes Because reconciling SCCs can endanger existing workloads, we only add the merging semantics for the `Volumes` field, because we need to carry updates we did in manifests. I have checked SCC manifests in 4.11, 4.12 and 4.13 payloads, and we only add items to `volumes`, so we do not need other fields for now. For other fields, only track _whether_ we would need to reconcile towards the manifest state, so that we can report this cluster state to the admin before they upgrade to 4.14 which changes the reconciliation behavior to strict. --- lib/resourcemerge/core.go | 16 -- lib/resourcemerge/security.go | 164 ++++----------- lib/resourcemerge/security_test.go | 309 ++++++++++++++++++----------- 3 files changed, 238 insertions(+), 251 deletions(-) diff --git a/lib/resourcemerge/core.go b/lib/resourcemerge/core.go index 9f3288c6b..0aa8a79cf 100644 --- a/lib/resourcemerge/core.go +++ b/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)) diff --git a/lib/resourcemerge/security.go b/lib/resourcemerge/security.go index 343165f45..58f455db4 100644 --- a/lib/resourcemerge/security.go +++ b/lib/resourcemerge/security.go @@ -1,77 +1,60 @@ package resourcemerge import ( - corev1 "k8s.io/api/core/v1" + securityv1 "github.com/openshift/api/security/v1" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/utils/pointer" - - securityv1 "github.com/openshift/api/security/v1" ) // EnsureSecurityContextConstraints compares the existing state with the required states and returns -// SCC to be applied to reconcile the state. If there were items in the existing state that were not -// present in the required state, the items are kept and the returned SCC contains items from both -// the existing and required state. If this happens then the boolean return value is set to true. +// SCC to be applied to reconcile the state. To avoid breaking existing installations with a buggy +// behavior, this method does not actually enforce the required state except for the Volumes field +// (see below), but only reports, via the boolean return value, whether the existing state matches +// the required one. +// +// The Volumes member is treated differently: the target state is the union of the items in the +// existing state with items in required state. // -// If no reconciliation is needed, the method returns nil. Note that the boolean can be still true -// indicating that existing state has additional items not present in the required state, but the -// existing state is already properly reconciled to desired state. +// If no reconciliation is required, the method returns nil. func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstraints, required securityv1.SecurityContextConstraints) (*securityv1.SecurityContextConstraints, bool) { var modified bool - var mergedClusterState bool + var differsFromRequired bool result := existing.DeepCopy() EnsureObjectMeta(&modified, &result.ObjectMeta, required.ObjectMeta) - setInt32Ptr(&modified, &result.Priority, required.Priority) - setBool(&modified, &result.AllowPrivilegedContainer, required.AllowPrivilegedContainer) - for _, required := range required.DefaultAddCapabilities { - found := false - for _, curr := range result.DefaultAddCapabilities { - if found = equality.Semantic.DeepEqual(required, curr); found { - break - } - } - if !found { - modified = true - result.DefaultAddCapabilities = append(result.DefaultAddCapabilities, required) - } - } - for _, required := range required.RequiredDropCapabilities { - found := false - for _, curr := range result.RequiredDropCapabilities { - if found = equality.Semantic.DeepEqual(required, curr); found { - break - } - } - if !found { - modified = true - result.RequiredDropCapabilities = append(result.RequiredDropCapabilities, required) - } - } - for _, required := range required.AllowedCapabilities { - found := false - for _, curr := range result.AllowedCapabilities { - if found = equality.Semantic.DeepEqual(required, curr); found { - break - } - } - if !found { - modified = true - result.AllowedCapabilities = append(result.AllowedCapabilities, required) - } + + // AllowPrivilegeEscalation is defaulted to true so set it as such when manifest + // does not specify it + if required.AllowPrivilegeEscalation == nil { + required.AllowPrivilegeEscalation = pointer.Bool(true) } - for _, capabilityLists := range [][][]corev1.Capability{ - {result.DefaultAddCapabilities, required.DefaultAddCapabilities}, - {result.RequiredDropCapabilities, required.RequiredDropCapabilities}, - {result.AllowedCapabilities, required.AllowedCapabilities}, - } { - if result, required := len(capabilityLists[0]), len(capabilityLists[1]); result > required { - mergedClusterState = true - } + if !(equality.Semantic.DeepEqual(result.Priority, required.Priority) && + equality.Semantic.DeepEqual(result.AllowPrivilegedContainer, required.AllowPrivilegedContainer) && + equality.Semantic.DeepEqual(result.AllowHostDirVolumePlugin, required.AllowHostDirVolumePlugin) && + equality.Semantic.DeepEqual(result.AllowHostNetwork, required.AllowHostNetwork) && + equality.Semantic.DeepEqual(result.AllowHostPorts, required.AllowHostPorts) && + equality.Semantic.DeepEqual(result.AllowHostPID, required.AllowHostPID) && + equality.Semantic.DeepEqual(result.AllowHostIPC, required.AllowHostIPC) && + equality.Semantic.DeepEqual(result.DefaultAllowPrivilegeEscalation, required.DefaultAllowPrivilegeEscalation) && + equality.Semantic.DeepEqual(result.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) && + equality.Semantic.DeepEqual(result.ReadOnlyRootFilesystem, required.ReadOnlyRootFilesystem) && + equality.Semantic.DeepEqual(result.DefaultAddCapabilities, required.DefaultAddCapabilities) && + equality.Semantic.DeepEqual(result.RequiredDropCapabilities, required.RequiredDropCapabilities) && + equality.Semantic.DeepEqual(result.AllowedCapabilities, required.AllowedCapabilities) && + equality.Semantic.DeepEqual(result.AllowedFlexVolumes, required.AllowedFlexVolumes) && + equality.Semantic.DeepEqual(result.SELinuxContext, required.SELinuxContext) && + equality.Semantic.DeepEqual(result.RunAsUser, required.RunAsUser) && + equality.Semantic.DeepEqual(result.SupplementalGroups, required.SupplementalGroups) && + equality.Semantic.DeepEqual(result.FSGroup, required.FSGroup) && + equality.Semantic.DeepEqual(result.Users, required.Users) && + equality.Semantic.DeepEqual(result.Groups, required.Groups) && + equality.Semantic.DeepEqual(result.SeccompProfiles, required.SeccompProfiles) && + equality.Semantic.DeepEqual(result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls) && + equality.Semantic.DeepEqual(result.ForbiddenSysctls, required.ForbiddenSysctls)) { + differsFromRequired = true } - setBool(&modified, &result.AllowHostDirVolumePlugin, required.AllowHostDirVolumePlugin) for _, required := range required.Volumes { found := false for _, curr := range result.Volumes { @@ -84,74 +67,13 @@ func EnsureSecurityContextConstraints(existing securityv1.SecurityContextConstra result.Volumes = append(result.Volumes, required) } } - for _, required := range required.AllowedFlexVolumes { - found := false - for _, curr := range result.AllowedFlexVolumes { - if found = equality.Semantic.DeepEqual(required.Driver, curr.Driver); found { - break - } - } - if !found { - modified = true - result.AllowedFlexVolumes = append(result.AllowedFlexVolumes, required) - } - } - - if len(result.Volumes) > len(required.Volumes) || len(result.AllowedFlexVolumes) > len(required.AllowedFlexVolumes) { - mergedClusterState = true - } - - setBool(&modified, &result.AllowHostNetwork, required.AllowHostNetwork) - setBool(&modified, &result.AllowHostPorts, required.AllowHostPorts) - setBool(&modified, &result.AllowHostPID, required.AllowHostPID) - setBool(&modified, &result.AllowHostIPC, required.AllowHostIPC) - setBoolPtr(&modified, &result.DefaultAllowPrivilegeEscalation, required.DefaultAllowPrivilegeEscalation) - - // AllowPrivilegeEscalation is optional and defaults to true if not specified, - // so we enforce default if manifest does not specify it. - if required.AllowPrivilegeEscalation == nil { - setBoolPtr(&modified, &result.AllowPrivilegeEscalation, pointer.Bool(true)) - } else { - setBoolPtr(&modified, &result.AllowPrivilegeEscalation, required.AllowPrivilegeEscalation) - } - if !equality.Semantic.DeepEqual(result.SELinuxContext, required.SELinuxContext) { - modified = true - result.SELinuxContext = required.SELinuxContext - } - if !equality.Semantic.DeepEqual(result.RunAsUser, required.RunAsUser) { - modified = true - result.RunAsUser = required.RunAsUser - } - if !equality.Semantic.DeepEqual(result.FSGroup, required.FSGroup) { - modified = true - result.FSGroup = required.FSGroup - } - if !equality.Semantic.DeepEqual(result.SupplementalGroups, required.SupplementalGroups) { - modified = true - result.SupplementalGroups = required.SupplementalGroups - } - setBool(&modified, &result.ReadOnlyRootFilesystem, required.ReadOnlyRootFilesystem) - mergeStringSlice(&modified, &result.Users, required.Users) - mergeStringSlice(&modified, &result.Groups, required.Groups) - mergeStringSlice(&modified, &result.SeccompProfiles, required.SeccompProfiles) - mergeStringSlice(&modified, &result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls) - mergeStringSlice(&modified, &result.ForbiddenSysctls, required.ForbiddenSysctls) - - for _, itemLists := range [][][]string{ - {result.Users, required.Users}, - {result.Groups, required.Groups}, - {result.SeccompProfiles, required.SeccompProfiles}, - {result.AllowedUnsafeSysctls, required.AllowedUnsafeSysctls}, - {result.ForbiddenSysctls, required.ForbiddenSysctls}, - } { - if result, required := len(itemLists[0]), len(itemLists[1]); result > required { - mergedClusterState = true - } + if len(result.Volumes) > len(required.Volumes) { + differsFromRequired = true } if modified { - return result, mergedClusterState + return result, differsFromRequired } - return nil, mergedClusterState + return nil, differsFromRequired } diff --git a/lib/resourcemerge/security_test.go b/lib/resourcemerge/security_test.go index 32de03a35..7b9ab41c0 100644 --- a/lib/resourcemerge/security_test.go +++ b/lib/resourcemerge/security_test.go @@ -45,165 +45,246 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { existing securityv1.SecurityContextConstraints required securityv1.SecurityContextConstraints - expected *securityv1.SecurityContextConstraints - expectedMergedClusterState bool + expected *securityv1.SecurityContextConstraints + expectedDiffersFromRequired bool }{ { - name: "no reconcile needed when existing is equal to required", - existing: *defaulted(*restrictedv2.DeepCopy()), - required: *defaulted(*restrictedv2.DeepCopy()), - expectedMergedClusterState: false, - }, - { - name: "enforce primitive fields, primitives cannot be merged", - existing: securityv1.SecurityContextConstraints{ - Priority: pointer.Int32(123), - AllowPrivilegedContainer: true, - AllowHostDirVolumePlugin: false, - AllowHostNetwork: true, - AllowHostPorts: false, - AllowHostPID: true, - AllowHostIPC: false, - DefaultAllowPrivilegeEscalation: pointer.Bool(true), - AllowPrivilegeEscalation: pointer.Bool(false), - ReadOnlyRootFilesystem: true, - }, + name: "no reconcile needed when existing is equal to required", + existing: *defaulted(*restrictedv2.DeepCopy()), + required: *defaulted(*restrictedv2.DeepCopy()), + expectedDiffersFromRequired: false, + }, + { + name: "primitive field Priority is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{Priority: pointer.Int32(123)}), + required: securityv1.SecurityContextConstraints{Priority: pointer.Int32(42)}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowPrivilegedContainer is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowPrivilegedContainer: true}), + required: securityv1.SecurityContextConstraints{AllowPrivilegedContainer: false}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowHostDirVolumePlugin is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowHostDirVolumePlugin: false}), + required: securityv1.SecurityContextConstraints{AllowHostDirVolumePlugin: true}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowHostNetwork is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowHostNetwork: true}), + required: securityv1.SecurityContextConstraints{AllowHostNetwork: false}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowHostPorts is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowHostPorts: false}), + required: securityv1.SecurityContextConstraints{AllowHostPorts: true}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowHostPID is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowHostPID: true}), + required: securityv1.SecurityContextConstraints{AllowHostPID: false}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowHostIPC is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowHostIPC: false}), + required: securityv1.SecurityContextConstraints{AllowHostIPC: true}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field DefaultAllowPrivilegeEscalation is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{DefaultAllowPrivilegeEscalation: pointer.Bool(true)}), + required: securityv1.SecurityContextConstraints{DefaultAllowPrivilegeEscalation: pointer.Bool(false)}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field AllowPrivilegeEscalation is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: pointer.Bool(false)}), + required: securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: pointer.Bool(true)}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "primitive field ReadOnlyRootFilesystem is not reconciled but reported as different from required", + existing: *defaulted(securityv1.SecurityContextConstraints{ReadOnlyRootFilesystem: true}), + required: securityv1.SecurityContextConstraints{ReadOnlyRootFilesystem: false}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "allowPrivilegeEscalation is not reconciled to default true when not specified, but reported as different from required", + existing: securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: pointer.Bool(false)}, + required: securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: nil}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "allowPrivilegeEscalation does not need to be reconciled when existing state is true (default)", + existing: securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: pointer.Bool(true)}, + required: securityv1.SecurityContextConstraints{AllowPrivilegeEscalation: nil}, + expectedDiffersFromRequired: false, + }, + { + name: "DefaultAddCapabilities are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B"}}), + required: securityv1.SecurityContextConstraints{DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "REQUIRED_A", "REQUIRED_B"}}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "RequiredDropCapabilities are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D"}}), + required: securityv1.SecurityContextConstraints{RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "REQUIRED_C", "REQUIRED_D"}}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "AllowedCapabilities are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F"}}), + required: securityv1.SecurityContextConstraints{AllowedCapabilities: []corev1.Capability{"SHARED_C", "REQUIRED_E", "REQUIRED_F"}}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "merge Volumes, merges are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret}, + }), required: securityv1.SecurityContextConstraints{ - Priority: pointer.Int32(42), - AllowPrivilegedContainer: false, - AllowHostDirVolumePlugin: true, - AllowHostNetwork: false, - AllowHostPorts: true, - AllowHostPID: false, - AllowHostIPC: true, - DefaultAllowPrivilegeEscalation: pointer.Bool(false), - AllowPrivilegeEscalation: pointer.Bool(true), - ReadOnlyRootFilesystem: false, + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, }, - expected: &securityv1.SecurityContextConstraints{ - Priority: pointer.Int32(42), - AllowPrivilegedContainer: false, - AllowHostDirVolumePlugin: true, - AllowHostNetwork: false, - AllowHostPorts: true, - AllowHostPID: false, - AllowHostIPC: true, - DefaultAllowPrivilegeEscalation: pointer.Bool(false), - AllowPrivilegeEscalation: pointer.Bool(true), - ReadOnlyRootFilesystem: false, - }, - expectedMergedClusterState: false, + expected: defaulted(securityv1.SecurityContextConstraints{ + Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, + }), + expectedDiffersFromRequired: true, }, { - name: "enforce allowPrivilegeEscalation as default true when not specified", - existing: securityv1.SecurityContextConstraints{ - AllowPrivilegeEscalation: pointer.Bool(false), - }, + name: "AllowedFlexVolumes are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}}, + }), required: securityv1.SecurityContextConstraints{ - AllowPrivilegeEscalation: nil, - }, - expected: &securityv1.SecurityContextConstraints{ - AllowPrivilegeEscalation: pointer.Bool(true), + AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "required-1"}, {Driver: "required-2"}}, }, - expectedMergedClusterState: false, + expectedDiffersFromRequired: true, }, { - name: "allowPrivilegeEscalation does not need to be reconciled when existing state is true (default)", - existing: securityv1.SecurityContextConstraints{ - AllowPrivilegeEscalation: pointer.Bool(true), - }, + name: "SELinuxContext is not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs, SELinuxOptions: &corev1.SELinuxOptions{User: "ooser"}}, + }), required: securityv1.SecurityContextConstraints{ - AllowPrivilegeEscalation: nil, + SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, }, - expectedMergedClusterState: false, + expected: nil, + expectedDiffersFromRequired: true, }, { - name: "enforce capabilities, merges are tracked", + name: "RunAsUser is not reconciled but differences are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ - DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B"}, - RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D"}, - AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F"}, + RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyRunAsAny}, }), required: securityv1.SecurityContextConstraints{ - DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "REQUIRED_A", "REQUIRED_B"}, - RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "REQUIRED_C", "REQUIRED_D"}, - AllowedCapabilities: []corev1.Capability{"SHARED_C", "REQUIRED_E", "REQUIRED_F"}, + RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, }, - expected: defaulted(securityv1.SecurityContextConstraints{ - DefaultAddCapabilities: []corev1.Capability{"SHARED_A", "EXISTING_A", "EXISTING_B", "REQUIRED_A", "REQUIRED_B"}, - RequiredDropCapabilities: []corev1.Capability{"SHARED_B", "EXISTING_C", "EXISTING_D", "REQUIRED_C", "REQUIRED_D"}, - AllowedCapabilities: []corev1.Capability{"SHARED_C", "EXISTING_E", "EXISTING_F", "REQUIRED_E", "REQUIRED_F"}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "SupplementalGroups is not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyMustRunAs}, }), - expectedMergedClusterState: true, + required: securityv1.SecurityContextConstraints{ + SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, + }, + expected: nil, + expectedDiffersFromRequired: true, }, { - name: "enforce volumes, merges are tracked", + name: "FSGroup is not reconciled but differences are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ - Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret}, - AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}}, + FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyRunAsAny}, }), required: securityv1.SecurityContextConstraints{ - Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, - AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "required-1"}, {Driver: "required-2"}}, + FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, }, - expected: defaulted(securityv1.SecurityContextConstraints{ - Volumes: []securityv1.FSType{securityv1.FSTypeAzureFile, securityv1.FSTypeEphemeral, securityv1.FSTypeSecret, securityv1.FSProjected, securityv1.FSTypePersistentVolumeClaim}, - AllowedFlexVolumes: []securityv1.AllowedFlexVolume{{Driver: "shared"}, {Driver: "existing-1"}, {Driver: "existing-2"}, {Driver: "required-1"}, {Driver: "required-2"}}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "Users are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + Users: []string{"shared-u", "existing-user-1", "existing-user-2"}, }), - expectedMergedClusterState: true, + required: securityv1.SecurityContextConstraints{ + Users: []string{"shared-u", "required-user-1", "required-user-2"}, + }, + expected: nil, + expectedDiffersFromRequired: true, }, { - name: "enforce strategy options, cannot be merged", + name: "Groups are not reconciled but differences are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ - SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyMustRunAs, SELinuxOptions: &corev1.SELinuxOptions{User: "ooser"}}, - RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyRunAsAny}, - SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyMustRunAs}, - FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyRunAsAny}, + Groups: []string{"shared-g", "existing-group-1", "existing-group-2"}, }), required: securityv1.SecurityContextConstraints{ - SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, - RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, - SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, - FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, + Groups: []string{"shared-g", "required-group-1", "required-group-2"}, }, - expected: defaulted(securityv1.SecurityContextConstraints{ - SELinuxContext: securityv1.SELinuxContextStrategyOptions{Type: securityv1.SELinuxStrategyRunAsAny}, - RunAsUser: securityv1.RunAsUserStrategyOptions{Type: securityv1.RunAsUserStrategyMustRunAsNonRoot}, - SupplementalGroups: securityv1.SupplementalGroupsStrategyOptions{Type: securityv1.SupplementalGroupsStrategyRunAsAny}, - FSGroup: securityv1.FSGroupStrategyOptions{Type: securityv1.FSGroupStrategyMustRunAs}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "SeccompProfiles are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2"}, }), - expectedMergedClusterState: false, + required: securityv1.SecurityContextConstraints{ + SeccompProfiles: []string{"shared-s", "required-seccomp-1", "required-seccomp-2"}, + }, + expected: nil, + expectedDiffersFromRequired: true, }, { - name: "enforce users, groups, seccomp profiles, sysctls, merges are tracked", + name: "AllowedUnsafeSysctls are not reconciled but differences are tracked", existing: *defaulted(securityv1.SecurityContextConstraints{ - Users: []string{"shared-u", "existing-user-1", "existing-user-2"}, - Groups: []string{"shared-g", "existing-group-1", "existing-group-2"}, - SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2"}, AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2"}, - ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2"}, }), required: securityv1.SecurityContextConstraints{ - Users: []string{"shared-u", "required-user-1", "required-user-2"}, - Groups: []string{"shared-g", "required-group-1", "required-group-2"}, - SeccompProfiles: []string{"shared-s", "required-seccomp-1", "required-seccomp-2"}, AllowedUnsafeSysctls: []string{"shared-a", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, - ForbiddenSysctls: []string{"shared-f", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, }, - expected: defaulted(securityv1.SecurityContextConstraints{ - Users: []string{"shared-u", "existing-user-1", "existing-user-2", "required-user-1", "required-user-2"}, - Groups: []string{"shared-g", "existing-group-1", "existing-group-2", "required-group-1", "required-group-2"}, - SeccompProfiles: []string{"shared-s", "existing-seccomp-1", "existing-seccomp-2", "required-seccomp-1", "required-seccomp-2"}, - AllowedUnsafeSysctls: []string{"shared-a", "existing-unsafe-sysctl-1", "existing-unsafe-sysctl-2", "required-unsafe-sysctl-1", "required-unsafe-sysctl-2"}, - ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, + expected: nil, + expectedDiffersFromRequired: true, + }, + { + name: "ForbiddenSysctls are not reconciled but differences are tracked", + existing: *defaulted(securityv1.SecurityContextConstraints{ + ForbiddenSysctls: []string{"shared-f", "existing-forbidden-sysctl-1", "existing-forbidden-sysctl-2"}, }), - expectedMergedClusterState: true, + required: securityv1.SecurityContextConstraints{ + ForbiddenSysctls: []string{"shared-f", "required-forbidden-sysctl-1", "required-forbidden-sysctl-2"}, + }, + expected: nil, + expectedDiffersFromRequired: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - result, mergedClusterState := EnsureSecurityContextConstraints(tc.existing, tc.required) + result, differsFromRequired := EnsureSecurityContextConstraints(tc.existing, tc.required) origExisting, origRequired := tc.existing.DeepCopy(), tc.required.DeepCopy() @@ -218,8 +299,8 @@ func TestEnsureSecurityContextConstraints(t *testing.T) { t.Errorf("Unexpected side effect on required state:\n%s", diff) } - if mergedClusterState != tc.expectedMergedClusterState { - t.Errorf("expected mergedClusterState %v, got %v", tc.expectedMergedClusterState, mergedClusterState) + if differsFromRequired != tc.expectedDiffersFromRequired { + t.Errorf("expected differsFromRequired=%T, got %v", tc.expectedDiffersFromRequired, differsFromRequired) } }) } From 4855dd5cc15e26dd04d0debdaa2231506ef77d0a Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 20 Oct 2023 16:04:11 +0200 Subject: [PATCH 7/7] Do not set Upgradeable=False on modified SCCs, only warn in logs --- lib/resourceapply/security.go | 55 ++--------------------------------- pkg/cvo/upgradeable.go | 21 ------------- 2 files changed, 2 insertions(+), 74 deletions(-) diff --git a/lib/resourceapply/security.go b/lib/resourceapply/security.go index 69675b5e4..72a94e215 100644 --- a/lib/resourceapply/security.go +++ b/lib/resourceapply/security.go @@ -2,9 +2,6 @@ package resourceapply import ( "context" - "sort" - "sync" - "time" "github.com/google/go-cmp/cmp" @@ -17,54 +14,6 @@ import ( "github.com/openshift/cluster-version-operator/lib/resourcemerge" ) -type modifiedSccTracker struct { - sync.Mutex - lastUpdated time.Time - names map[string]time.Time -} - -var modifiedSccs modifiedSccTracker - -func init() { - modifiedSccs = modifiedSccTracker{ - lastUpdated: time.Now().UTC(), - names: make(map[string]time.Time), - } -} - -// ModifiedSCCs returns the list of SCCs that have been modified in the cluster -func ModifiedSCCs() []string { - modifiedSccs.Lock() - defer modifiedSccs.Unlock() - names := make([]string, 0, len(modifiedSccs.names)) - - // discard any records older than 10 minutes before last cache write, they are - // likely stale (this should not happen, but we are being defensive) - staleThreshold := modifiedSccs.lastUpdated.Add(-10 * time.Minute) - for name, saw := range modifiedSccs.names { - if saw.Before(staleThreshold) { - delete(modifiedSccs.names, name) - } else { - names = append(names, name) - } - } - - sort.Strings(names) - return names -} - -// markSccModifiedInCluster marks the SCC as modified in the cluster -func trackModifiedScc(name string, modified bool) { - modifiedSccs.Lock() - defer modifiedSccs.Unlock() - modifiedSccs.lastUpdated = time.Now().UTC() - if modified { - modifiedSccs.names[name] = modifiedSccs.lastUpdated - } else { - delete(modifiedSccs.names, name) - } -} - // ApplySecurityContextConstraintsv1 applies the required SecurityContextConstraints to the cluster. func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclientv1.SecurityContextConstraintsGetter, required *securityv1.SecurityContextConstraints, reconciling bool) (*securityv1.SecurityContextConstraints, bool, error) { existing, err := client.SecurityContextConstraints().Get(ctx, required.Name, metav1.GetOptions{}) @@ -83,8 +32,8 @@ func ApplySecurityContextConstraintsv1(ctx context.Context, client securityclien reconcile, requiredMerges := resourcemerge.EnsureSecurityContextConstraints(*existing, *required) - if reconciling { - trackModifiedScc(required.Name, requiredMerges) + 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 { diff --git a/pkg/cvo/upgradeable.go b/pkg/cvo/upgradeable.go index e08954074..cc8804f79 100644 --- a/pkg/cvo/upgradeable.go +++ b/pkg/cvo/upgradeable.go @@ -21,7 +21,6 @@ import ( "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" - "github.com/openshift/cluster-version-operator/lib/resourceapply" "github.com/openshift/cluster-version-operator/lib/resourcedelete" "github.com/openshift/cluster-version-operator/lib/resourcemerge" "github.com/openshift/cluster-version-operator/pkg/internal" @@ -428,7 +427,6 @@ func (optr *Operator) defaultUpgradeableChecks() []upgradeableCheck { }, &clusterOperatorsUpgradeable{coLister: optr.coLister}, &clusterManifestDeleteInProgressUpgradeable{}, - &modifiedSccDetectedUpgradeable{}, } } @@ -496,22 +494,3 @@ func (intervals *upgradeableCheckIntervals) throttlePeriod(cv *configv1.ClusterV } return intervals.min } - -type modifiedSccDetectedUpgradeable struct{} - -func (m modifiedSccDetectedUpgradeable) Check() *configv1.ClusterOperatorStatusCondition { - modifiedSccs := resourceapply.ModifiedSCCs() - if len(modifiedSccs) == 0 { - return nil - } - - resources := strings.Join(modifiedSccs, ",") - klog.V(2).Infof("Found SCC resources in the cluster with elements not present in payload manifest; resources=%s", resources) - - return &configv1.ClusterOperatorStatusCondition{ - Type: configv1.ClusterStatusConditionType("ModifiedSccResources"), - Reason: "DetectedModifiedSccResources", - Status: configv1.ConditionTrue, - Message: fmt.Sprintf("Detected modified SecurityContextConstraints: %s. These modifications would be removed by the update. Please ensure all cluster workloads are able to run without these usupported modifications, then delete the modified SCC resources (they will be recreated without the modifications). See https://access.redhat.com/solutions/7033949 for more information.", resources), - } -}