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

Document helm chart PodSecurityPolicy deprecation and PodSecurityAdmission alternative #7626

Merged
merged 1 commit into from
Mar 6, 2023

Conversation

jmhbnz
Copy link
Member

@jmhbnz jmhbnz commented Feb 17, 2023

Pod security policy has been removed from kubernetes 1.25 onwards. The successor pod security admission is not fully supported in helm as it involved labeling the release namespace which helm does not support. Refer helm/helm#3503.

Additionally since pod security admission is managed at the namespace level we probably shouldn't touch it unless we can be certain we are the sole namespace tenant.

Perhaps we should just be updating documentation to outline how users of the chart can label their namespace post chart deployment to add a pod security policy label?

Starting this pull request as a draft to generate some discussion on next steps and hopefully get a consensus.

Fixes: #7608

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2023
@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #7626 (d4b1ca6) into master (bc518f7) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7626      +/-   ##
==========================================
- Coverage   42.16%   42.16%   -0.01%     
==========================================
  Files         217      217              
  Lines       12092    12092              
  Branches      179      179              
==========================================
- Hits         5099     5098       -1     
- Misses       6708     6709       +1     
  Partials      285      285              

@jmhbnz jmhbnz marked this pull request as ready for review February 20, 2023 17:21
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 20, 2023
@desaintmartin
Copy link
Member

desaintmartin commented Feb 23, 2023

The current way does not block using Kubernetes 1.25+ since one just has to disable PSP. Moreover, it is disabled by default. So it depends how much time we want to support Kubernetes 1.24-, but I would not merge this now since we don't really have a reason to actually break deployments still relying on PSP. It can be present for a long time, maybe with a comment stating it is not compatible with 1.25+.

Regarding it's successor, it goes against the Helm philosophy, but indeed we could add one, disabled by default, clearly documenting in the values the limitations. Usually, the kubernetes-dashboard namespace only contains one dashboard, so it would secure most of the deployments.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2023
@jmhbnz
Copy link
Member Author

jmhbnz commented Feb 25, 2023

The current way does not block using Kubernetes 1.25+ since one just has to disable PSP. Moreover, it is disabled by default. So it depends how much time we want to support Kubernetes 1.24-, but I would not merge this now since we don't really have a reason to actually break deployments still relying on PSP. It can be present for a long time, maybe with a comment stating it is not compatible with 1.25+.

Regarding it's successor, it goes against the Helm philosophy, but indeed we could add one, disabled by default, clearly documenting in the values the limitations. Usually, the kubernetes-dashboard namespace only contains one dashboard, so it would secure most of the deployments.

Good point, you're right we should not break older deployments still wanting PodSecurityPolicy. I've updated to a more middle ground stance and made it clear in docs about the deprecation and the namespace level PodSecurityAdmission alternative.

/retitle Document helm chart PodSecurityPolicy deprecation and PodSecurityAdmission alternative

@k8s-ci-robot
Copy link
Contributor

@jmhbnz: Re-titling can only be requested by trusted users, like repository collaborators.

In response to this:

The current way does not block using Kubernetes 1.25+ since one just has to disable PSP. Moreover, it is disabled by default. So it depends how much time we want to support Kubernetes 1.24-, but I would not merge this now since we don't really have a reason to actually break deployments still relying on PSP. It can be present for a long time, maybe with a comment stating it is not compatible with 1.25+.

Regarding it's successor, it goes against the Helm philosophy, but indeed we could add one, disabled by default, clearly documenting in the values the limitations. Usually, the kubernetes-dashboard namespace only contains one dashboard, so it would secure most of the deployments.

Good point, you're right we should break older deployments still wanting PodSecurityPolicy. I've updated to a more middle ground stance and made it clear in docs about the deprecation and the namespace level PodSecurityAdmission alternative.

/retitle Document helm chart PodSecurityPolicy deprecation and PodSecurityAdmission alternative

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.

@jmhbnz jmhbnz changed the title Remove deprecated pod security policy. Document helm chart PodSecurityPolicy deprecation and PodSecurityAdmission alternative Feb 25, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2023
@desaintmartin
Copy link
Member

Thanks!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: desaintmartin, jmhbnz

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:

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

@desaintmartin
Copy link
Member

/retest

@jmhbnz
Copy link
Member Author

jmhbnz commented Feb 27, 2023

@desaintmartin It looks like the e2e tests ran into issues but /retest doesn't seem to have restarted them.

Should I raise an issue for these tests it looks like they are taking a long time to run and flaky?

Let me know if there is anything you need me to do at my end to retrigger these.

@desaintmartin
Copy link
Member

Indeed!
@maciaszczykm, could you re-run the test?
or @jmhbnz, you could just rebase in order to trigger the tests again?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 2, 2023
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@shu-mutou
Copy link
Contributor

Thank you for tackling this!
I aware of that we have trouble in CI/CD workflow, but no errors found from logs. So we may need time to fix e2e test, I think.
I guess the workflow has trouble in building for now.

@shu-mutou
Copy link
Contributor

To rerun tests, open the details window by clicking Details link in the check list.
screenshot-github com-2023 03 02-18_09_38

Then, push Re-run all jobs button in the details window, if you find.
screenshot-github com-2023 03 02-18_11_01

@desaintmartin
Copy link
Member

@shu-mutou I don't have permissions to re-run tests, unfortunately!

Add documentation for podsecurityadmission at namespace level as an alternative.

Signed-off-by: James Blair <mail@jamesblair.net>
@jmhbnz
Copy link
Member Author

jmhbnz commented Mar 5, 2023

Hey @shu-mutou I have tried rebasing a few times to retrigger tests but the e2e test keeps timing out. Are you able to merge this as is?

@maciaszczykm
Copy link
Member

End-to-end tests are flaky at the moment. I will merge manually.

@maciaszczykm maciaszczykm merged commit ac08146 into kubernetes:master Mar 6, 2023
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate PSP to v1 in helm chart
5 participants