ROX-10034: Make PodSecurityPolicies Opt-Out by mtesseract · Pull Request #1249 · stackrox/stackrox

mtesseract · 2022-04-11T09:04:44Z

Description

This PR is the first step towards deprecation of PSPs (PodSecurityPolicies), which are to be removed with Kubernetes 1.25.

More concretely, this PR...

implements Toggles within our Helm charts for enabling/disabling generation of PSP resources (defaulting to PSPs enabled).
implements CLI flag for roxctl for controlling the generation of PSPs for deployment bundles (defaulting to PSPs enabled).
extends deployment script to respect the $POD_SECURITY_POLICIES environment variable (defaulting to true)
extends CI to run a subset of tests on a Kubernetes cluster provisioned specifically with PSPs switched off.

For a follow-up PR (#1747) we can then change the above defaults so that

Helm charts and deployment bundles do not generate PSPs by default, making them opt-in.
Deployment scripts default to POD_SECURITY_POLICIES=false.
CI tests are executed (mostly?) with PSPs disabled.

Some more detailed explanations:

For the Helm charts I thought it makes most sense to introduce a setting for enabling PSPs, which currently defaults to true to retain current behaviour and will be changed to false in the future (before arrival of K8s 1.25).
On the other hand, for APIs I have implemented a toggle for disabling this. This is for backwards compatibility: This way one can use old roxctl version which are not aware of this toggle together with a new central and retrieve the expected output (i.e. with PSPs still there).
I have extracted the PSP related resources (ClusterRole, PodSecurityPolicy and RoleBinding) within the Helm charts to separate files, to keep them logically separated from the other non-deprecated resources.
For the Helm charts, the default values for the PSP toggle are injected via our meta-templating mechanism.

This PR should be safe to merge without any feature flags because it retains the current behaviour of producing PSPs while allowing to switch them off.

Note that this PR extends the meaning of an environment variable which is used in our CI pipeline: $POD_SECURITY_POLICIES=true/false. Irregardless of this PR the configuration of clusters to be provisionsed with respect to PSPs is based on this environment setting. E.g., $POD_SECURITY_POLICIES=false causes a cluster to be provisionsed with the PSP Admission Controller being disabled. With this PR the same environment variable is also taken into account by deployment scripts and controls whether StackRox itself should be installed including PSP resources or not.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
~~Evaluated and added CHANGELOG entry if required~~ (don't think we need another CHANGELOG entry for this)
Determined and documented upgrade steps
Documented user facing changes (create PR based on stackrox/openshift-docs and merge into rhacs-docs)

Testing Performed

CI plus some manual tests.

ghost · 2022-04-11T09:28:44Z

Tag for build #645421 is 3.70.x-307-gb13a08e56e.

💻 For deploying this image using the dev scripts, run the following first:

export MAIN_IMAGE_TAG='3.70.x-307-gb13a08e56e'

🕹️ A roxctl binary can be downloaded from the CircleCI artifacts.

mtesseract · 2022-05-17T07:06:00Z

/retest

mtesseract · 2022-05-17T18:42:52Z

/retest

mtesseract · 2022-05-17T18:44:13Z

/test openshift-api-e2e-tests

openshift-ci · 2022-05-17T18:44:16Z

@mtesseract: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test gke-upgrade-tests
/test style-checks

Use /test all to run all jobs.

Details

In response to this:

/test openshift-api-e2e-tests

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.

mtesseract · 2022-05-18T07:46:40Z

docs/Makefile

 	# Verify that submodules have been initialized, otherwise we cannot build much.
-	[ -f content/.git ] || { echo >&2 "Submodule docs/content is not initialized, please run 'git submodules update --init'" ; exit 1 ; }
-	[ -f tools/.git ] || { echo >&2 "Submodule docs/tools is not initialized, please run 'git submodules update --init'" ; exit 1 ; }
+	[ -f content/.git ] || { echo >&2 "Submodule docs/content is not initialized, please run 'git submodule update --init'" ; exit 1 ; }


Just a doc fix I stumbled over.

SimonBaeumer

Great job @mtesseract!

Two major things I noticed:

CI tests are too costly compared to how little it tests. Deploying to a cluster without PSPs works if PSPs are disabled, we only need to test that our Helm option does work properly.
Speaking of DeprecatedPodSecurityPolicies is not optimal. The deprecation is relative to the used kubernetes version, rather mentioning that this is a setting for k8s < 1.25.

deploy/common/k8sbased.sh

SimonBaeumer · 2022-05-19T07:46:05Z

image/templates/helm/shared/internal/scanner-config-shape.yaml

    key: null # string
    generate: null # bool
+system:
+  enableDeprecatedPodSecurityPolicies: null # bool


Suggested change

enableDeprecatedPodSecurityPolicies: null # bool

enablePodSecurityPolicies: null # bool

SimonBaeumer · 2022-05-19T07:46:46Z

image/templates/helm/shared/templates/02-scanner-01-psps.yaml

@@ -0,0 +1,69 @@
+{{- include "srox.init" . -}}
+
+{{- if and (not ._rox.scanner.disable) ._rox.system.enableDeprecatedPodSecurityPolicies }}


Suggested change

{{- if and (not ._rox.scanner.disable) ._rox.system.enableDeprecatedPodSecurityPolicies }}

{{- if and (not ._rox.scanner.disable) ._rox.system.enablePodSecurityPolicies }}

SimonBaeumer · 2022-05-19T07:47:19Z

image/templates/helm/stackrox-central/internal/config-shape.yaml

 globalPrefix: null # string
 system:
  createSCCs: null # bool
+  enableDeprecatedPodSecurityPolicies: null # bool


Suggested change

enableDeprecatedPodSecurityPolicies: null # bool

enablePodSecurityPolicies: null # bool

SimonBaeumer · 2022-05-19T07:49:23Z

image/templates/helm/stackrox-secured-cluster/templates/sensor-pod-security.yaml

@@ -1,5 +1,6 @@
 {{- include "srox.init" . -}}

+{{- if ._rox.system.enableDeprecatedPodSecurityPolicies }}


Suggested change

{{- if ._rox.system.enableDeprecatedPodSecurityPolicies }}

{{- if ._rox.system.enablePodSecurityPolicies }}

SimonBaeumer · 2022-05-19T07:50:39Z

pkg/apiparams/cluster_zip.go

 	SlimCollector    *bool  `json:"slimCollector"`
 	IstioVersion     string `json:"istioVersion"`
+
+	DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`


Can this field be inverted so that it aligns with the other configurations to enablePodSecurityPolicies?

Suggested change

DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`

DisableDeprecatedPodSecurityPolicies bool `json:"enablePodSecurityPolicies"`

Well, as I tried to explain in the PR description, I inverted the logic here on purpose for backwards compatibility reasons: If it is a positive toggle (enable...) and it is missing, then it would be implicitly understood to be false when an old roxctl is speaking to a new central.

When it is a negative toggle (disable...), then the absence would be understood as false, i.e. PSPs are enabled -- I guess this would follow the principle of the least surprise for users using older roxctl versions?

SimonBaeumer · 2022-05-19T07:51:42Z

pkg/apiparams/scanner.go


 	IstioVersion string `json:"istioVersion"`
+
+	DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`


Suggested change

DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`

DisablePodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`

roxctl/central/generate/generate.go

SimonBaeumer · 2022-05-19T07:56:11Z

.circleci/config.yml

            - check-label-to-skip-tests:
                label: ci-no-qa-tests

+  gke-api-e2e-tests-no-psps:


Great job testing it the Helm chart without PSPs but these tests look very costly to me to only test that our application can be installed into a cluster without PSPs.

In cases like these it makes more sense to have a forward test, so that our CI always runs our test suite against the next unstable kubernetes version under development.

To me PSPs seem sufficient to test via Helm test.

porridge

Please consider:

including the ticket number in the PR title
mentioning that POD_SECURITY_POLICIES is a variable we already use in CI to control PSP support in the control plane, and that we're also honoring it in deploy scripts
splitting out doc fixes to a separate PR (to not lose them in case this change needs to be rolled back for whatever reason)

I don't know whether skipping PSP installation has any impact on the core product functionality, so just running the API e2e tests might be of limited usefulness. But then again, saying "I don't know" proves it's a good idea to run them :-) so +1 on this careful approach. As you say, we can flip the flag in the 3.71 release cycle and then remove this flavor. Let's use those CircleCI credits while we still have them!

porridge · 2022-05-19T11:48:07Z

Also, since this is intended to be a preparatory no-visible-impact change, I think you can tick those last two checkboxes already?

mtesseract · 2022-05-26T20:44:05Z

Also, since this is intended to be a preparatory no-visible-impact change, I think you can tick those last two checkboxes already?

done

mtesseract · 2022-05-26T20:45:07Z

including the ticket number in the PR title

Done

mentioning that POD_SECURITY_POLICIES is a variable we already use in CI to control PSP support in the control plane, and that we're also honoring it in deploy scripts

You mean in the PR description or where?

splitting out doc fixes to a separate PR (to not lose them in case this change needs to be rolled back for whatever reason)

done

I don't know whether skipping PSP installation has any impact on the core product functionality, so just running the API e2e tests might be of limited usefulness. But then again, saying "I don't know" proves it's a good idea to run them :-) so +1 on this careful approach. As you say, we can flip the flag in the 3.71 release cycle and then remove this flavor. Let's use those CircleCI credits while we still have them!

Yeah, I am still not exactly sure what to do about the tests for the non-PSP case.

porridge · 2022-05-27T05:11:12Z

mentioning that POD_SECURITY_POLICIES is a variable we already use in CI to control PSP support in the control plane, and that we're also honoring it in deploy scripts

You mean in the PR description or where?

Yes. LGTM

ghost · 2022-06-01T19:00:16Z

Images are ready for the commit at b13a08e.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.70.x-326-ge7f0d592e3.

mtesseract · 2022-06-07T16:31:48Z

@SimonBaeumer The API e2e were removed from the CircleCI configuration (fon master, I mean). For the purpose of this PR I would like to leave it this way and skip no-psp tests for two reasons:

It is a temporary situation anyway, because the second PR will need to be merged soon

and

The previous CI test run gave positive results for the no-psp tests as they were previously configured.

Are you OK with this?

We could still open a PR for openshift/releases and extend the CI test suite there to run some smoke test in a non-PSP setting.

SimonBaeumer

LGTM!

openshift-ci · 2022-06-08T14:29:26Z

@mtesseract: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/gke-qa-e2e-tests	`b13a08e`	link	false	`/test gke-qa-e2e-tests`

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.

mtesseract · 2022-06-08T16:16:39Z

Regarding the test failures:

(openshift-4-6-operator-e2e-tests) https://app.circleci.com/pipelines/github/stackrox/stackrox/13738/workflows/1bfe6392-2320-4603-bc2a-ab3f15bdcfa4/jobs/645649: is already tracked as ROX-11141.

(openshift-4-api-e2e-tests) https://app.circleci.com/pipelines/github/stackrox/stackrox/13738/workflows/1bfe6392-2320-4603-bc2a-ab3f15bdcfa4/jobs/645609: looks like a flake.

(openshift-aro-api-e2e-tests) https://app.circleci.com/pipelines/github/stackrox/stackrox/13738/workflows/1bfe6392-2320-4603-bc2a-ab3f15bdcfa4/jobs/645631: apparently same issue.

(ci/prow/gke-qa-e2e-tests) https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/stackrox_stackrox/1249/pull-ci-stackrox-stackrox-master-gke-qa-e2e-tests/1534506729507655680 has been observed by others as well, regarding as an independent test failure.

@SimonBaeumer

github-actions bot added the area/helm label Apr 11, 2022

mtesseract added ci-openshift-4-operator-tests labels Apr 11, 2022

mtesseract force-pushed the mc/psp-removal branch from ca52e01 to 579bb3b Compare April 11, 2022 10:11

github-actions bot added the area/roxctl label Apr 12, 2022

mtesseract force-pushed the mc/psp-removal branch from 01d8d94 to ece9a14 Compare April 25, 2022 09:24

openshift-ci bot added the needs-rebase label May 8, 2022

mtesseract force-pushed the mc/psp-removal branch from ece9a14 to 2fb6f8d Compare May 16, 2022 09:11

openshift-ci bot added do-not-merge/work-in-progress and removed needs-rebase labels May 16, 2022

github-actions bot added the area/central label May 17, 2022

mtesseract changed the title ~~Deactivate PSPs~~ Make PodSecurityPolicies Opt-Out May 17, 2022

mtesseract force-pushed the mc/psp-removal branch from f0c1fde to 3b51eed Compare May 17, 2022 14:09

mtesseract removed the ci-openshift-rosa-tests label May 17, 2022

mtesseract force-pushed the mc/psp-removal branch from 5bb7917 to 515a874 Compare May 17, 2022 20:43

mtesseract marked this pull request as ready for review May 17, 2022 21:32

mtesseract requested a review from a team May 17, 2022 21:32

mtesseract requested a review from a team as a code owner May 17, 2022 21:32

openshift-ci bot removed the do-not-merge/work-in-progress label May 17, 2022

mtesseract force-pushed the mc/psp-removal branch from 515a874 to d02c258 Compare May 18, 2022 07:14

mtesseract commented May 18, 2022

View reviewed changes

SimonBaeumer self-requested a review May 18, 2022 12:24

mtesseract requested a review from porridge May 19, 2022 07:10

SimonBaeumer suggested changes May 19, 2022

View reviewed changes

porridge approved these changes May 19, 2022

View reviewed changes

mtesseract changed the title ~~Make PodSecurityPolicies Opt-Out~~ ROX-10034: Make PodSecurityPolicies Opt-Out May 26, 2022

mtesseract force-pushed the mc/psp-removal branch from 022fb3f to 3fb087b Compare May 26, 2022 20:45

mtesseract force-pushed the mc/psp-removal branch from ec47717 to 8ad8b24 Compare May 27, 2022 07:17

mtesseract requested a review from SimonBaeumer May 27, 2022 07:27

mtesseract force-pushed the mc/psp-removal branch 3 times, most recently from 0cfdeb8 to 54bf9f1 Compare June 1, 2022 07:07

mtesseract force-pushed the mc/psp-removal branch 2 times, most recently from a7b931c to 67b38af Compare June 2, 2022 14:40

mtesseract added the ci-api-e2e-tests-no-psps label Jun 2, 2022

openshift-ci bot added the needs-rebase label Jun 6, 2022

Make PodSecurityPolicies opt-out

5d03cec

mtesseract force-pushed the mc/psp-removal branch from e5cf243 to 5d03cec Compare June 7, 2022 16:29

openshift-ci bot removed the needs-rebase label Jun 7, 2022

SimonBaeumer approved these changes Jun 8, 2022

View reviewed changes

CI

b13a08e

mtesseract merged commit 1771472 into master Jun 8, 2022

mtesseract deleted the mc/psp-removal branch June 8, 2022 21:17

	enableDeprecatedPodSecurityPolicies: null # bool
	enablePodSecurityPolicies: null # bool

		@@ -0,0 +1,69 @@
		{{- include "srox.init" . -}}

		{{- if and (not ._rox.scanner.disable) ._rox.system.enableDeprecatedPodSecurityPolicies }}

		@@ -1,5 +1,6 @@
		{{- include "srox.init" . -}}

		{{- if ._rox.system.enableDeprecatedPodSecurityPolicies }}

	{{- if ._rox.system.enableDeprecatedPodSecurityPolicies }}
	{{- if ._rox.system.enablePodSecurityPolicies }}

	DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`
	DisableDeprecatedPodSecurityPolicies bool `json:"enablePodSecurityPolicies"`


		IstioVersion string `json:"istioVersion"`

		DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`

	DisableDeprecatedPodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`
	DisablePodSecurityPolicies bool `json:"disableDeprecatedPodSecurityPolicies"`

Conversation

mtesseract commented Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

ghost commented Apr 11, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtesseract commented May 17, 2022

Uh oh!

mtesseract commented May 17, 2022

Uh oh!

mtesseract commented May 17, 2022

Uh oh!

openshift-ci bot commented May 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SimonBaeumer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

porridge left a comment

Choose a reason for hiding this comment

Uh oh!

porridge commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtesseract commented May 26, 2022

Uh oh!

mtesseract commented May 26, 2022

Uh oh!

porridge commented May 27, 2022

Uh oh!

ghost commented Jun 1, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtesseract commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonBaeumer left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jun 8, 2022

Uh oh!

mtesseract commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mtesseract commented Apr 11, 2022 •

edited

Loading

ghost commented Apr 11, 2022 •

edited by ghost

Loading

porridge commented May 19, 2022 •

edited

Loading

ghost commented Jun 1, 2022 •

edited by ghost

Loading

mtesseract commented Jun 7, 2022 •

edited

Loading

mtesseract commented Jun 8, 2022 •

edited

Loading