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 set-default-security-context flag to handle runAs user in ES 8.0+ #3342

Merged
merged 7 commits into from
Aug 31, 2020

Conversation

david-kow
Copy link
Contributor

@david-kow david-kow commented Jun 29, 2020

Implements @sebgl proposal from #2791 (comment). We should make a final call whether we want to only do it for 8.0+ and I can add that.

Also, for Kibana to work we also need a separate fix for 8.0 default resources issue - Kibana OOMs with 1Gi.

@david-kow david-kow force-pushed the set_default_fsgroup branch from 0fca32f to 62205f1 Compare July 28, 2020 14:18
@david-kow david-kow force-pushed the set_default_fsgroup branch from 62205f1 to 29d37d1 Compare July 28, 2020 14:22
@david-kow david-kow marked this pull request as ready for review July 28, 2020 15:07
cmd/manager/main.go Outdated Show resolved Hide resolved
pkg/controller/common/operator/parameters.go Outdated Show resolved Hide resolved
pkg/controller/common/defaults/pod_template.go Outdated Show resolved Hide resolved
@thbkrkr thbkrkr added the v1.3.0 label Aug 3, 2020
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.

This looks good. But I realise that we will have a problem with this flag for our OLM version of ECK because users cannot the flip this switch and OLM applies to both OpenShift and vanilla k8s environments, so no good default here. I am OK tackling that problem separately though, as long as we don't forget. [Update] TIL there seems to be a way to customize operators deployed via OLM through the subscription CRD https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/subscription-config.md

cmd/manager/main.go Outdated Show resolved Hide resolved
@anyasabo
Copy link
Contributor

anyasabo commented Aug 5, 2020

TIL there seems to be a way to customize operators deployed via OLM through the subscription CRD https://github.com/operator-framework/operator-lifecycle-manager/blob/master/doc/design/subscription-config.md

This seems like a good argument to move to using env vars by default rather than cli flags (which take precedence): https://github.com/elastic/cloud-on-k8s/blob/master/hack/manifest-gen/assets/charts/eck/templates/statefulset.yaml#L58

@barkbay
Copy link
Contributor

barkbay commented Aug 6, 2020

TIL there seems to be a way to customize operators deployed via OLM through the subscription CRD

This can't be done through the web ui though. Even if primary interest of OLM is no lost (managing the deployment and the lifecycle of the operator) it will be hard for users using the Openshift console to figure out what is happening.

@pebrc
Copy link
Collaborator

pebrc commented Aug 6, 2020

This can't be done through the web ui though.

Technically it can be done even in the OpenShift console if you count editing raw-YAML as "through the web ui"

Screenshot 2020-08-06 at 09 57 41

@barkbay
Copy link
Contributor

barkbay commented Aug 6, 2020

Technically it can be done even in the OpenShift console if you count editing raw-YAML as "through the web ui"

Agreed, but my point was more about the fact that it can't be done when the user clicks on the "Install" button. I think that most of the users are expecting an up and running operator when doing that, editing the raw content of the Subscription in a second step sounds like a broken process to me.

@david-kow david-kow changed the title Add set-default-fsgroup flag to the operator Add set-default-security-context flag to the operator Aug 25, 2020
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!

@david-kow david-kow requested a review from sebgl August 26, 2020 05:41
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

I'm good with the approach 👍

I would expect we also change the init container script to get rid of this block starting 8.0:

######################
# Volumes chown #
######################
# chown the data and logs volume to the elasticsearch user
# only done when running as root, other cases should be handled
# with a proper security context
chown_start=$(date +%s)
if [[ $EUID -eq 0 ]]; then
{{range .ChownToElasticsearch}}
echo "chowning {{.}} to elasticsearch:elasticsearch"
chown -v elasticsearch:elasticsearch {{.}}
{{end}}
fi
echo "chown duration: $(duration $chown_start) sec."

Other than that we probably need some E2E tests tweak for ES 8.0 on Openshift, and the right documentation for this new behaviour, but that can be done as part of different PRs.

pkg/controller/common/version/version.go Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

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

@sebgl sebgl left a comment

Choose a reason for hiding this comment

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

LGTM

@david-kow david-kow merged commit 9a4bede into elastic:master Aug 31, 2020
@david-kow david-kow deleted the set_default_fsgroup branch August 31, 2020 14:15
@david-kow david-kow changed the title Add set-default-security-context flag to the operator Add set-default-security-context flag to handle runAs user in ES 8.0+ Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>breaking >bug Something isn't working v1.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants