Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove PodSecurityPolicy from requiredPlugins and add a user-facing warning to consider migration #6700

Merged
merged 10 commits into from
Oct 19, 2022

Conversation

shafeeqes
Copy link
Contributor

@shafeeqes shafeeqes commented Sep 20, 2022

How to categorize this PR?

/area usability control-plane testing
/kind enhancement

What this PR does / why we need it:
This PR adds a user-faced warning for shoots >= v1.23 and < v1.25, to consider migrating from PodSecurityPolicy to PodSecurity admission controller.
This PR also removes the PodSecurityPolicy from the required plugins.
The e2e tests for create-update-delete shoot is also enhanced.

Which issue(s) this PR fixes:
Part of #5250

Special notes for your reviewer:
/hold until gardener/gardener-extension-provider-openstack#485 is released.

Release note:

It is now possible to disable `PodSecurityPolicy` admission plugin, please make sure you have updated the extensions to a version which supports this change.

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 20, 2022

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

@gardener-prow gardener-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/usability Usability related area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension labels Sep 20, 2022
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Sep 20, 2022

@shafeeqes: The label(s) area/test cannot be applied, because the repository doesn't have them.

In response to this:

How to categorize this PR?

/area usability control-plane test
/kind enhancement

What this PR does / why we need it:
This PR adds a user-faced warning for shoots >= v1.23 and < v1.25, to consider migrating from PodSecurityPolicy to PodSecurity admission controller.
This PR also removes the PodSecurityPolicy from the required plugins.

Which issue(s) this PR fixes:
Part of #5250

Special notes for your reviewer:
/hold until gardener/gardener-extension-provider-openstack#485 is released.

Release note:

NONE

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.

@gardener-prow gardener-prow bot added the cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. label Sep 20, 2022
@gardener-prow gardener-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 20, 2022
@shafeeqes shafeeqes changed the title Enh/warning psp Remove PodSecurityPolicy from requiredPlugins and add a user-facing warning to consider migration Sep 20, 2022
@shafeeqes
Copy link
Contributor Author

/test all

@shafeeqes
Copy link
Contributor Author

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

Test name Commit Details Required Rerun command
pull-gardener-check-vulnerabilities 70c6179 link false /test pull-gardener-check-vulnerabilities
pull-gardener-e2e-kind 70c6179 link true /test pull-gardener-e2e-kind
Full PR test history. Your PR dashboard. Command help for this repository. Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

 Expected success, but got an error:
      <*retry.Error | 0xc000512a40>: {
          ctxError: <context.deadlineExceededError>{},
          err: <*errors.StatusError | 0xc000827180>{
              ErrStatus: {
                  TypeMeta: {Kind: "", APIVersion: ""},
                  ListMeta: {
                      SelfLink: "",
                      ResourceVersion: "",
                      Continue: "",
                      RemainingItemCount: nil,
                  },
                  Status: "Failure",
                  Message: "Shoot.core.gardener.cloud \"e2e-default\" is invalid: [spec.kubernetes.version: Forbidden: admission plugin \"PodSecurityPolicy\" should be disabled for Kubernetes versions >=1.25, please check https://github.com/gardener/gardener/blob/master/docs/usage/pod-security.md#migrating-from-podsecuritypolicys-to-podsecurity-admission-controller, spec.kubernetes.allowPrivilegedContainers: Forbidden: for Kubernetes versions >= 1.25, allowPrivilegedContainers field should not be set, please see https://github.com/gardener/gardener/blob/master/docs/usage/pod-security.md#speckubernetesallowprivilegedcontainers-in-the-shoot-spec]",
                  Reason: "Invalid",
                  Details: {
                      Name: "e2e-default",
                      Group: "core.gardener.cloud",
                      Kind: "Shoot",
                      UID: "",
                      Causes: [
                          {
                              Type: "FieldValueForbidden",
                              Message: "Forbidden: admission plugin \"PodSecurityPolicy\" should be disabled for Kubernetes versions >=1.25, please check https://github.com/gardener/gardener/blob/master/docs/usage/pod-security.md#migrating-from-podsecuritypolicys-to-podsecurity-admission-controller",
                              Field: "spec.kubernetes.version",
                          },
                          {
                              Type: "FieldValueForbidden",
                              Message: "Forbidden: for Kubernetes versions >= 1.25, allowPrivilegedContainers field should not be set, please see https://github.com/gardener/gardener/blob/master/docs/usage/pod-security.md#speckubernetesallowprivilegedcontainers-in-the-shoot-spec",
                              Field: "spec.kubernetes.allowPrivilegedContainers",
                          },
                      ],
                      RetryAfterSeconds: 0,
                  },
                  Code: 422,
              },
          },
      }

This failure is expected. Will now fix this in another commit.

@shafeeqes shafeeqes force-pushed the enh/warning-psp branch 2 times, most recently from 2556f05 to 071cc71 Compare September 21, 2022 04:54
@shafeeqes shafeeqes marked this pull request as ready for review September 21, 2022 04:54
@gardener-prow gardener-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2022
@shafeeqes shafeeqes force-pushed the enh/warning-psp branch 2 times, most recently from 71ee96f to 10216f8 Compare September 21, 2022 05:32
@rfranzke
Copy link
Member

/assign

@shafeeqes
Copy link
Contributor Author

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

Test name Commit Details Required Rerun command
pull-gardener-e2e-kind 10216f8 link true /test pull-gardener-e2e-kind
pull-gardener-check-vulnerabilities c10e8fb link false /test pull-gardener-check-vulnerabilities
pull-gardener-integration c10e8fb link true /test pull-gardener-integration
Full PR test history. Your PR dashboard. Command help for this repository. Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

Related to #6671
/test pull-gardener-integration

@shafeeqes shafeeqes force-pushed the enh/warning-psp branch 2 times, most recently from 989077e to e07f2e9 Compare September 21, 2022 08:05
@shafeeqes
Copy link
Contributor Author

/retest

@shafeeqes
Copy link
Contributor Author

shafeeqes commented Oct 6, 2022

Is it safe to remove the check if PodSecurityPolicy is required for k8s versions before 1.22.0? IIUC this could cause some problems for older versions of the extensions that were not adapted to handle missing PSPs (@shafeeqes)?

This extension issue can still happen for versions > 1.22 also, right?

@shafeeqes
Copy link
Contributor Author

shafeeqes commented Oct 6, 2022

I added a release note asking to update the extensions to versions which support this change. But we don't have a compatibility matrix as of now.

@acumino acumino added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 6, 2022
@rfranzke
Copy link
Member

rfranzke commented Oct 6, 2022

PSP is not really required for a functional cluster, is it? If it was disabled then there would simply be no effect (it's like "unprivileged" mode).

@gardener-prow gardener-prow bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2022
@gardener-prow gardener-prow bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 7, 2022
@shafeeqes
Copy link
Contributor Author

PSP is not really required for a functional cluster, is it? If it was disabled then there would simply be no effect (it's like "unprivileged" mode).

Did you mean to say "privileged"? Without the PSP admission plugin, there won't be any "restrictions" for the pods.

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Oct 16, 2022

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

Test name Commit Details Required Rerun command
pull-dependency-watchdog-verify-image-build 3ed9cf0 link true /test pull-dependency-watchdog-verify-image-build
pull-gardener-check-vulnerabilities e6f1a78 link false /test pull-gardener-check-vulnerabilities

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

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.

@acumino
Copy link
Member

acumino commented Oct 19, 2022

New provider-openstack version is released, We can now unhold this PR /cc @shafeeqes .

Copy link
Member

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2022
@rfranzke
Copy link
Member

/milestone v1.58

@gardener-prow gardener-prow bot added this to the v1.58 milestone Oct 19, 2022
@shafeeqes
Copy link
Contributor Author

/unhold
All the extensions are adapted with necessary change and released.

@gardener-prow gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 19, 2022
@gardener-prow gardener-prow bot merged commit 486cf71 into gardener:master Oct 19, 2022
@shafeeqes shafeeqes deleted the enh/warning-psp branch December 1, 2022 04:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane Control plane related area/usability Usability related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/enhancement Enhancement, improvement, extension lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants