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 ability to allow privileged containers via a manifest flag. #620

Merged
merged 5 commits into from Sep 6, 2019

Conversation

@r0mant
Copy link
Contributor

commented Sep 4, 2019

This PR adds an additional boolean manifest field systemOptions.allowPrivileged that enables privileged containers in the cluster. Requires gravitational/planet#476. Closes #619.

@r0mant r0mant requested review from a-palchikov and knisbet Sep 4, 2019

@knisbet
Copy link
Contributor

left a comment

docs updates for using the privileged flag?

@r0mant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@knisbet Yeah, I was gonna do them in a separate PR but I guess here would be fine too.

@r0mant

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@knisbet I've added the docs too.

@@ -4,12 +4,12 @@ Every Cluster created from a Gravity Cluster Image is a fully featured
Kubernetes environment. It contains the following components:

1. All user applications that have been packaged into the Cluster Image.
2. All Kubernetes daemons like `kube-scheduler`, `kube-apiserver`, and others.

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 5, 2019

Author Contributor

That's just my text editor auto-truncating trailing whitespaces. There's a bunch of these here.

This comment has been minimized.

Copy link
@benarent

benarent Sep 5, 2019

Contributor

😡

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 5, 2019

Author Contributor

@benarent FWIW I think everyone should configure their editors to truncate trailing whitespaces :)

@a-palchikov
Copy link
Contributor

left a comment

lgtm as long as updates work with the new flag turned on.

@@ -644,6 +644,7 @@ const manifestSchema = `
"additionalProperties": false,
"properties": {
"baseImage": {"type": "string"},
"allowPrivileged": {"type": "boolean"},

This comment has been minimized.

Copy link
@a-palchikov

a-palchikov Sep 5, 2019

Contributor

Did you try updating with this flag on? I remember additionalProperties: false being the culprit for update failures as the existing cluster will probably fail to validate the new application's manifest.

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 6, 2019

Author Contributor

@a-palchikov Good call, yeah - it didn't work obviously due to this schema issue.

I took a stab at fixing it, check out my latest commit: 561f1b3. With these changes the upgrade works (I tried 6.0 to this new version with privileged flag enabled).

@benarent
Copy link
Contributor

left a comment

I've left a few comments, do we also wan to address Privilege Escalation: with this? or if this is set, is it always false.

@@ -4,12 +4,12 @@ Every Cluster created from a Gravity Cluster Image is a fully featured
Kubernetes environment. It contains the following components:

1. All user applications that have been packaged into the Cluster Image.
2. All Kubernetes daemons like `kube-scheduler`, `kube-apiserver`, and others.

This comment has been minimized.

Copy link
@benarent

benarent Sep 5, 2019

Contributor

😡

@@ -465,13 +465,16 @@ systemOptions:
args: ["--system-reserved=memory=500Mi"]
hairpinMode: "promiscuous-bridge"
# When set to true, allows running privileged containers in the deployed
# clusters, defaults to false
allowPrivileged: true

This comment has been minimized.

Copy link
@benarent

benarent Sep 5, 2019

Contributor

It's hard to see the context here, but do we want to set this true? If it's a default example?

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 5, 2019

Author Contributor

Well, the purpose of this particular sample manifest is to show how to enable everything you can possibly enable. So this is not technically "default" manifest you'd want to use. I'm okay with setting this to false here though.

This comment has been minimized.

Copy link
@knisbet

knisbet Sep 6, 2019

Contributor

I think if you look at the way we do other samples, such as extensions.logs.disabled we show the default value (false). So it would seem to match to have allowPrivileged show the default value as well.

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 6, 2019

Author Contributor

Yeah, I'll change it to false.

@@ -579,6 +585,9 @@ type AppConfig struct {
// Packages allow to override default env.Packages when creating
// an app service
Packages pack.PackageService
// Backend allows to override default env.Backend when creating

This comment has been minimized.

Copy link
@a-palchikov

a-palchikov Sep 6, 2019

Contributor

... allows to override default backend when creating the service

// The local service is needed to handle cases such as newly introduced
// manifest field which gravity-site which may be running the old code
// does not recognize.
apps, err := env.AppServiceLocal(localenv.AppConfig{

This comment has been minimized.

Copy link
@a-palchikov

a-palchikov Sep 6, 2019

Contributor

I'm now wondering why this is not part of the cluster environment - why not have ClusterApps (similar to ClusterPackages that uses the cluster application service)?

This comment has been minimized.

Copy link
@r0mant

r0mant Sep 6, 2019

Author Contributor

Well... this app service is kind of a weird beast cause it uses cluster's packages/backend, but by itself is local... I guess I could put it in there anyway.

@r0mant r0mant merged commit e689aca into master Sep 6, 2019

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@r0mant r0mant deleted the roman/privileged branch Sep 6, 2019

@knisbet
knisbet approved these changes Sep 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.