Skip to content

Commit 6467c33

Browse files
mikebrowmikebrow
committed
refactor based on comments
Signed-off-by: Mike Brown <[email protected]>
1 parent b4727ea commit 6467c33

6 files changed

Lines changed: 146 additions & 110 deletions

File tree

‎go.mod‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ require (
6161
k8s.io/apiserver v0.19.4
6262
k8s.io/client-go v0.19.4
6363
k8s.io/component-base v0.19.4
64-
k8s.io/cri-api 3990421b69a024ba452db5f6e4ae7d45d93a019a
64+
k8s.io/cri-api v0.20.0-beta.1.0.20201105173512-3990421b69a0
6565
k8s.io/klog/v2 v2.2.0
6666
k8s.io/utils v0.0.0-20200729134348-d5654de09c73
6767
)

‎go.sum‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -714,8 +714,8 @@ k8s.io/client-go v0.19.4/go.mod h1:ZrEy7+wj9PjH5VMBCuu/BDlvtUAku0oVFk4MmnW9mWA=
714714
k8s.io/component-base v0.19.4 h1:HobPRToQ8KJ9ubRju6PUAk9I5V1GNMJZ4PyWbiWA0uI=
715715
k8s.io/component-base v0.19.4/go.mod h1:ZzuSLlsWhajIDEkKF73j64Gz/5o0AgON08FgRbEPI70=
716716
k8s.io/cri-api v0.17.3/go.mod h1:X1sbHmuXhwaHs9xxYffLqJogVsnI+f6cPRcgPel7ywM=
717-
k8s.io/cri-api v0.19.4 h1:Vc00x5LSSbLBgvj7UAi4kjsv276n4SGX0XlI/pWhG2E=
718-
k8s.io/cri-api v0.19.4/go.mod h1:UN/iU9Ua0iYdDREBXNE9vqCJ7MIh/FW3VIL0d8pw7Fw=
717+
k8s.io/cri-api v0.20.0-beta.1.0.20201105173512-3990421b69a0 h1:/AO/xlTAHFP+ZLhfYMjSUzMsZe9of1fwh1TupXAKzKE=
718+
k8s.io/cri-api v0.20.0-beta.1.0.20201105173512-3990421b69a0/go.mod h1:UN/iU9Ua0iYdDREBXNE9vqCJ7MIh/FW3VIL0d8pw7Fw=
719719
k8s.io/gengo v0.0.0-20200413195148-3a45101e95ac/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
720720
k8s.io/klog/v2 v2.0.0/go.mod h1:PBfzABfn139FHAV07az/IF9Wp1bkk3vpT2XSJ76fSDE=
721721
k8s.io/klog/v2 v2.2.0 h1:XRvcwJozkgZ1UQJmfMGpvRthQHOvihEhYtDfAaxMz/A=

‎pkg/cri/server/container_create_linux.go‎

Lines changed: 89 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -306,9 +306,15 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
306306
}
307307
specOpts = append(specOpts, customopts.WithAdditionalGIDs(userstr))
308308

309+
asp := securityContext.GetApparmor()
310+
if asp == nil {
311+
asp, err = generateApparmorSecurityProfile(securityContext.GetApparmorProfile()) // nolint:staticcheck deprecated but we don't want to remove yet
312+
if err != nil {
313+
return nil, errors.Wrap(err, "failed to generate apparmor spec opts")
314+
}
315+
}
309316
apparmorSpecOpts, err := generateApparmorSpecOpts(
310-
securityContext.GetApparmor(),
311-
securityContext.GetApparmorProfile(), // nolint:staticcheck deprecated but we don't want to remove yet
317+
asp,
312318
securityContext.GetPrivileged(),
313319
c.apparmorEnabled())
314320
if err != nil {
@@ -318,9 +324,17 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
318324
specOpts = append(specOpts, apparmorSpecOpts)
319325
}
320326

327+
ssp := securityContext.GetSeccomp()
328+
if ssp == nil {
329+
ssp, err = generateSeccompSecurityProfile(
330+
securityContext.GetSeccompProfilePath(), // nolint:staticcheck deprecated but we don't want to remove yet
331+
c.config.UnsetSeccompProfile)
332+
if err != nil {
333+
return nil, errors.Wrap(err, "failed to generate seccomp spec opts")
334+
}
335+
}
321336
seccompSpecOpts, err := c.generateSeccompSpecOpts(
322-
securityContext.GetSeccomp(),
323-
securityContext.GetSeccompProfilePath(), // nolint:staticcheck deprecated but we don't want to remove yet
337+
ssp,
324338
securityContext.GetPrivileged(),
325339
c.seccompEnabled())
326340
if err != nil {
@@ -332,24 +346,51 @@ func (c *criService) containerSpecOpts(config *runtime.ContainerConfig, imageCon
332346
return specOpts, nil
333347
}
334348

349+
func generateSeccompSecurityProfile(profilePath string, unsetProfilePath string) (*runtime.SecurityProfile, error) {
350+
if profilePath != "" {
351+
return generateSecurityProfile(profilePath)
352+
}
353+
if unsetProfilePath != "" {
354+
return generateSecurityProfile(unsetProfilePath)
355+
}
356+
return nil, nil
357+
}
358+
func generateApparmorSecurityProfile(profilePath string) (*runtime.SecurityProfile, error) {
359+
if profilePath != "" {
360+
return generateSecurityProfile(profilePath)
361+
}
362+
return nil, nil
363+
}
364+
365+
func generateSecurityProfile(profilePath string) (*runtime.SecurityProfile, error) {
366+
switch profilePath {
367+
case runtimeDefault, dockerDefault, "":
368+
return &runtime.SecurityProfile{
369+
ProfileType: runtime.SecurityProfile_RuntimeDefault,
370+
}, nil
371+
case unconfinedProfile:
372+
return &runtime.SecurityProfile{
373+
ProfileType: runtime.SecurityProfile_Unconfined,
374+
}, nil
375+
default:
376+
// Require and Trim default profile name prefix
377+
if !strings.HasPrefix(profilePath, profileNamePrefix) {
378+
return nil, errors.Errorf("invalid profile %q", profilePath)
379+
}
380+
return &runtime.SecurityProfile{
381+
ProfileType: runtime.SecurityProfile_Localhost,
382+
LocalhostRef: strings.TrimPrefix(profilePath, profileNamePrefix),
383+
}, nil
384+
}
385+
}
386+
335387
// generateSeccompSpecOpts generates containerd SpecOpts for seccomp.
336-
func (c *criService) generateSeccompSpecOpts(sp *runtime.SecurityProfile, seccompProf string, privileged, seccompEnabled bool) (oci.SpecOpts, error) {
388+
func (c *criService) generateSeccompSpecOpts(sp *runtime.SecurityProfile, privileged, seccompEnabled bool) (oci.SpecOpts, error) {
337389
if privileged {
338390
// Do not set seccomp profile when container is privileged
339391
return nil, nil
340392
}
341-
if seccompProf == "" && sp == nil {
342-
seccompProf = c.config.UnsetSeccompProfile
343-
}
344-
// Set seccomp profile
345-
if seccompProf == runtimeDefault || seccompProf == dockerDefault {
346-
// use correct default profile (Eg. if not configured otherwise, the default is docker/default)
347-
seccompProf = seccompDefaultProfile
348-
}
349393
if !seccompEnabled {
350-
if seccompProf != "" && seccompProf != unconfinedProfile {
351-
return nil, errors.New("seccomp is not supported")
352-
}
353394
if sp != nil {
354395
if sp.ProfileType != runtime.SecurityProfile_Unconfined {
355396
return nil, errors.New("seccomp is not supported")
@@ -358,49 +399,33 @@ func (c *criService) generateSeccompSpecOpts(sp *runtime.SecurityProfile, seccom
358399
return nil, nil
359400
}
360401

361-
if sp != nil {
362-
if sp.ProfileType != runtime.SecurityProfile_Localhost && sp.LocalhostRef != "" {
363-
return nil, errors.New("seccomp config invalid LocalhostRef must only be set if ProfileType is Localhost")
364-
}
365-
switch sp.ProfileType {
366-
case runtime.SecurityProfile_Unconfined:
367-
// Do not set seccomp profile.
368-
return nil, nil
369-
case runtime.SecurityProfile_RuntimeDefault:
370-
return seccomp.WithDefaultProfile(), nil
371-
case runtime.SecurityProfile_Localhost:
372-
// trimming the localhost/ prefix just in case even through it should not
373-
// be necessary with the new SecurityProfile struct
374-
return seccomp.WithProfile(strings.TrimPrefix(sp.LocalhostRef, profileNamePrefix)), nil
375-
default:
376-
return nil, errors.New("seccomp unknown ProfileType")
377-
}
402+
if sp == nil {
403+
return nil, nil
378404
}
379405

380-
switch seccompProf {
381-
case "", unconfinedProfile:
406+
if sp.ProfileType != runtime.SecurityProfile_Localhost && sp.LocalhostRef != "" {
407+
return nil, errors.New("seccomp config invalid LocalhostRef must only be set if ProfileType is Localhost")
408+
}
409+
switch sp.ProfileType {
410+
case runtime.SecurityProfile_Unconfined:
382411
// Do not set seccomp profile.
383412
return nil, nil
384-
case dockerDefault:
385-
// Note: WithDefaultProfile specOpts must be added after capabilities
413+
case runtime.SecurityProfile_RuntimeDefault:
386414
return seccomp.WithDefaultProfile(), nil
415+
case runtime.SecurityProfile_Localhost:
416+
// trimming the localhost/ prefix just in case even though it should not
417+
// be necessary with the new SecurityProfile struct
418+
return seccomp.WithProfile(strings.TrimPrefix(sp.LocalhostRef, profileNamePrefix)), nil
387419
default:
388-
// Require and Trim default profile name prefix
389-
if !strings.HasPrefix(seccompProf, profileNamePrefix) {
390-
return nil, errors.Errorf("invalid seccomp profile %q", seccompProf)
391-
}
392-
return seccomp.WithProfile(strings.TrimPrefix(seccompProf, profileNamePrefix)), nil
420+
return nil, errors.New("seccomp unknown ProfileType")
393421
}
394422
}
395423

396424
// generateApparmorSpecOpts generates containerd SpecOpts for apparmor.
397-
func generateApparmorSpecOpts(sp *runtime.SecurityProfile, apparmorProf string, privileged, apparmorEnabled bool) (oci.SpecOpts, error) {
425+
func generateApparmorSpecOpts(sp *runtime.SecurityProfile, privileged, apparmorEnabled bool) (oci.SpecOpts, error) {
398426
if !apparmorEnabled {
399427
// Should fail loudly if user try to specify apparmor profile
400428
// but we don't support it.
401-
if apparmorProf != "" && apparmorProf != unconfinedProfile {
402-
return nil, errors.New("apparmor is not supported")
403-
}
404429
if sp != nil {
405430
if sp.ProfileType != runtime.SecurityProfile_Unconfined {
406431
return nil, errors.New("apparmor is not supported")
@@ -409,62 +434,40 @@ func generateApparmorSpecOpts(sp *runtime.SecurityProfile, apparmorProf string,
409434
return nil, nil
410435
}
411436

412-
if sp != nil {
413-
if sp.ProfileType != runtime.SecurityProfile_Localhost && sp.LocalhostRef != "" {
414-
return nil, errors.New("apparmor config invalid LocalhostRef must only be set if ProfileType is Localhost")
415-
}
437+
if sp == nil {
438+
// Based on kubernetes#51746, default apparmor profile should be applied
439+
// for when apparmor is not specified.
440+
sp, _ = generateSecurityProfile("")
441+
}
416442

417-
switch sp.ProfileType {
418-
case runtime.SecurityProfile_Unconfined:
419-
// Do not set apparmor profile.
420-
return nil, nil
421-
case runtime.SecurityProfile_RuntimeDefault:
422-
if privileged {
423-
// Do not set apparmor profile when container is privileged
424-
return nil, nil
425-
}
426-
return apparmor.WithDefaultProfile(appArmorDefaultProfileName), nil
427-
case runtime.SecurityProfile_Localhost:
428-
// trimming the localhost/ prefix just in case even through it should not
429-
// be necessary with the new SecurityProfile struct
430-
appArmorProfile := strings.TrimPrefix(sp.LocalhostRef, profileNamePrefix)
431-
if profileExists, err := appArmorProfileExists(appArmorProfile); !profileExists {
432-
if err != nil {
433-
return nil, errors.Wrap(err, "failed to generate apparmor spec opts")
434-
}
435-
return nil, errors.Errorf("apparmor profile not found %s", appArmorProfile)
436-
}
437-
return apparmor.WithProfile(appArmorProfile), nil
438-
default:
439-
return nil, errors.New("apparmor unknown ProfileType")
440-
}
443+
if sp.ProfileType != runtime.SecurityProfile_Localhost && sp.LocalhostRef != "" {
444+
return nil, errors.New("apparmor config invalid LocalhostRef must only be set if ProfileType is Localhost")
441445
}
442446

443-
switch apparmorProf {
444-
// Based on kubernetes#51746, default apparmor profile should be applied
445-
// for when apparmor is not specified.
446-
case runtimeDefault, "":
447+
switch sp.ProfileType {
448+
case runtime.SecurityProfile_Unconfined:
449+
// Do not set apparmor profile.
450+
return nil, nil
451+
case runtime.SecurityProfile_RuntimeDefault:
447452
if privileged {
448453
// Do not set apparmor profile when container is privileged
449454
return nil, nil
450455
}
451456
// TODO (mikebrow): delete created apparmor default profile
452457
return apparmor.WithDefaultProfile(appArmorDefaultProfileName), nil
453-
case unconfinedProfile:
454-
return nil, nil
455-
default:
456-
// Require and Trim default profile name prefix
457-
if !strings.HasPrefix(apparmorProf, profileNamePrefix) {
458-
return nil, errors.Errorf("invalid apparmor profile %q", apparmorProf)
459-
}
460-
appArmorProfile := strings.TrimPrefix(apparmorProf, profileNamePrefix)
458+
case runtime.SecurityProfile_Localhost:
459+
// trimming the localhost/ prefix just in case even through it should not
460+
// be necessary with the new SecurityProfile struct
461+
appArmorProfile := strings.TrimPrefix(sp.LocalhostRef, profileNamePrefix)
461462
if profileExists, err := appArmorProfileExists(appArmorProfile); !profileExists {
462463
if err != nil {
463464
return nil, errors.Wrap(err, "failed to generate apparmor spec opts")
464465
}
465466
return nil, errors.Errorf("apparmor profile not found %s", appArmorProfile)
466467
}
467468
return apparmor.WithProfile(appArmorProfile), nil
469+
default:
470+
return nil, errors.New("apparmor unknown ProfileType")
468471
}
469472
}
470473

‎pkg/cri/server/container_create_linux_test.go‎

Lines changed: 42 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -832,10 +832,6 @@ func TestGenerateSeccompSecurityProfileSpecOpts(t *testing.T) {
832832
profile: profileNamePrefix + "test-profile",
833833
specOpts: seccomp.WithProfile("test-profile"),
834834
},
835-
"should return error if specified profile is invalid": {
836-
profile: "test-profile",
837-
expectErr: true,
838-
},
839835
"should use default profile when seccomp is empty": {
840836
defaultProfile: profileNamePrefix + "test-profile",
841837
specOpts: seccomp.WithProfile("test-profile"),
@@ -903,14 +899,29 @@ func TestGenerateSeccompSecurityProfileSpecOpts(t *testing.T) {
903899
t.Run(fmt.Sprintf("TestCase %q", desc), func(t *testing.T) {
904900
cri := &criService{}
905901
cri.config.UnsetSeccompProfile = test.defaultProfile
906-
specOpts, err := cri.generateSeccompSpecOpts(test.sp, test.profile, test.privileged, !test.disable)
907-
assert.Equal(t,
908-
reflect.ValueOf(test.specOpts).Pointer(),
909-
reflect.ValueOf(specOpts).Pointer())
910-
if test.expectErr {
911-
assert.Error(t, err)
902+
ssp := test.sp
903+
csp, err := generateSeccompSecurityProfile(
904+
test.profile,
905+
test.defaultProfile)
906+
if err != nil {
907+
if test.expectErr {
908+
assert.Error(t, err)
909+
} else {
910+
assert.NoError(t, err)
911+
}
912912
} else {
913-
assert.NoError(t, err)
913+
if ssp == nil {
914+
ssp = csp
915+
}
916+
specOpts, err := cri.generateSeccompSpecOpts(ssp, test.privileged, !test.disable)
917+
assert.Equal(t,
918+
reflect.ValueOf(test.specOpts).Pointer(),
919+
reflect.ValueOf(specOpts).Pointer())
920+
if test.expectErr {
921+
assert.Error(t, err)
922+
} else {
923+
assert.NoError(t, err)
924+
}
914925
}
915926
})
916927
}
@@ -1039,14 +1050,27 @@ func TestGenerateApparmorSpecOpts(t *testing.T) {
10391050
},
10401051
} {
10411052
t.Logf("TestCase %q", desc)
1042-
specOpts, err := generateApparmorSpecOpts(test.sp, test.profile, test.privileged, !test.disable)
1043-
assert.Equal(t,
1044-
reflect.ValueOf(test.specOpts).Pointer(),
1045-
reflect.ValueOf(specOpts).Pointer())
1046-
if test.expectErr {
1047-
assert.Error(t, err)
1053+
asp := test.sp
1054+
csp, err := generateApparmorSecurityProfile(test.profile)
1055+
if err != nil {
1056+
if test.expectErr {
1057+
assert.Error(t, err)
1058+
} else {
1059+
assert.NoError(t, err)
1060+
}
10481061
} else {
1049-
assert.NoError(t, err)
1062+
if asp == nil {
1063+
asp = csp
1064+
}
1065+
specOpts, err := generateApparmorSpecOpts(asp, test.privileged, !test.disable)
1066+
assert.Equal(t,
1067+
reflect.ValueOf(test.specOpts).Pointer(),
1068+
reflect.ValueOf(specOpts).Pointer())
1069+
if test.expectErr {
1070+
assert.Error(t, err)
1071+
} else {
1072+
assert.NoError(t, err)
1073+
}
10501074
}
10511075
}
10521076
}

‎pkg/cri/server/sandbox_run_linux.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,10 +163,19 @@ func (c *criService) sandboxContainerSpecOpts(config *runtime.PodSandboxConfig,
163163
var (
164164
securityContext = config.GetLinux().GetSecurityContext()
165165
specOpts []oci.SpecOpts
166+
err error
166167
)
168+
ssp := securityContext.GetSeccomp()
169+
if ssp == nil {
170+
ssp, err = generateSeccompSecurityProfile(
171+
securityContext.GetSeccompProfilePath(), // nolint:staticcheck deprecated but we don't want to remove yet
172+
c.config.UnsetSeccompProfile)
173+
if err != nil {
174+
return nil, errors.Wrap(err, "failed to generate seccomp spec opts")
175+
}
176+
}
167177
seccompSpecOpts, err := c.generateSeccompSpecOpts(
168-
securityContext.GetSeccomp(),
169-
securityContext.GetSeccompProfilePath(), // nolint:staticcheck deprecated but we don't want to remove yet
178+
ssp,
170179
securityContext.GetPrivileged(),
171180
c.seccompEnabled())
172181
if err != nil {

‎vendor/modules.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -501,7 +501,7 @@ k8s.io/client-go/util/workqueue
501501
# k8s.io/component-base v0.19.4
502502
## explicit
503503
k8s.io/component-base/logs/logreduction
504-
# k8s.io/cri-api v0.19.4
504+
# k8s.io/cri-api v0.20.0-beta.1.0.20201105173512-3990421b69a0
505505
## explicit
506506
k8s.io/cri-api/pkg/apis
507507
k8s.io/cri-api/pkg/apis/runtime/v1alpha2

0 commit comments

Comments
 (0)