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

add pod & container security context Che Cluster Fields #1729

Merged
merged 7 commits into from
Aug 10, 2023

Conversation

AObuchow
Copy link
Contributor

@AObuchow AObuchow commented Jul 11, 2023

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 and spec.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 and devEnvironments.disableContainerBuildCapabilities is set to false, the container security context required for the container-builds SCC will be used, overriding the security context set in devEnvironments.security.containerSecurityContext.

Some house-keeping:

  • Moved all the DWOC reconciler tests into separate functions
  • Cleaned up some DWOC reconciler test cases by removing expected fields that are unrelated to the test cases, and ignoring these fields in the cmp.diff options
  • Cleaned up some DWOC reconciler test case names for accuracy
  • Removed an unused error in the DWOC reconciler updateWorkspacePodSchedulerNameConfig function

Screenshot/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 and spec.devEnvironments.podSecurityContext in the Che Cluster CR: kubectl edit checluster eclipse-che -n eclipse-che

For example:

spec:  
(...)
 devEnvironments:  
   disableContainerBuildCapabilities: true  
   containerBuildConfiguration:  
     openShiftSecurityContextConstraint: container-build  
   containerSecurityContext:  
     allowPrivilegeEscalation: false  
     capabilities:  
       add:  
       - SYS_TIME  
       - CHOWN  
       drop:  
       - SETUID  
       - KILL  
       - SETGID  
     privileged: false  
     runAsUser: 1000690001
   podSecurityContext:  
     runAsGroup: 0  
     runAsNonRoot: false  
     runAsUser: 1000690001

Check that the containerSecurityContext and podSecurityContext are propagated to the Che-owned DWOC: kubectl describe dwoc -n eclipse-che

Kind:                    DevWorkspaceOperatorConfig
API Version:  controller.devfile.io/v1alpha1  
Config:  
 Workspace:  
   Container Security Context:  
     Allow Privilege Escalation:  false  
     Capabilities:  
       Add:  
         SYS_TIME  
         CHOWN  
       Drop:  
         SETUID  
         KILL  
         SETGID  
     Privileged:         false  
     Run As User:        1000690001  
   Deployment Strategy:  Recreate  
   Pod Security Context:  
     Run As Group:     0  
     Run As Non Root:  false  
     Run As User:      1000690001  
   Progress Timeout:   300s  
   Service Account:  
     Disable Creation:  false

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 have allowPrivilegeEscalation: true and the SETGID, SETUID add capabilities :

config:  
 workspace:  
   containerSecurityContext:  
     allowPrivilegeEscalation: true  
     capabilities:  
       add:  
       - SETGID  
       - SETUID  
   deploymentStrategy: Recreate  
   progressTimeout: 300s  
   serviceAccount:  
     disableCreation: false

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

@openshift-ci
Copy link

openshift-ci bot commented Jul 11, 2023

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 {
Copy link
Contributor Author

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
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #1729 (5709d98) into main (923972e) will decrease coverage by 0.22%.
Report is 6 commits behind head on main.
The diff coverage is 36.48%.

❗ Current head 5709d98 differs from pull request most recent head c6b9505. Consider uploading reports for the commit c6b9505 to get more accurate results

@@            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     
Files Changed Coverage Δ
api/v2/checluster_types.go 22.64% <ø> (ø)
api/v2/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
...eploy/dev-workspace-config/dev_workspace_config.go 91.35% <77.14%> (-2.68%) ⬇️

@@ -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.
Copy link
Contributor

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.

Copy link
Contributor Author

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)?

Copy link
Contributor

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.

@l0rd
Copy link
Contributor

l0rd commented Jul 13, 2023

We currently have:

devEnvironments:
  containerBuildConfiguration:
    openShiftSecurityContextConstraint: container-build
  disableContainerBuildCapabilities: true

I think we should have instead:

devEnvironments:
  openShiftSecurityContextConstraint: container-build
  containerSecurityContext:
    ...
  podSecurityContext:
   ...

or even

devEnvironments:
  security:
    openShiftSecurityContextConstraint: container-build
    containerSecurityContext:
      ...
    podSecurityContext:
     ...

with the defaults securityContext being the bare minimum to enable building containers.

@AObuchow
Copy link
Contributor Author

or even

devEnvironments:
  security:
    openShiftSecurityContextConstraint: container-build
    containerSecurityContext:
      ...
    podSecurityContext:
     ...

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.
@tolusha what are your thoughts?

Won't existing Che Cluster CR instances that have disableContainerBuildCapabilities set become invalid if disableContainerBuildCapabilities is removed from the CRD? Maybe this issue was alleviated by eclipse-che/che#22064, as disableContainerBuildCapabilities doesn't appear in the Che Cluster CR spec when instantiated, even though it's set to true.

Moreover, moving devEnvironments.openShiftSecurityContextConstraint to devEnvironments.security.openShiftSecurityContextConstraint would also break some (currently valid) Che Cluster CR instances I think.

@tolusha if you have any advice on any precautions I need to take in order to accomplish this, please let me know.

@tolusha
Copy link
Contributor

tolusha commented Jul 14, 2023

We can easily remove fields from a CRD. In this case the corresponding field will be removed from a CR a well.
Notice, that container build capabilities are disabled by default on K8S, otherwise workspaces fail to start. So, removing this field my affect k8s.

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 security section for now.

or keep containerBuildConfiguration as is:

devEnvironments:
  containerBuildConfiguration:
    openShiftSecurityContextConstraint: container-build
  security:
    containerSecurityContext:
      ...
    podSecurityContext:
     ...

@l0rd
Copy link
Contributor

l0rd commented Jul 17, 2023

I am ok with @tolusha proposal but I want to make sure we are on the same page:

  • Today the admin has the choice to enable or disable "container-build capabilities". We need to replace that with a way to specify fine-grained and arbitrary pods/containers security context and users SCC.
  • When the admin doesn't customize them in the CheCluster then the default security contexts and SCC should identical to today defaults.
  • The admin should be also able to specify in the CheCluster an "empty" securityContext for workspace pods/containers and no SCC.
  • Whatever customizaztion the admin does we should assume he knows what he is doing: we should not check if the SCC is compatible with the security contexts.
  • For existing installations: if disableContainerBuildCapabilities: true we should set "empty" securityContext and no SCC. If disableContainerBuildCapabilities: false we should apply the default securityContext and use the SCC specified (if any) or use the container-build SCC.
  • The SCC is OpenShift-only but pods/container security context should be set on kubernetes too.

@amisevsk
Copy link
Contributor

Re: removing fields in the CRD (e.g. removing .containerBuildConfiguration.openShiftSecurityContextConstraint) -- this will deceptively appear to not be a problem until OLM attempts to upgrade the operator.

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:

  • If all CheClusters on the cluster do not contain the removed field, the upgrade will succeed without issue. This is the case you will see in a quick/default test
  • If any CheCluster contains the removed field (e.g. they have set a different name for the SCC), the upgrade will fail as the CR would be invalid if the upgrade was allowed

@AObuchow AObuchow force-pushed the configure_security_context branch 4 times, most recently from bf955aa to 4e4e94f Compare July 27, 2023 17:45
@AObuchow AObuchow changed the title add pod & container security context Che Cluster Fields; mark disableContainerBuildCapabilities as deprecated add pod & container security context Che Cluster Fields Jul 27, 2023
@AObuchow
Copy link
Contributor Author

AObuchow commented Jul 27, 2023

I've updated the PR & the PR description:

  • The containerSecurityContext and podSecurityContext fields are placed under devEnvironments.security, but devEnvironments.containerBuildConfiguration and devEnvironments.disableContainerBuildCapabilities remain unchanged.
  • The workspace security context can be configured by setting devEnvironments.disableContainerBuildCapabilities: true and specifying the security context in devEnvironments.security.containerSecurityContext. This is a bit awkward, so I've made mention of it in the containerSecurityContext field's documentation.
  • The default container security context used is the one required for container builds. It can be unset by setting devEnvironments.disableContainerBuildCapabilities: true and an empty value for devEnvironments.security.containerSecurityContext.
  • The default container security context is now defined with the environment variable CHE_DEFAULT_SPEC_DEVENVIRONMENTS_CONTAINERSECURITYCONTEXT

I did not mark devEnvironments.disableContainerBuildCapabilities as deprecated in its documentation, as it is still required to be set to true in order for devEnvironments.security.containerSecurityContext to take effect. @l0rd should we still mark it as deprecated?

api/v2/checluster_types.go Outdated Show resolved Hide resolved
@tolusha
Copy link
Contributor

tolusha commented Aug 1, 2023

I did not mark devEnvironments.disableContainerBuildCapabilities as deprecated in its documentation.

I believe we shouldn't mark this field as deprecated.

Copy link
Contributor

@amisevsk amisevsk left a 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

@openshift-ci openshift-ci bot added lgtm and removed lgtm labels Aug 1, 2023
@l0rd
Copy link
Contributor

l0rd commented Aug 2, 2023

@l0rd should we still mark it as deprecated?

@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>
@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.9 :: copyIIBsToQuay/1690: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: sync-to-downstream_3.x/4000: SUCCESS

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;

@devstudio-release
Copy link

Build 3.9 :: operator-bundle_3.x/1746: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/4000 triggered

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1172: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1172: SUCCESS

3.9.0 CI

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.9 :: copyIIBsToQuay/1691: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: sync-to-downstream_3.x/4001: SUCCESS

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;

@devstudio-release
Copy link

Build 3.9 :: operator-bundle_3.x/1747: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/4001 triggered

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1173: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1173: SUCCESS

3.9.0 CI

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.9 :: copyIIBsToQuay/1693: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: sync-to-downstream_3.x/4004: SUCCESS

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;

@devstudio-release
Copy link

Build 3.9 :: operator-bundle_3.x/1749: SUCCESS

Upstream sync done; /DS_CI/sync-to-downstream_3.x/4004 triggered

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1176: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.9 :: dsc_3.x/1176: SUCCESS

3.9.0 CI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for configuring default container/pod SecurityContext in CheCluster CR
5 participants