Feature: pod security admission #165

feloy · 2023-02-20T12:29:14Z

What does this PR do?:

This PR adds the possibility to get a patched PodTemplate based on a Pod Security Admission policy, so the Pod can be accepted by the Pod Security Admission controller.

Prior to being able to do this, some refactoring has been done:

updating the k8s dependencies to latest version, to be able to use the latest version of k8s.io/pod-security-admission
update to go 1.19, to be able to use the latest k8s dependencies
A new method GetPodTemplateSpec has been added, that will handle the Pod Security patching and the container/pod overrides in an atomic way (instead of making them in different places), to be used instead of GetContainers/GetInitContainers
GetDeployment now accepts a podSecurityTemplate instead of containers, initContainers and volumes (these 3 values are now marked as deprecated)

Which issue(s) this PR fixes:

redhat-developer/odo#6339

PR acceptance criteria:

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

Open new test/doc issues under the devfile/api repo

Check each criteria if:

There is a separate tracking issue. Add the issue link under the criteria
or

test/doc updates are made as part of this PR

If unchecked, explain why it's not needed

How to test changes / Special notes to the reviewer:

Signed-off-by: Philippe Martin <[email protected]>

codecov · 2023-02-20T12:58:04Z

Codecov Report

Patch coverage: 67.42% and project coverage change: +0.65 🎉

Comparison is base (4f0ff68) 59.67% compared to head (2239fa9) 60.33%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   59.67%   60.33%   +0.65%     
==========================================
  Files          36       37       +1     
  Lines        4226     4369     +143     
==========================================
+ Hits         2522     2636     +114     
- Misses       1557     1571      +14     
- Partials      147      162      +15

Impacted Files	Coverage Δ
pkg/devfile/parser/data/v2/header.go	`100.00% <ø> (ø)`
pkg/devfile/parser/data/v2/projects.go	`94.73% <ø> (ø)`
pkg/devfile/parser/parse.go	`59.01% <ø> (+5.14%)`	⬆️
pkg/devfile/parser/sourceAttribute.go	`85.91% <ø> (ø)`
pkg/util/util.go	`38.69% <ø> (ø)`
pkg/devfile/generator/policy.go	`64.10% <64.10%> (ø)`
pkg/devfile/generator/generators.go	`63.09% <69.47%> (-2.14%)`	⬇️
pkg/devfile/generator/utils.go	`90.90% <100.00%> (+0.28%)`	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Signed-off-by: Philippe Martin <[email protected]>

pkg/devfile/generator/generators.go

elsony · 2023-02-23T13:12:45Z

To maintain backward compatibility, we need to keep the old functions and create new ones to provide the new function

…ace calls to GetContainers + GetInitContainers) Signed-off-by: Philippe Martin <[email protected]>

Signed-off-by: Philippe Martin <[email protected]>

pkg/devfile/generator/generators.go

rm3l · 2023-02-28T16:50:19Z

.github/workflows/go.yml

@@ -16,7 +16,7 @@ jobs:
    - name: Setup Go environment
      uses: actions/[email protected]
      with:
-        go-version: 1.16
+        go-version: 1.19


I guess we also need to update the Go version in codecov.yml? I can see that this workflow is not triggered on PRs, but only upon a push on main, which makes me think that this may no longer pass on main once this PR is merged.

Also, wondering if we should not update the version in go.mod to reflect this? This way, all calls to the setup-go action could simply use the version declared in go.mod (see https://github.com/actions/setup-go#getting-go-version-from-the-gomod-file). But I am not sure about the implications on consumers of the library if the Go version in go.mod is updated. This might be a different issue, that said..

rm3l · 2023-02-28T17:08:56Z

pkg/devfile/generator/policy.go

+		podTemplateSpec.Spec.SecurityContext = &corev1.PodSecurityContext{}
+	}
+	podTemplateSpec.Spec.SecurityContext.RunAsNonRoot = pointer.Bool(true)
+	// No need to set the value as true for containers, as setting at the Pod level is sufficient


To me (and per https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container), security settings at the Container level override settings made at the Pod level when there is an overlap. So I think, we don't need to set the value as true to containers only if they are not specified at the container level.
So we should still visit all containers and set the field to true, no?

To me (and per https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container), security settings at the Container level override settings made at the Pod level when there is an overlap. So I think, we don't need to set the value as true to containers only if they are not specified at the container level.

I agree up to here.

So we should still visit all containers and set the field to true, no?

By design, we know the field at container level will never be set (because they could be set only from the library, and they are not). For this reason, we only need to set it at pod level.

rm3l · 2023-02-28T17:10:58Z

pkg/devfile/generator/policy.go

+		if podTemplateSpec.Spec.SecurityContext.SeccompProfile == nil {
+			podTemplateSpec.Spec.SecurityContext.SeccompProfile = &corev1.SeccompProfile{}
+		}
+		podTemplateSpec.Spec.SecurityContext.SeccompProfile.Type = "RuntimeDefault"


Similar point here: I think we should visit all containers and set the field too..

To me (and per https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-security-context-for-a-container), security settings at the Container level override settings made at the Pod level when there is an overlap. So I think, we don't need to set the value to containers only if they are not specified at the container level.
So we should still visit all containers and set the field, no?

pkg/testingutil/k8sClient.go

openshift-ci · 2023-02-28T17:12:10Z

@rm3l: changing LGTM is restricted to collaborators

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Philippe Martin <[email protected]>

rm3l

LGTM - thanks!

openshift-ci · 2023-03-01T12:17:54Z

@rm3l: changing LGTM is restricted to collaborators

In response to this:

LGTM - thanks!

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

yangcao77 · 2023-03-07T20:12:36Z

pkg/devfile/generator/generators.go

+// - patches the pod template and containers to satisfy PodSecurityAdmissionPolicy
+// - patches the pod template and containers to apply pod and container overrides
+func GetPodTemplateSpec(devfileObj parser.DevfileObj, podTemplateParams PodTemplateParams) (*corev1.PodTemplateSpec, error) {
+	containers, err := GetContainers(devfileObj, common.DevfileOptions{})


the GetContainers and GetInitContainers are being called by this new function introduced. which means the two functions are still in use, and need to be updated upon any changes to GetContainers/GetInitContainers behaviors. why mark the two functions as deprecated?

IMO, the GetContainers/GetInitContainers could be removed later from the library interface, by renaming them as getContainers/getInitContainers

yangcao77 · 2023-03-07T22:08:37Z

pkg/devfile/generator/generators.go

+			return nil, errors.New("InitContainers, Containers and Volumes cannot be set when PodTemplateSpec is set in parameters")
+		}
+
+		deploySpecParams = deploymentSpecParams{


dup with

library/pkg/devfile/generator/generators.go

Lines 209 to 212 in 5985a28

deploySpecParams = deploymentSpecParams{

PodTemplateSpec: *podTemplateSpec,

PodSelectorLabels: deployParams.PodSelectorLabels,

Replicas: deployParams.Replicas,

, can be move out side of the if-else block?

yangcao77 · 2023-03-07T22:46:54Z

pkg/devfile/generator/generators.go

 	}

-	return deployment, nil
+	getContainerByName := func(name string) (*corev1.Container, bool) {


you do not need to introduce a new function for this purpose.
should use

library/pkg/devfile/parser/data/v2/components.go

Line 28 in 5985a28

func (d *DevfileV2) GetComponents(options common.DevfileOptions) ([]v1.Component, error) {

, and set DevfileOptions.FilterByName = <the containerName>

Are you sure? getContainerByName finds a specific container from the containers passed as parameter, but the method you mentions would find a component from the Devfile.

feloy · 2023-03-08T13:12:56Z

Thanks @yangcao77 for you review

Signed-off-by: Philippe Martin <[email protected]>

yangcao77

thanks for making this change, generally looks good to me

openshift-ci · 2023-03-08T15:53:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feloy, rm3l, yangcao77

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yangcao77]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress label Feb 20, 2023

openshift-ci bot requested review from maysunfaisal and yangcao77 February 20, 2023 12:29

feloy marked this pull request as draft February 20, 2023 12:29

Update k8s libraries to v0.26.1

614deae

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch 2 times, most recently from eef21a1 to 8572863 Compare February 20, 2023 12:38

feloy force-pushed the feature-pod-security-admission branch from a3c1ec6 to 4ed8ec6 Compare February 20, 2023 13:00

feloy added 3 commits February 20, 2023 17:30

Run tests with go 1.19

ffdff05

Signed-off-by: Philippe Martin <[email protected]>

Fix fmt with go1.19

e68438e

Signed-off-by: Philippe Martin <[email protected]>

Update Check license for go 1.19

800b0c1

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch 6 times, most recently from 3bd8f3f to a7af25b Compare February 21, 2023 14:03

feloy marked this pull request as ready for review February 21, 2023 14:13

openshift-ci bot requested a review from elsony February 21, 2023 14:14

feloy changed the title ~~[wip] Feature pod security admission~~ Feature pod security admission Feb 22, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 22, 2023

rm3l reviewed Feb 22, 2023

View reviewed changes

pkg/devfile/generator/generators.go Show resolved Hide resolved

feloy changed the title ~~Feature pod security admission~~ Feature: pod security admission Feb 23, 2023

feloy force-pushed the feature-pod-security-admission branch 2 times, most recently from 5fd63a3 to 12f8332 Compare February 23, 2023 13:27

feloy changed the title ~~Feature: pod security admission~~ [wip] Feature: pod security admission Feb 23, 2023

openshift-ci bot added the do-not-merge/work-in-progress label Feb 23, 2023

Add GetPodTemplateSpec to build the complete PodTemplateSpec (to repl…

d93f82c

…ace calls to GetContainers + GetInitContainers) Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch from 12f8332 to 497d2a0 Compare February 23, 2023 14:24

Add deprecation information

d797fca

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch from 88bd295 to d797fca Compare February 23, 2023 14:36

feloy changed the title ~~[wip] Feature: pod security admission~~ Feature: pod security admission Feb 23, 2023

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 23, 2023

feloy requested review from rm3l and removed request for elsony, yangcao77 and maysunfaisal February 23, 2023 14:37

kim-tsao mentioned this pull request Feb 27, 2023

Fix registry dependabot issues devfile/api#1030

Closed

3 tasks

rm3l suggested changes Feb 28, 2023

View reviewed changes

feloy added 2 commits March 1, 2023 09:54

Review

91e86b5

Signed-off-by: Philippe Martin <[email protected]>

Update go version fo codecov

5985a28

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch from 2b29e22 to 5985a28 Compare March 1, 2023 08:54

feloy requested a review from rm3l March 1, 2023 08:55

rm3l approved these changes Mar 1, 2023

View reviewed changes

yangcao77 reviewed Mar 7, 2023

View reviewed changes

feloy requested a review from yangcao77 March 8, 2023 13:11

Review: factorize some duplicate code

2239fa9

Signed-off-by: Philippe Martin <[email protected]>

feloy force-pushed the feature-pod-security-admission branch from a2a2790 to 2239fa9 Compare March 8, 2023 13:13

yangcao77 approved these changes Mar 8, 2023

View reviewed changes

openshift-ci bot assigned yangcao77 Mar 8, 2023

openshift-ci bot added the lgtm label Mar 8, 2023

openshift-ci bot added the approved label Mar 8, 2023

kim-tsao merged commit bd4a12f into devfile:main Mar 8, 2023

feloy mentioned this pull request Jul 21, 2023

Implement PodSecurity admission checks in the devfile/library devfile/api#1017

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: pod security admission #165

Feature: pod security admission #165

feloy commented Feb 20, 2023 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

elsony commented Feb 23, 2023

rm3l Feb 28, 2023 •

edited

Loading

rm3l Feb 28, 2023 •

edited

Loading

rm3l Feb 28, 2023

feloy Mar 1, 2023

rm3l Feb 28, 2023

openshift-ci bot commented Feb 28, 2023

rm3l left a comment

openshift-ci bot commented Mar 1, 2023

yangcao77 Mar 7, 2023

feloy Mar 8, 2023

yangcao77 Mar 7, 2023

yangcao77 Mar 7, 2023

feloy Mar 8, 2023

feloy commented Mar 8, 2023

yangcao77 left a comment

openshift-ci bot commented Mar 8, 2023

	deploySpecParams = deploymentSpecParams{
	PodTemplateSpec: *podTemplateSpec,
	PodSelectorLabels: deployParams.PodSelectorLabels,
	Replicas: deployParams.Replicas,

Feature: pod security admission #165

Feature: pod security admission #165

Conversation

feloy commented Feb 20, 2023 • edited Loading

What does this PR do?:

Which issue(s) this PR fixes:

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

codecov bot commented Feb 20, 2023 • edited Loading

Codecov Report

elsony commented Feb 23, 2023

rm3l Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

rm3l Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 28, 2023

rm3l left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feloy commented Mar 8, 2023

yangcao77 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 8, 2023

feloy commented Feb 20, 2023 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

rm3l Feb 28, 2023 •

edited

Loading

rm3l Feb 28, 2023 •

edited

Loading