-
Notifications
You must be signed in to change notification settings - Fork 88
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
add pod & container security context Che Cluster Fields #1729
add pod & container security context Che Cluster Fields #1729
Conversation
Hi @AObuchow. Thanks for your PR. I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
fromDropCapabilitiesMap := getCapabilitiesSet(from.Drop) | ||
|
||
// Remove add capabilities that are being dropped | ||
for dropCapability := range fromDropCapabilitiesMap { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: for this PR, we don't need to be removing add capabilities that are dropped in from
, since from
is constants.DefaultWorkspaceContainerSecurityContext.Capabilities
which does not drop any capabilities.
However, I figured it'd make more sense to handle all cases, even if they aren't needed.
Codecov Report
@@ Coverage Diff @@
## main #1729 +/- ##
==========================================
- Coverage 60.06% 59.84% -0.22%
==========================================
Files 71 71
Lines 8423 8490 +67
==========================================
+ Hits 5059 5081 +22
- Misses 3017 3060 +43
- Partials 347 349 +2
|
api/v2/checluster_types.go
Outdated
@@ -121,8 +121,17 @@ type CheClusterDevEnvironments struct { | |||
// +kubebuilder:default:=-1 | |||
SecondsOfRunBeforeIdling *int32 `json:"secondsOfRunBeforeIdling,omitempty"` | |||
// Disables the container build capabilities. | |||
// DEPRECATED: Usage of the containerSecurityContext and podSecurityContext fields is recommended instead to configure privileges required for container builds. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am against marking this field as DEPRECATED.
It is still a valid and easy way to enable container build capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is still a valid and easy way to enable container build capabilities.
I agree @tolusha and am open to leaving it as-is.
@l0rd WDYT?
Perhaps its better to just modify the disableContainerBuildCapabilities documentation to mention that it overrides the containerSecurityContext (or have that noted on the containerSecurityContext documentation)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Container build capabilities should enabled by default. So this is easy by default.
Now, do an admin need an easy way to "disable container build capabilities"? I don't think so. The admin that wants to disable those capability has probably in mind a precise set of capabilities that he wants to grant to the developers and will need the granularity of the new fields.
We currently have:
I think we should have instead:
or even
with the defaults securityContext being the bare minimum to enable building containers. |
I'm +1 for this, though I'm not sure how the process for removing Kubernetes CRD fields works. Won't existing Che Cluster CR instances that have Moreover, moving @tolusha if you have any advice on any precautions I need to take in order to accomplish this, please let me know. |
We can easily remove fields from a CRD. In this case the corresponding field will be removed from a CR a well. Unfortunately, we can't move fields. In order to do that, we need to introduce a new CheCluster API version. So, let's not introduce or keep
|
I am ok with @tolusha proposal but I want to make sure we are on the same page:
|
Re: removing fields in the CRD (e.g. removing When updating Operators, OLM checks all CRs on the cluster to verify that they are still valid in the new operator version, and fails the upgrade if they are not. This is somewhat subtle:
|
bf955aa
to
4e4e94f
Compare
4e4e94f
to
e05092f
Compare
I've updated the PR & the PR description:
I did not mark |
I believe we shouldn't mark this field as deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from @tolusha's comments
@AObuchow no I am ok with that. But I would mention in the doc that, when set to false (the default), containerSecurityContext field will be ignored and that a specific containerSecurityContext will be applied (with the details of the containerSecurityContext). |
Also modify devEnvironments.disableContainerBuildCapabilities field documentation to mention it overrides devEnvironments.security.containerSecurityContext when set to false. Signed-off-by: Andrew Obuchowicz <aobuchow@redhat.com>
Build 3.9 :: sync-to-downstream_3.x/4000: Console, Changes, Git Data |
Build 3.9 :: push-latest-container-to-quay_3.x/2934: Console, Changes, Git Data |
Build 3.9 :: copyIIBsToQuay/1690: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4000: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/3854 triggered; /job/DS_CI/job/dsc_3.x triggered; |
Build 3.9 :: operator-bundle_3.x/1746: Upstream sync done; /DS_CI/sync-to-downstream_3.x/4000 triggered |
Build 3.9 :: dsc_3.x/1172: Console, Changes, Git Data |
Build 3.9 :: dsc_3.x/1172: 3.9.0 CI |
Build 3.9 :: operator-bundle_3.x/1747: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4001: Console, Changes, Git Data |
Build 3.9 :: push-latest-container-to-quay_3.x/2935: Console, Changes, Git Data |
Build 3.9 :: copyIIBsToQuay/1691: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4001: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/3855 triggered; /job/DS_CI/job/dsc_3.x triggered; |
Build 3.9 :: operator-bundle_3.x/1747: Upstream sync done; /DS_CI/sync-to-downstream_3.x/4001 triggered |
Build 3.9 :: dsc_3.x/1173: Console, Changes, Git Data |
Build 3.9 :: dsc_3.x/1173: 3.9.0 CI |
Build 3.9 :: operator-bundle_3.x/1749: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4004: Console, Changes, Git Data |
Build 3.9 :: push-latest-container-to-quay_3.x/2938: Console, Changes, Git Data |
Build 3.9 :: copyIIBsToQuay/1693: Console, Changes, Git Data |
Build 3.9 :: sync-to-downstream_3.x/4004: Build container: devspaces-operator-bundle synced; /DS_CI/get-sources-rhpkg-container-build_3.x/3858 triggered; /job/DS_CI/job/dsc_3.x triggered; |
Build 3.9 :: operator-bundle_3.x/1749: Upstream sync done; /DS_CI/sync-to-downstream_3.x/4004 triggered |
Build 3.9 :: dsc_3.x/1176: Console, Changes, Git Data |
Build 3.9 :: dsc_3.x/1176: 3.9.0 CI |
What does this PR do?
This PR introduces a couple of changes:
Adds 3 new Che Cluster CR fields:
spec.devEnvironments.security
,spec.devEnvironments.security.containerSecurityContext
andspec.devEnvironments.security.podSecurityContext
. The latter two fields allow configuring the pod and security contexts used by workspaces by setting the corresponding DWOC fields.When the
devEnvironments.security.containerSecurityContext
field is used anddevEnvironments.disableContainerBuildCapabilities
is set tofalse
, the container security context required for thecontainer-builds
SCC will be used, overriding the security context set indevEnvironments.security.containerSecurityContext
.Some house-keeping:
cmp.diff
optionsupdateWorkspacePodSchedulerNameConfig
functionScreenshot/screencast of this PR
n/a
What issues does this PR fix or reference?
Fix eclipse-che/che#22307
How to test this PR?
Install Che with the changes from this PR. On OpenShift, run
build/scripts/olm/test-catalog-from-sources.sh
Normal usage
Disable container builds by setting
spec.devEnvironments.disableContainerBuildCapabilities: true
in the Che Cluster CR:kubectl edit checluster eclipse-che -n eclipse-che
Configure
spec.devEnvironments.containerSecurityContext
andspec.devEnvironments.podSecurityContext
in the Che Cluster CR:kubectl edit checluster eclipse-che -n eclipse-che
For example:
Check that the containerSecurityContext and podSecurityContext are propagated to the Che-owned DWOC:
kubectl describe dwoc -n eclipse-che
Container builds enabled case
Enable container builds by setting
spec.devEnvironments.disableContainerBuildCapabilities: false
in the Che Cluster CR:kubectl edit checluster eclipse-che -n eclipse-che
Any configuration to
spec.devEnvironments.containerSecurityContext
in the Che Cluster CR should be ignored, and instead, the security context required for container builds should be used. This can be verified by checking the DWOC's containerSecurityContext, it should haveallowPrivilegeEscalation: true
and theSETGID
,SETUID
add capabilities :PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.