From 4e48eeac24acb501512b888890c96dc211d314e0 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Fri, 14 Jul 2023 13:00:42 +0300 Subject: [PATCH 1/2] pkg/scc/sccadmission: fix argument order, new object comes first --- pkg/securitycontextconstraints/sccadmission/admission_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/securitycontextconstraints/sccadmission/admission_test.go b/pkg/securitycontextconstraints/sccadmission/admission_test.go index ccd510815..47a89d4e8 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission_test.go +++ b/pkg/securitycontextconstraints/sccadmission/admission_test.go @@ -222,7 +222,7 @@ func TestShouldIgnore(t *testing.T) { updatedPod := mutate(p.DeepCopy()) return admission.NewAttributesRecord( - p, updatedPod, + updatedPod, p, coreapi.Kind("Pod").WithVersion("version"), p.Namespace, p.Name, coreapi.Resource("pods").WithVersion("version"), From 8e50815fce1f544ab439874199bdc0be048d4169 Mon Sep 17 00:00:00 2001 From: Ilias Rinis Date: Thu, 13 Jul 2023 17:52:49 +0300 Subject: [PATCH 2/2] pkg/scc/sccadmission: ignore known informative annotation additions upon admission --- .../sccadmission/admission.go | 70 ++++++++++++-- .../sccadmission/admission_test.go | 95 ++++++++++++++++++- 2 files changed, 155 insertions(+), 10 deletions(-) diff --git a/pkg/securitycontextconstraints/sccadmission/admission.go b/pkg/securitycontextconstraints/sccadmission/admission.go index 01120a173..701d978f7 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission.go +++ b/pkg/securitycontextconstraints/sccadmission/admission.go @@ -9,6 +9,7 @@ import ( "time" apiequality "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/labels" kutilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" @@ -28,10 +29,11 @@ import ( rbacregistry "k8s.io/kubernetes/pkg/registry/rbac" securityv1 "github.com/openshift/api/security/v1" - "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sccmatching" - sccsort "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util/sort" securityv1informer "github.com/openshift/client-go/security/informers/externalversions/security/v1" securityv1listers "github.com/openshift/client-go/security/listers/security/v1" + + "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sccmatching" + sccsort "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util/sort" ) const PluginName = "security.openshift.io/SecurityContextConstraint" @@ -470,6 +472,10 @@ var ignoredSubresources = sets.NewString( "status", ) +var ignoredAnnotations = sets.NewString( + "k8s.ovn.org/pod-networks", +) + func shouldIgnore(a admission.Attributes) (bool, error) { if a.GetResource().GroupResource() != coreapi.Resource("pods") { return true, nil @@ -491,16 +497,66 @@ func shouldIgnore(a admission.Attributes) (bool, error) { if pod.Spec.OS != nil && pod.Spec.OS.Name == coreapi.Windows { return true, nil } - // if this is an update, see if we are only updating the ownerRef. Garbage collection does this - // and we should allow it in general, since you had the power to update and the power to delete. - // The worst that happens is that you delete something, but you aren't controlling the privileged object itself - if a.GetOperation() == admission.Update && rbacregistry.IsOnlyMutatingGCFields(a.GetObject(), a.GetOldObject(), kapihelper.Semantic) { - return true, nil + + if a.GetOperation() == admission.Update { + oldPod, ok := a.GetOldObject().(*coreapi.Pod) + if !ok { + return false, admission.NewForbidden(a, fmt.Errorf("object was marked as kind pod but was unable to be converted: %v", a.GetOldObject())) + } + + // never ignore any spec changes + if !kapihelper.Semantic.DeepEqual(pod.Spec, oldPod.Spec) { + return false, nil + } + + // see if we are only doing meta changes that should be ignored during admission + // for example, the OVN controller adds informative networking annotations that shouldn't cause the pod to go through admission again + if shouldIgnoreMetaChanges(pod, oldPod) { + return true, nil + } } return false, nil } +func shouldIgnoreMetaChanges(newPod, oldPod *coreapi.Pod) bool { + // check if we're adding or changing only annotations from the ignore list + for key, newVal := range newPod.ObjectMeta.Annotations { + if oldVal, ok := oldPod.ObjectMeta.Annotations[key]; ok && newVal == oldVal { + continue + } + + if !ignoredAnnotations.Has(key) { + return false + } + } + + // check if we're removing only annotations from the ignore list + for key := range oldPod.ObjectMeta.Annotations { + if _, ok := newPod.ObjectMeta.Annotations[key]; ok { + continue + } + + if !ignoredAnnotations.Has(key) { + return false + } + } + + newPodCopy := newPod.DeepCopyObject() + newPodCopyMeta, err := meta.Accessor(newPodCopy) + if err != nil { + return false + } + newPodCopyMeta.SetAnnotations(oldPod.ObjectMeta.Annotations) + + // see if we are only updating the ownerRef. Garbage collection does this + // and we should allow it in general, since you had the power to update and the power to delete. + // The worst that happens is that you delete something, but you aren't controlling the privileged object itself + res := rbacregistry.IsOnlyMutatingGCFields(newPodCopy, oldPod, kapihelper.Semantic) + + return res +} + // SetSecurityInformers implements WantsSecurityInformer interface for constraint. func (c *constraint) SetSecurityInformers(informers securityv1informer.SecurityContextConstraintsInformer) { c.sccLister = informers.Lister() diff --git a/pkg/securitycontextconstraints/sccadmission/admission_test.go b/pkg/securitycontextconstraints/sccadmission/admission_test.go index 47a89d4e8..5e13e83d1 100644 --- a/pkg/securitycontextconstraints/sccadmission/admission_test.go +++ b/pkg/securitycontextconstraints/sccadmission/admission_test.go @@ -24,9 +24,10 @@ import ( "k8s.io/utils/pointer" securityv1 "github.com/openshift/api/security/v1" + securityv1listers "github.com/openshift/client-go/security/listers/security/v1" + "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/sccmatching" sccsort "github.com/openshift/apiserver-library-go/pkg/securitycontextconstraints/util/sort" - securityv1listers "github.com/openshift/client-go/security/listers/security/v1" ) // createSAForTest Build and Initializes a ServiceAccount for tests @@ -240,11 +241,13 @@ func TestShouldIgnore(t *testing.T) { }) } - tests := []struct { + type testCase struct { description string shouldIgnore bool admissionAttributes admission.Attributes - }{ + } + + tests := []testCase{ { description: "Windows pod should be ignored", shouldIgnore: true, @@ -284,6 +287,22 @@ func TestShouldIgnore(t *testing.T) { }), }, } + + for _, annotation := range ignoredAnnotations.List() { + tests = append(tests, testCase{ + description: fmt.Sprintf("add ignored annotation %v", annotation), + shouldIgnore: true, + admissionAttributes: withUpdate(goodPod(), "annotations", + func(p *coreapi.Pod) *coreapi.Pod { + if p.ObjectMeta.Annotations == nil { + p.ObjectMeta.Annotations = map[string]string{} + } + p.ObjectMeta.Annotations[annotation] = "somevalue" + return p + }), + }) + } + for _, test := range tests { t.Run(test.description, func(t *testing.T) { ignored, err := shouldIgnore(test.admissionAttributes) @@ -297,6 +316,76 @@ func TestShouldIgnore(t *testing.T) { } } +func TestShouldIgnoreMetaChanges(t *testing.T) { + emptyMeta := metav1.ObjectMeta{} + emptyAnno := metav1.ObjectMeta{Annotations: map[string]string{}} + baseMeta := metav1.ObjectMeta{ + Annotations: map[string]string{"aaa": "aaa", "bbb": "bbb"}, + ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "manager"}}, + OwnerReferences: []metav1.OwnerReference{{Name: "foo"}}, + Finalizers: []string{"final"}, + } + + ignoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "pod-networks-here", "aaa": "aaa", "bbb": "bbb"}} + onlyIgnoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "pod-networks-here"}} + changedIgnoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "pod-networks-here-indeed", "aaa": "aaa", "bbb": "bbb"}} + nonIgnoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"aaa": "aaa", "bbb": "bbb", "ccc": "ccc"}} + changedNonIgnoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"aaa": "aaa", "bbb": "bbb", "ccc": "CCC"}} + ignoredAndNonIgnoredAnno := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "pod-networks-here", "aaa": "aaa", "bbb": "bbb", "ccc": "ccc"}} + ignoredAndNonIgnoredAnnoUpdated := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "pod-networks-here-2", "aaa": "AAA", "bbb": "BBB", "ccc": "CCC"}} + managedFields := metav1.ObjectMeta{ManagedFields: []metav1.ManagedFieldsEntry{{Manager: "manager"}}} + ownerRef := metav1.ObjectMeta{OwnerReferences: []metav1.OwnerReference{{Name: "foo"}}} + ownerRefFinalizer := metav1.ObjectMeta{ + OwnerReferences: []metav1.OwnerReference{{Name: "foo"}}, + Finalizers: []string{"final"}, + } + + labelsMeta := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "net"}, Labels: map[string]string{"label1": "val1", "label2": "val2"}} + labelsMetaAdded := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "net"}, Labels: map[string]string{"label1": "val1", "label2": "val2", "label3": "val3"}} + labelsMetaRemoved := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "net"}, Labels: map[string]string{"label1": "val1"}} + labelsMetaChanged := metav1.ObjectMeta{Annotations: map[string]string{"k8s.ovn.org/pod-networks": "net"}, Labels: map[string]string{"label1": "val1", "label2": "val2-new"}} + + tests := []struct { + description string + newMeta metav1.ObjectMeta + oldMeta metav1.ObjectMeta + want bool + }{ + {"nil new and old annotations", emptyMeta, emptyMeta, true}, + {"empty new and old annotations", emptyAnno, emptyAnno, true}, + {"same annotations", baseMeta, baseMeta, true}, + {"different managedFields", emptyMeta, managedFields, true}, + {"only ownerRef", emptyMeta, ownerRef, true}, + {"ownerRef and finalizer", emptyMeta, ownerRefFinalizer, true}, + {"only ignored annotations added on nil old", onlyIgnoredAnno, emptyMeta, true}, + {"only ignored annotations added on empty old", onlyIgnoredAnno, emptyAnno, true}, + {"only ignored annotations removed", emptyMeta, onlyIgnoredAnno, true}, + {"ignored and other annotations added on nil old", ignoredAnno, emptyMeta, false}, + {"ignored and other annotations added on empty old", ignoredAnno, emptyAnno, false}, + {"ignored and other annotations removed", emptyAnno, ignoredAnno, false}, + {"ignored annotations added", ignoredAnno, baseMeta, true}, + {"ignored annotations changed", changedIgnoredAnno, ignoredAnno, true}, + {"ignored annotations removed", baseMeta, ignoredAnno, true}, + {"non-ignored annotations added", nonIgnoredAnno, baseMeta, false}, + {"non-ignored annotations changed", changedNonIgnoredAnno, nonIgnoredAnno, false}, + {"non-ignored annotations removed", baseMeta, nonIgnoredAnno, false}, + {"ignored and other annotations changed", ignoredAndNonIgnoredAnnoUpdated, ignoredAndNonIgnoredAnno, false}, + {"labels added", labelsMetaAdded, labelsMeta, false}, + {"labels removed", labelsMetaRemoved, labelsMeta, false}, + {"labels changed", labelsMetaChanged, labelsMeta, false}, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + newPod := &coreapi.Pod{ObjectMeta: test.newMeta} + oldPod := &coreapi.Pod{ObjectMeta: test.oldMeta} + if got := shouldIgnoreMetaChanges(newPod, oldPod); got != test.want { + t.Errorf("got %v; want %v; newMeta: %v; oldMeta: %v", got, test.want, test.newMeta, test.oldMeta) + } + }) + } +} + func testSCCAdmit(testCaseName string, sccs []*securityv1.SecurityContextConstraints, pod *coreapi.Pod, shouldPass bool, t *testing.T) { t.Helper()