From 7bd78b06e93dfab400fa56d6b1caa3064fcd7877 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Wed, 6 Mar 2024 23:01:48 -0800 Subject: [PATCH] Warn on deprecated AppArmor annotation use --- pkg/api/pod/warnings.go | 15 +++++ pkg/api/pod/warnings_test.go | 88 ++++++++++++++++++++++++++ pkg/registry/core/pod/strategy.go | 18 +++++- pkg/registry/core/pod/strategy_test.go | 41 ++++++++++-- 4 files changed, 154 insertions(+), 8 deletions(-) diff --git a/pkg/api/pod/warnings.go b/pkg/api/pod/warnings.go index 59c2fe85b4b..8958aa51a47 100644 --- a/pkg/api/pod/warnings.go +++ b/pkg/api/pod/warnings.go @@ -24,10 +24,12 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" + utilfeature "k8s.io/apiserver/pkg/util/feature" nodeapi "k8s.io/kubernetes/pkg/api/node" pvcutil "k8s.io/kubernetes/pkg/api/persistentvolumeclaim" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/pods" + "k8s.io/kubernetes/pkg/features" ) func GetWarningsForPod(ctx context.Context, pod, oldPod *api.Pod) []string { @@ -212,6 +214,7 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta warnings = append(warnings, fmt.Sprintf(`%s: non-functional in v1.27+; use the "seccompProfile" field instead`, fieldPath.Child("metadata", "annotations").Key(api.SeccompPodAnnotationKey))) } } + hasPodAppArmorProfile := podSpec.SecurityContext != nil && podSpec.SecurityContext.AppArmorProfile != nil pods.VisitContainersWithPath(podSpec, fieldPath.Child("spec"), func(c *api.Container, p *field.Path) bool { // use of container seccomp annotation without accompanying field @@ -221,6 +224,18 @@ func warningsForPodSpecAndMeta(fieldPath *field.Path, podSpec *api.PodSpec, meta } } + // use of container AppArmor annotation without accompanying field + if utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) { + isPodTemplate := fieldPath != nil // Pod warnings are emitted through applyAppArmorVersionSkew instead. + hasAppArmorField := hasPodAppArmorProfile || (c.SecurityContext != nil && c.SecurityContext.AppArmorProfile != nil) + if isPodTemplate && !hasAppArmorField { + key := api.DeprecatedAppArmorAnnotationKeyPrefix + c.Name + if _, exists := meta.Annotations[key]; exists { + warnings = append(warnings, fmt.Sprintf(`%s: deprecated since v1.30; use the "appArmorProfile" field instead`, fieldPath.Child("metadata", "annotations").Key(key))) + } + } + } + // fractional memory/ephemeral-storage requests/limits (#79950, #49442, #18538) if value, ok := c.Resources.Limits[api.ResourceMemory]; ok && value.MilliValue()%int64(1000) != int64(0) { warnings = append(warnings, fmt.Sprintf("%s: fractional byte value %q is invalid, must be an integer", p.Child("resources", "limits").Key(string(api.ResourceMemory)), value.String())) diff --git a/pkg/api/pod/warnings_test.go b/pkg/api/pod/warnings_test.go index dd15a700410..6405977b7cb 100644 --- a/pkg/api/pod/warnings_test.go +++ b/pkg/api/pod/warnings_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/apimachinery/pkg/util/validation/field" api "k8s.io/kubernetes/pkg/apis/core" utilpointer "k8s.io/utils/pointer" ) @@ -1095,3 +1096,90 @@ func TestWarnings(t *testing.T) { }) } } + +func TestTemplateOnlyWarnings(t *testing.T) { + testcases := []struct { + name string + template *api.PodTemplateSpec + oldTemplate *api.PodTemplateSpec + expected []string + }{ + { + name: "annotations", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + `container.apparmor.security.beta.kubernetes.io/foo`: `unconfined`, + }}, + Spec: api.PodSpec{Containers: []api.Container{{Name: "foo"}}}, + }, + expected: []string{ + `template.metadata.annotations[container.apparmor.security.beta.kubernetes.io/foo]: deprecated since v1.30; use the "appArmorProfile" field instead`, + }, + }, + { + name: "AppArmor pod field", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + `container.apparmor.security.beta.kubernetes.io/foo`: `unconfined`, + }}, + Spec: api.PodSpec{ + SecurityContext: &api.PodSecurityContext{ + AppArmorProfile: &api.AppArmorProfile{Type: api.AppArmorProfileTypeUnconfined}, + }, + Containers: []api.Container{{ + Name: "foo", + }}, + }, + }, + expected: []string{}, + }, + { + name: "AppArmor container field", + template: &api.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + `container.apparmor.security.beta.kubernetes.io/foo`: `unconfined`, + }}, + Spec: api.PodSpec{ + Containers: []api.Container{{ + Name: "foo", + SecurityContext: &api.SecurityContext{ + AppArmorProfile: &api.AppArmorProfile{Type: api.AppArmorProfileTypeUnconfined}, + }, + }}, + }, + }, + expected: []string{}, + }, + } + + for _, tc := range testcases { + t.Run("podspec_"+tc.name, func(t *testing.T) { + var oldTemplate *api.PodTemplateSpec + if tc.oldTemplate != nil { + oldTemplate = tc.oldTemplate + } + actual := sets.New[string](GetWarningsForPodTemplate(context.TODO(), field.NewPath("template"), tc.template, oldTemplate)...) + expected := sets.New[string](tc.expected...) + for _, missing := range sets.List[string](expected.Difference(actual)) { + t.Errorf("missing: %s", missing) + } + for _, extra := range sets.List[string](actual.Difference(expected)) { + t.Errorf("extra: %s", extra) + } + }) + + t.Run("pod_"+tc.name, func(t *testing.T) { + var pod *api.Pod + if tc.template != nil { + pod = &api.Pod{ + ObjectMeta: tc.template.ObjectMeta, + Spec: tc.template.Spec, + } + } + actual := GetWarningsForPod(context.TODO(), pod, &api.Pod{}) + if len(actual) > 0 { + t.Errorf("unexpected template-only warnings on pod: %v", actual) + } + }) + } +} diff --git a/pkg/registry/core/pod/strategy.go b/pkg/registry/core/pod/strategy.go index 0ce8332a4a5..5b464e1503a 100644 --- a/pkg/registry/core/pod/strategy.go +++ b/pkg/registry/core/pod/strategy.go @@ -41,6 +41,7 @@ import ( "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/apiserver/pkg/warning" "k8s.io/client-go/tools/cache" "k8s.io/kubernetes/pkg/api/legacyscheme" podutil "k8s.io/kubernetes/pkg/api/pod" @@ -92,7 +93,7 @@ func (podStrategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { applySchedulingGatedCondition(pod) mutatePodAffinity(pod) - applyAppArmorVersionSkew(pod) + applyAppArmorVersionSkew(ctx, pod) } // PrepareForUpdate clears fields that are not allowed to be set by end users on update. @@ -763,7 +764,7 @@ func applySchedulingGatedCondition(pod *api.Pod) { // applyAppArmorVersionSkew implements the version skew behavior described in: // https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/24-apparmor#version-skew-strategy -func applyAppArmorVersionSkew(pod *api.Pod) { +func applyAppArmorVersionSkew(ctx context.Context, pod *api.Pod) { if !utilfeature.DefaultFeatureGate.Enabled(features.AppArmorFields) { return } @@ -811,12 +812,25 @@ func applyAppArmorVersionSkew(pod *api.Pod) { newField = nil } + // warn if we had an annotation that we couldn't derive a valid field from + deprecationWarning := newField == nil + // Only copy the annotation to the field if it is different from the pod-level profile. if newField != nil && !apiequality.Semantic.DeepEqual(newField, podProfile) { if ctr.SecurityContext == nil { ctr.SecurityContext = &api.SecurityContext{} } ctr.SecurityContext.AppArmorProfile = newField + // warn if there was an annotation without a corresponding field + deprecationWarning = true + } + + if deprecationWarning { + // Note: annotation deprecation warning must be added here rather than the + // typical WarningsOnCreate path to emit the warning before syncing the + // annotations & fields. + fldPath := field.NewPath("metadata", "annotations").Key(key) + warning.AddWarning(ctx, "", fmt.Sprintf(`%s: deprecated since v1.30; use the "appArmorProfile" field instead`, fldPath)) } } diff --git a/pkg/registry/core/pod/strategy_test.go b/pkg/registry/core/pod/strategy_test.go index 9b28597b107..8b22cc29e34 100644 --- a/pkg/registry/core/pod/strategy_test.go +++ b/pkg/registry/core/pod/strategy_test.go @@ -38,6 +38,7 @@ import ( "k8s.io/apimachinery/pkg/types" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/apiserver/pkg/warning" "k8s.io/client-go/tools/cache" featuregatetesting "k8s.io/component-base/featuregate/testing" utilpointer "k8s.io/utils/pointer" @@ -2113,9 +2114,10 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { testProfile := "test" tests := []struct { - description string - pod *api.Pod - validation func(*testing.T, *api.Pod) + description string + pod *api.Pod + validation func(*testing.T, *api.Pod) + expectWarning bool }{{ description: "Security context nil", pod: &api.Pod{ @@ -2361,6 +2363,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Nil(t, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.LocalhostProfile) assert.Nil(t, pod.Spec.SecurityContext) }, + expectWarning: true, }, { description: "Annotation for non-existent container", pod: &api.Pod{ @@ -2408,6 +2411,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Nil(t, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.LocalhostProfile) assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.SecurityContext.AppArmorProfile.Type) }, + expectWarning: true, }, { description: "Multiple containers by annotations", pod: &api.Pod{ @@ -2446,6 +2450,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Nil(t, pod.Spec.Containers[2].SecurityContext) assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type) }, + expectWarning: true, }, { description: "Conflicting field and annotations", pod: &api.Pod{ @@ -2526,6 +2531,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Equal(t, api.AppArmorProfileTypeRuntimeDefault, pod.Spec.SecurityContext.AppArmorProfile.Type) assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.Containers[0].SecurityContext.AppArmorProfile.Type) }, + expectWarning: true, }, { description: "Mixed annotations and fields", pod: &api.Pod{ @@ -2565,12 +2571,13 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Equal(t, api.AppArmorProfileTypeUnconfined, pod.Spec.Containers[1].SecurityContext.AppArmorProfile.Type) assert.Nil(t, pod.Spec.Containers[2].SecurityContext) }, + expectWarning: true, }, { description: "Invalid annotation value", pod: &api.Pod{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - api.DeprecatedAppArmorAnnotationKeyPrefix + "ctr": "not-a-real-type", + api.DeprecatedAppArmorAnnotationKeyPrefix + "ctr": "localhost/", }, }, Spec: api.PodSpec{ @@ -2579,11 +2586,12 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { }, validation: func(t *testing.T, pod *api.Pod) { assert.Equal(t, map[string]string{ - api.DeprecatedAppArmorAnnotationKeyPrefix + "ctr": "not-a-real-type", + api.DeprecatedAppArmorAnnotationKeyPrefix + "ctr": "localhost/", }, pod.Annotations) assert.Nil(t, pod.Spec.Containers[0].SecurityContext) assert.Nil(t, pod.Spec.SecurityContext) }, + expectWarning: true, }, { description: "Invalid localhost annotation", pod: &api.Pod{ @@ -2601,6 +2609,7 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { assert.Nil(t, pod.Spec.Containers[0].SecurityContext) assert.Nil(t, pod.Spec.SecurityContext) }, + expectWarning: true, }, { description: "Invalid field type", pod: &api.Pod{ @@ -2640,8 +2649,28 @@ func TestApplyAppArmorVersionSkew(t *testing.T) { for _, test := range tests { t.Run(test.description, func(t *testing.T) { - applyAppArmorVersionSkew(test.pod) + warnings := &warningRecorder{} + ctx := warning.WithWarningRecorder(context.Background(), warnings) + applyAppArmorVersionSkew(ctx, test.pod) test.validation(t, test.pod) + + if test.expectWarning { + if assert.NotEmpty(t, warnings.warnings, "expect warnings") { + assert.Contains(t, warnings.warnings[0], `deprecated since v1.30; use the "appArmorProfile" field instead`) + } + } else { + assert.Empty(t, warnings.warnings, "shouldn't emit a warning") + } }) } } + +type warningRecorder struct { + warnings []string +} + +var _ warning.Recorder = &warningRecorder{} + +func (w *warningRecorder) AddWarning(_, text string) { + w.warnings = append(w.warnings, text) +}