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 autodetect option to security context configuration #5150

Merged
merged 11 commits into from
Jan 11, 2022

Conversation

naemono
Copy link
Contributor

@naemono naemono commented Dec 9, 2021

resolves #5061

This change adds an option to the set-default-security-context flag to help the user with settings that pertain to security context. It adds the auto-detect option (which is triggered by simply not setting the set-default-security-context flag) which does the following when set:

  1. If the set-default-security-context flag is explicitly set to either true, or false, use this value.
  2. if the set-default-security-context flag is set to auto-detect (the default value in helm charts) use openshift detection to determine whether or not we are running within an openshift cluster. If we determine we are on an openshift cluster then default to setting this flag to false to allow openshift to set the security context properly, otherwise set this flag to true to allow the operator to set the security context appropriately.

Any other set value will throw a syntax error.

testing

This was tested in both an openshift cluster, and vanilla cluster with the auto-detect option set and the results were as follows

vanilla cluster

Elasticsearch stateful set had the following settings

spec.securityContext:
          fsGroup: 1000

openshift cluster

container.securityContext:
      capabilities:
        drop:
        - KILL
        - MKNOD
        - SETGID
        - SETUID
      runAsUser: 1000630000

spec.securityContext:
    fsGroup: 1000630000
    seLinuxOptions:
      level: s0:c25,c15

Update helm values with more documentation around setting default security context
@botelastic botelastic bot added the triage label Dec 9, 2021
@thbkrkr thbkrkr added >enhancement Enhancement of existing functionality v2.0.0 labels Dec 10, 2021
@botelastic botelastic bot removed the triage label Dec 10, 2021
cmd/manager/main.go Outdated Show resolved Hide resolved
deploy/eck-operator/values.yaml Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

Almost LGTM, a few nits and the build is failing due to the import statements in the new test.

cmd/manager/main.go Outdated Show resolved Hide resolved
return true, nil
}
}
// if the security.openshift.io group isn't found, we are not in openshift
Copy link
Collaborator

Choose a reason for hiding this comment

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

as above let's stick with the official camel casing

cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
cmd/manager/main_test.go Outdated Show resolved Hide resolved
cmd/manager/main_test.go Show resolved Hide resolved
cmd/manager/main_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM

@naemono naemono merged commit f2fcf2e into elastic:main Jan 11, 2022
naemono added a commit to naemono/cloud-on-k8s that referenced this pull request Jan 13, 2022
* Add auto-detect option to setting security context
Update helm values with more documentation around setting default security context

* Refactor because of linter issues

* return invalid security context errors.

Co-authored-by: Michael Morello <michael.morello@gmail.com>

* simplify logic when detecting openshift

Co-authored-by: Michael Morello <michael.morello@gmail.com>

* Move openshift detection logic into it's own function
remove erroneous logging line

* Update to only allow 3 options for setting security context: true, false, auto-detect.

* catch api error "not found", and assume not on openshift cluster.

* Add unit tests around checking how we set the default security context in/out of openshift.

* Update comments per PR review

* review changes

* Move resources required for test to be directly after the test that uses them.

Co-authored-by: Michael Morello <michael.morello@gmail.com>
fantapsody pushed a commit to fantapsody/cloud-on-k8s that referenced this pull request Feb 7, 2023
* Add auto-detect option to setting security context
Update helm values with more documentation around setting default security context

* Refactor because of linter issues

* return invalid security context errors.

Co-authored-by: Michael Morello <michael.morello@gmail.com>

* simplify logic when detecting openshift

Co-authored-by: Michael Morello <michael.morello@gmail.com>

* Move openshift detection logic into it's own function
remove erroneous logging line

* Update to only allow 3 options for setting security context: true, false, auto-detect.

* catch api error "not found", and assume not on openshift cluster.

* Add unit tests around checking how we set the default security context in/out of openshift.

* Update comments per PR review

* review changes

* Move resources required for test to be directly after the test that uses them.

Co-authored-by: Michael Morello <michael.morello@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality v2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add autodetect option to set-default-security-context flag
4 participants