Skip to content

Commit e5d2cad

Browse files
tallclairtallclair
committed
Refactor PSP provider
1 parent 939f8ff commit e5d2cad

File tree

4 files changed

+79
-77
lines changed

4 files changed

+79
-77
lines changed

‎pkg/security/podsecuritypolicy/provider.go‎

Lines changed: 30 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -59,10 +59,10 @@ func NewSimpleProvider(psp *policy.PodSecurityPolicy, namespace string, strategy
5959
}, nil
6060
}
6161

62-
// DefaultPodSecurityContext sets the default values of the required but not filled fields.
63-
// It modifies the SecurityContext and annotations of the provided pod. Validation should be
64-
// used after the context is defaulted to ensure it complies with the required restrictions.
65-
func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error {
62+
// MutatePod sets the default values of the required but not filled fields.
63+
// Validation should be used after the context is defaulted to ensure it
64+
// complies with the required restrictions.
65+
func (s *simpleProvider) MutatePod(pod *api.Pod) error {
6666
sc := securitycontext.NewPodSecurityContextMutator(pod.Spec.SecurityContext)
6767

6868
if sc.SupplementalGroups() == nil {
@@ -104,13 +104,25 @@ func (s *simpleProvider) DefaultPodSecurityContext(pod *api.Pod) error {
104104

105105
pod.Spec.SecurityContext = sc.PodSecurityContext()
106106

107+
for i := range pod.Spec.InitContainers {
108+
if err := s.mutateContainer(pod, &pod.Spec.InitContainers[i]); err != nil {
109+
return err
110+
}
111+
}
112+
113+
for i := range pod.Spec.Containers {
114+
if err := s.mutateContainer(pod, &pod.Spec.Containers[i]); err != nil {
115+
return err
116+
}
117+
}
118+
107119
return nil
108120
}
109121

110-
// DefaultContainerSecurityContext sets the default values of the required but not filled fields.
122+
// mutateContainer sets the default values of the required but not filled fields.
111123
// It modifies the SecurityContext of the container and annotations of the pod. Validation should
112124
// be used after the context is defaulted to ensure it complies with the required restrictions.
113-
func (s *simpleProvider) DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error {
125+
func (s *simpleProvider) mutateContainer(pod *api.Pod, container *api.Container) error {
114126
sc := securitycontext.NewEffectiveContainerSecurityContextMutator(
115127
securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext),
116128
securitycontext.NewContainerSecurityContextMutator(container.SecurityContext),
@@ -282,11 +294,22 @@ func (s *simpleProvider) ValidatePod(pod *api.Pod) field.ErrorList {
282294
}
283295
}
284296
}
297+
298+
fldPath := field.NewPath("spec", "initContainers")
299+
for i := range pod.Spec.InitContainers {
300+
allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.InitContainers[i], fldPath.Index(i))...)
301+
}
302+
303+
fldPath = field.NewPath("spec", "containers")
304+
for i := range pod.Spec.Containers {
305+
allErrs = append(allErrs, s.validateContainer(pod, &pod.Spec.Containers[i], fldPath.Index(i))...)
306+
}
307+
285308
return allErrs
286309
}
287310

288311
// Ensure a container's SecurityContext is in compliance with the given constraints
289-
func (s *simpleProvider) ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList {
312+
func (s *simpleProvider) validateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList {
290313
allErrs := field.ErrorList{}
291314

292315
podSC := securitycontext.NewPodSecurityContextAccessor(pod.Spec.SecurityContext)

‎pkg/security/podsecuritypolicy/provider_test.go‎

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ import (
3131
policy "k8s.io/api/policy/v1beta1"
3232
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3333
"k8s.io/apimachinery/pkg/util/diff"
34-
"k8s.io/apimachinery/pkg/util/validation/field"
3534
api "k8s.io/kubernetes/pkg/apis/core"
3635
k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1"
3736
"k8s.io/kubernetes/pkg/security/apparmor"
@@ -41,7 +40,7 @@ import (
4140

4241
const defaultContainerName = "test-c"
4342

44-
func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
43+
func TestMutatePodNonmutating(t *testing.T) {
4544
// Create a pod with a security context that needs filling in
4645
createPod := func() *api.Pod {
4746
return &api.Pod{
@@ -89,7 +88,7 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
8988
if err != nil {
9089
t.Fatalf("unable to create provider %v", err)
9190
}
92-
err = provider.DefaultPodSecurityContext(pod)
91+
err = provider.MutatePod(pod)
9392
if err != nil {
9493
t.Fatalf("unable to create psc %v", err)
9594
}
@@ -98,14 +97,14 @@ func TestDefaultPodSecurityContextNonmutating(t *testing.T) {
9897
// since all the strategies were permissive
9998
if !reflect.DeepEqual(createPod(), pod) {
10099
diffs := diff.ObjectDiff(createPod(), pod)
101-
t.Errorf("pod was mutated by DefaultPodSecurityContext. diff:\n%s", diffs)
100+
t.Errorf("pod was mutated by MutatePod. diff:\n%s", diffs)
102101
}
103102
if !reflect.DeepEqual(createPSP(), psp) {
104-
t.Error("psp was mutated by DefaultPodSecurityContext")
103+
t.Error("psp was mutated by MutatePod")
105104
}
106105
}
107106

108-
func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
107+
func TestMutateContainerNonmutating(t *testing.T) {
109108
untrue := false
110109
tests := []struct {
111110
security *api.SecurityContext
@@ -134,7 +133,6 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
134133
Name: "psp-sa",
135134
Annotations: map[string]string{
136135
seccomp.AllowedProfilesAnnotationKey: "*",
137-
seccomp.DefaultProfileAnnotationKey: "foo",
138136
},
139137
},
140138
Spec: policy.PodSecurityPolicySpec{
@@ -165,7 +163,7 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
165163
if err != nil {
166164
t.Fatalf("unable to create provider %v", err)
167165
}
168-
err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0])
166+
err = provider.MutatePod(pod)
169167
if err != nil {
170168
t.Fatalf("unable to create container security context %v", err)
171169
}
@@ -174,15 +172,15 @@ func TestDefaultContainerSecurityContextNonmutating(t *testing.T) {
174172
// since all the strategies were permissive
175173
if !reflect.DeepEqual(createPod(), pod) {
176174
diffs := diff.ObjectDiff(createPod(), pod)
177-
t.Errorf("pod was mutated by DefaultContainerSecurityContext. diff:\n%s", diffs)
175+
t.Errorf("pod was mutated. diff:\n%s", diffs)
178176
}
179177
if !reflect.DeepEqual(createPSP(), psp) {
180-
t.Error("psp was mutated by DefaultContainerSecurityContext")
178+
t.Error("psp was mutated")
181179
}
182180
}
183181
}
184182

185-
func TestValidatePodSecurityContextFailures(t *testing.T) {
183+
func TestValidatePodFailures(t *testing.T) {
186184
failHostNetworkPod := defaultPod()
187185
failHostNetworkPod.Spec.SecurityContext.HostNetwork = true
188186

@@ -504,6 +502,7 @@ func TestValidateContainerFailures(t *testing.T) {
504502
},
505503
}
506504
failSELinuxPod := defaultPod()
505+
failSELinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"}
507506
failSELinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{
508507
Level: "bar",
509508
}
@@ -620,22 +619,24 @@ func TestValidateContainerFailures(t *testing.T) {
620619
}
621620

622621
for k, v := range errorCases {
623-
provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
624-
if err != nil {
625-
t.Fatalf("unable to create provider %v", err)
626-
}
627-
errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath(""))
628-
if len(errs) == 0 {
629-
t.Errorf("%s expected validation failure but did not receive errors", k)
630-
continue
631-
}
632-
if !strings.Contains(errs[0].Error(), v.expectedError) {
633-
t.Errorf("%s received unexpected error %v\nexpected: %s", k, errs, v.expectedError)
634-
}
622+
t.Run(k, func(t *testing.T) {
623+
provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
624+
if err != nil {
625+
t.Fatalf("unable to create provider %v", err)
626+
}
627+
errs := provider.ValidatePod(v.pod)
628+
if len(errs) == 0 {
629+
t.Errorf("expected validation failure but did not receive errors")
630+
return
631+
}
632+
if !strings.Contains(errs[0].Error(), v.expectedError) {
633+
t.Errorf("unexpected error %v\nexpected: %s", errs, v.expectedError)
634+
}
635+
})
635636
}
636637
}
637638

638-
func TestValidatePodSecurityContextSuccess(t *testing.T) {
639+
func TestValidatePodSuccess(t *testing.T) {
639640
hostNetworkPSP := defaultPSP()
640641
hostNetworkPSP.Spec.HostNetwork = true
641642
hostNetworkPod := defaultPod()
@@ -941,6 +942,7 @@ func TestValidateContainerSuccess(t *testing.T) {
941942
},
942943
}
943944
seLinuxPod := defaultPod()
945+
seLinuxPod.Spec.SecurityContext.SELinuxOptions = &api.SELinuxOptions{Level: "foo"}
944946
seLinuxPod.Spec.Containers[0].SecurityContext.SELinuxOptions = &api.SELinuxOptions{
945947
Level: "foo",
946948
}
@@ -1007,6 +1009,7 @@ func TestValidateContainerSuccess(t *testing.T) {
10071009

10081010
seccompPod := defaultPod()
10091011
seccompPod.Annotations = map[string]string{
1012+
api.SeccompPodAnnotationKey: "foo",
10101013
api.SeccompContainerAnnotationKeyPrefix + seccompPod.Spec.Containers[0].Name: "foo",
10111014
}
10121015

@@ -1074,15 +1077,16 @@ func TestValidateContainerSuccess(t *testing.T) {
10741077
}
10751078

10761079
for k, v := range successCases {
1077-
provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
1078-
if err != nil {
1079-
t.Fatalf("unable to create provider %v", err)
1080-
}
1081-
errs := provider.ValidateContainer(v.pod, &v.pod.Spec.Containers[0], field.NewPath(""))
1082-
if len(errs) != 0 {
1083-
t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta))
1084-
continue
1085-
}
1080+
t.Run(k, func(t *testing.T) {
1081+
provider, err := NewSimpleProvider(v.psp, "namespace", NewSimpleStrategyFactory())
1082+
if err != nil {
1083+
t.Fatalf("unable to create provider %v", err)
1084+
}
1085+
errs := provider.ValidatePod(v.pod)
1086+
if len(errs) != 0 {
1087+
t.Errorf("%s expected validation pass but received errors %v\n%s", k, errs, spew.Sdump(v.pod.ObjectMeta))
1088+
}
1089+
})
10861090
}
10871091
}
10881092

@@ -1146,7 +1150,7 @@ func TestGenerateContainerSecurityContextReadOnlyRootFS(t *testing.T) {
11461150
t.Errorf("%s unable to create provider %v", k, err)
11471151
continue
11481152
}
1149-
err = provider.DefaultContainerSecurityContext(v.pod, &v.pod.Spec.Containers[0])
1153+
err = provider.MutatePod(v.pod)
11501154
if err != nil {
11511155
t.Errorf("%s unable to create container security context %v", k, err)
11521156
continue
@@ -1351,10 +1355,10 @@ func TestAllowPrivilegeEscalation(t *testing.T) {
13511355
provider, err := NewSimpleProvider(psp, "namespace", NewSimpleStrategyFactory())
13521356
require.NoError(t, err)
13531357

1354-
err = provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[0])
1358+
err = provider.MutatePod(pod)
13551359
require.NoError(t, err)
13561360

1357-
errs := provider.ValidateContainer(pod, &pod.Spec.Containers[0], field.NewPath(""))
1361+
errs := provider.ValidatePod(pod)
13581362
if test.expectErr {
13591363
assert.NotEmpty(t, errs, "expected validation error")
13601364
} else {

‎pkg/security/podsecuritypolicy/types.go‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,12 @@ import (
3232
// Provider provides the implementation to generate a new security
3333
// context based on constraints or validate an existing security context against constraints.
3434
type Provider interface {
35-
// DefaultPodSecurityContext sets the default values of the required but not filled fields.
36-
// It modifies the SecurityContext and annotations of the provided pod.
37-
DefaultPodSecurityContext(pod *api.Pod) error
38-
// DefaultContainerSecurityContext sets the default values of the required but not filled fields.
39-
// It modifies the SecurityContext of the container and annotations of the pod.
40-
DefaultContainerSecurityContext(pod *api.Pod, container *api.Container) error
41-
// Ensure a pod is in compliance with the given constraints.
35+
// MutatePod sets the default values of the required but not filled fields of the pod and all
36+
// containers in the pod.
37+
MutatePod(pod *api.Pod) error
38+
// ValidatePod ensures a pod and all its containers are in compliance with the given constraints.
39+
// ValidatePod MUST NOT mutate the pod.
4240
ValidatePod(pod *api.Pod) field.ErrorList
43-
// Ensure a container's SecurityContext is in compliance with the given constraints.
44-
ValidateContainer(pod *api.Pod, container *api.Container, containerPath *field.Path) field.ErrorList
4541
// Get the name of the PSP that this provider was initialized with.
4642
GetPSPName() string
4743
}

‎plugin/pkg/admission/security/podsecuritypolicy/admission.go‎

Lines changed: 4 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -306,35 +306,14 @@ func (c *PodSecurityPolicyPlugin) computeSecurityContext(a admission.Attributes,
306306
func assignSecurityContext(provider psp.Provider, pod *api.Pod) field.ErrorList {
307307
errs := field.ErrorList{}
308308

309-
err := provider.DefaultPodSecurityContext(pod)
310-
if err != nil {
311-
errs = append(errs, field.Invalid(field.NewPath("spec", "securityContext"), pod.Spec.SecurityContext, err.Error()))
309+
if err := provider.MutatePod(pod); err != nil {
310+
// TODO(tallclair): MutatePod should return a field.ErrorList
311+
errs = append(errs, field.Invalid(field.NewPath(""), pod, err.Error()))
312312
}
313313

314314
errs = append(errs, provider.ValidatePod(pod)...)
315315

316-
for i := range pod.Spec.InitContainers {
317-
err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.InitContainers[i])
318-
if err != nil {
319-
errs = append(errs, field.Invalid(field.NewPath("spec", "initContainers").Index(i).Child("securityContext"), "", err.Error()))
320-
continue
321-
}
322-
errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.InitContainers[i], field.NewPath("spec", "initContainers").Index(i))...)
323-
}
324-
325-
for i := range pod.Spec.Containers {
326-
err := provider.DefaultContainerSecurityContext(pod, &pod.Spec.Containers[i])
327-
if err != nil {
328-
errs = append(errs, field.Invalid(field.NewPath("spec", "containers").Index(i).Child("securityContext"), "", err.Error()))
329-
continue
330-
}
331-
errs = append(errs, provider.ValidateContainer(pod, &pod.Spec.Containers[i], field.NewPath("spec", "containers").Index(i))...)
332-
}
333-
334-
if len(errs) > 0 {
335-
return errs
336-
}
337-
return nil
316+
return errs
338317
}
339318

340319
// createProvidersFromPolicies creates providers from the constraints supplied.

0 commit comments

Comments
 (0)