Skip to content

Commit

Permalink
Merge pull request #123789 from tallclair/apparmor-warnings
Browse files Browse the repository at this point in the history 
Warn on deprecated AppArmor annotation use
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Mar 7, 2024
2 parents 46f017a + 7bd78b0 commit cc6d9b3
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 8 deletions.
15 changes: 15 additions & 0 deletions pkg/api/pod/warnings.go
View file
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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()))
Expand Down
88 changes: 88 additions & 0 deletions pkg/api/pod/warnings_test.go
View file
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
}
})
}
}
18 changes: 16 additions & 2 deletions pkg/registry/core/pod/strategy.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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))
}
}

Expand Down
41 changes: 35 additions & 6 deletions pkg/registry/core/pod/strategy_test.go
View file
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand Down Expand Up @@ -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)
}

0 comments on commit cc6d9b3

Please sign in to comment.