Skip to content

OCPBUGS-11933: Ignore informative annotations upon admission #117

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 13, 2024
Merged

OCPBUGS-11933: Ignore informative annotations upon admission #117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e48eea
pkg/scc/sccadmission: fix argument order, new object comes first
liouk Jul 14, 2023
8e50815
pkg/scc/sccadmission: ignore known informative annotation additions u…
liouk Jul 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 63 additions & 7 deletions pkg/securitycontextconstraints/sccadmission/admission.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

once vertical scaling is a thing, we will ignore some. Just be ready.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is #128 legit, too?

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()
Expand Down
97 changes: 93 additions & 4 deletions pkg/securitycontextconstraints/sccadmission/admission_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -222,7 +223,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"),
Expand All @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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()

Expand Down