ROX-10034: Deactivate PodSecurityPolicies by default by mtesseract · Pull Request #1747 · stackrox/stackrox

mtesseract · 2022-05-17T14:21:51Z

Description

This PR is built on top of #1249 and toggles the defaults for generating deprecated PodSecurityPolicies effectively disabling them by default while still supporting explicit generation of them.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

TODO(replace-me)
Use this space to explain how you tested your PR, or, if you didn't test it, why
you did not do so. Valid reasons include, for example, "CI is sufficient",
"No testable changes". Feel free to attach JSON snippets, curl commands,
screenshots.

In addition to reviewing your code, reviewers must also review your testing
instructions and make sure they are sufficient.

openshift-ci · 2022-05-17T14:21:55Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

ghost · 2022-05-17T14:44:22Z

Tag for build #686872 is 3.70.x-438-gf1fd3fce33.

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

export MAIN_IMAGE_TAG='3.70.x-438-gf1fd3fce33'

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

ghost · 2022-06-01T18:47:49Z

Images are ready for the commit at f1fd3fc.

To use with deploy scripts, first export MAIN_IMAGE_TAG=3.70.x-439-g2de0900833.

SimonBaeumer

LGTM from a code perspective.
Can you write in the PR description why PSPs are disabled by default for documentation purposes?

To be honest I forgot why we disable them by default immediately as kube 1.25 is not released yet and unsure about the impact it might has on roxctl based installations.

pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml

roxctl/scanner/generate/generate.go

SimonBaeumer

Only two small changes

roxctl/central/generate/generate.go

SimonBaeumer · 2022-06-13T19:56:51Z

roxctl/central/generate/generate.go

 	logger.InfofLn("Generating deployment bundle...")

+	if config.EnablePodSecurityPolicies {
+		logger.InfofLn("Central deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")


If the message excludes the deployment name it can be extracted into a function which can be called for all components and reducing duplicated code in all three places.

Suggested change

logger.InfofLn("Central deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")

logger.InfofLn("Deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")

SimonBaeumer · 2022-06-13T19:58:12Z

roxctl/central/generate/generate.go


+	if config.EnablePodSecurityPolicies {
+		logger.InfofLn("Central deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")
+		logger.InfofLn("Use --enable-pod-security-policies=false for disabling PodSecurityPolicies.")


I am not a grammar expert, but using this form sounds cleaner to me:

Suggested change

logger.InfofLn("Use --enable-pod-security-policies=false for disabling PodSecurityPolicies.")

logger.InfofLn("Use --enable-pod-security-policies=false to disable PodSecurityPolicies.")

I think you are right.

mtesseract · 2022-06-14T22:35:21Z

I have decided to implement auto-sensing in this PR, since otherwise master would be in a somewhat inconsistent state: deployment bundles have PSPs enabled by default and Helm charts have PSPs disabled by default.
Now the Helm charts use auto-sensing and should just work.

SimonBaeumer

Auto-Sensing LGTM!

Small notes:

Extracting into one func the if-cases are missing
Conflict with CHANGELOG

SimonBaeumer · 2022-06-20T08:27:46Z

pkg/helm/charts/tests/securedclusterservices/testdata/helmtest/psp-autosense.test.yaml

+    values:
+      meta:
+        apiServer:
+          overrideAPIResources:


Interesting, never saw this, would like to learn a bit about it.
How does this work? Can you share a documentation link?

Malte came up with this. It's an abstraction layer on top of Helm's lookup functionality which is quite convenient to use and valuable in particular for testing purposes, allows mocking (un)availability of APIs.

I don't have a pointer to documentation for this unfortunately.

SimonBaeumer · 2022-06-20T08:29:55Z

roxctl/central/generate/generate.go

 func OutputZip(logger logger.Logger, config renderer.Config) error {
 	logger.InfofLn("Generating deployment bundle...")

+	if config.EnablePodSecurityPolicies {


The logic of the if-case is still duplicated.
Moving this if-case to a function printPSPInfoLog(config.EnablePodSecurityPolicies) and call it from all other occurrences of this clause.

SimonBaeumer · 2022-06-20T08:30:06Z

roxctl/scanner/generate/generate.go


-func (cmd *scannerGenerateCommand) generate() error {
+func (cmd *scannerGenerateCommand) generate(logger logger.Logger) error {
+	if cmd.enablePodSecurityPolicies {


Extract into one func

SimonBaeumer · 2022-06-20T08:30:23Z

roxctl/sensor/generate/generate.go

 	}
 	s.setClusterDefaults(env)

+	if s.enablePodSecurityPolicies {


Extract into one func

…and adjust CI

…rds compatibility. Emit info logs in roxctl generate commands.

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

github-actions bot added area/central area/helm area/roxctl labels May 17, 2022

mtesseract added ci-openshift-4-operator-tests and removed area/roxctl area/central area/helm labels May 17, 2022

mtesseract force-pushed the mc/psp-deactivated branch from 61238b0 to 38763b1 Compare May 17, 2022 15:40

github-actions bot added area/central area/helm area/roxctl labels May 17, 2022

mtesseract force-pushed the mc/psp-deactivated branch from 818526b to fb1d56f Compare May 18, 2022 08:23

mtesseract mentioned this pull request May 18, 2022

ROX-10034: Make PodSecurityPolicies Opt-Out #1249

Merged

5 tasks

mtesseract marked this pull request as ready for review May 18, 2022 08:27

mtesseract requested a review from a team May 18, 2022 08:27

mtesseract requested a review from a team as a code owner May 18, 2022 08:27

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

mtesseract changed the title ~~Deactivate PSPs~~ Deactivate PodSecurityPolicies by default May 18, 2022

mtesseract force-pushed the mc/psp-deactivated branch from fb1d56f to 1da8696 Compare May 19, 2022 07:16

mtesseract removed the request for review from a team May 23, 2022 14:02

mtesseract force-pushed the mc/psp-deactivated branch 2 times, most recently from 5e4b22e to aa2ee54 Compare May 29, 2022 22:28

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

mtesseract force-pushed the mc/psp-deactivated branch from aa2ee54 to aae9cbb Compare June 8, 2022 10:53

SimonBaeumer suggested changes Jun 9, 2022

View reviewed changes

pkg/helm/charts/tests/centralservices/testdata/helmtest/central.test.yaml Outdated Show resolved Hide resolved

roxctl/scanner/generate/generate.go Outdated Show resolved Hide resolved

mtesseract requested a review from SimonBaeumer June 9, 2022 10:19

SimonBaeumer suggested changes Jun 13, 2022

View reviewed changes

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

mtesseract requested a review from SimonBaeumer June 14, 2022 22:33

mtesseract force-pushed the mc/psp-deactivated branch from 365cb7e to 8ada6f7 Compare June 14, 2022 22:36

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

mtesseract changed the title ~~Deactivate PodSecurityPolicies by default~~ ROX-10034: Deactivate PodSecurityPolicies by default Jun 15, 2022

SimonBaeumer suggested changes Jun 20, 2022

View reviewed changes

mtesseract added 14 commits June 20, 2022 11:08

X-Smart-Branch-Parent: master

230dcdb

Invert logic for generating PSPs: Mak them opt-in instead of opt-out …

8d16a4e

…and adjust CI

Fix tests

ce5c046

Set PSP flags in roxctl to false

b3d4e2c

CI

e3d442a

Fix test name

b5c2279

Revert defaults for --enable-pod-security-policies to true for backwa…

c24a490

…rds compatibility. Emit info logs in roxctl generate commands.

Revert changes to scanner tests

8e472ad

Factor out PSP message

39d7ee2

Implement auto-sensing for PSPs

c9775c4

Add Tests

0dfd255

Added CHANGELOG entry

9fbc1a9

Fix

94502ef

Factor out more PSP Info logging logic

f1fd3fc

mtesseract force-pushed the mc/psp-deactivated branch from 19c078c to f1fd3fc Compare June 20, 2022 09:17

mtesseract requested a review from SimonBaeumer June 20, 2022 09:17

SimonBaeumer approved these changes Jun 20, 2022

View reviewed changes

mtesseract merged commit 7856a66 into master Jun 20, 2022

mtesseract deleted the mc/psp-deactivated branch June 20, 2022 20:47

Maddosaurus mentioned this pull request Feb 1, 2023

ROX-14485: update monitoring psp #4655

Merged

5 tasks

	logger.InfofLn("Central deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")
	logger.InfofLn("Deployment bundle includes PodSecurityPolicies (PSPs). This is incompatible with Kubernetes >= v1.25.")

	logger.InfofLn("Use --enable-pod-security-policies=false for disabling PodSecurityPolicies.")
	logger.InfofLn("Use --enable-pod-security-policies=false to disable PodSecurityPolicies.")

Conversation

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

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

openshift-ci bot commented May 17, 2022

Uh oh!

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

Uh oh!

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!

SimonBaeumer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SimonBaeumer left a comment

Choose a reason for hiding this comment

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!

mtesseract commented Jun 14, 2022

Uh oh!

SimonBaeumer left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mtesseract commented May 17, 2022 •

edited

Loading

ghost commented May 17, 2022 •

edited by ghost

Loading

ghost commented Jun 1, 2022 •

edited by ghost

Loading

SimonBaeumer left a comment •

edited

Loading