Add default SecurityContext to every new ksvc #1821
Conversation
There was a problem hiding this comment.
@dsimansk: 2 warnings.
In response to this:
Description
This's handling a warning from API-server validation about SecurityContext being empty for
kn service createand related cmds.I'll look into a few further improvements. E.g. either adding SecurityContext builder pattern or at least plain
--security-context seccon.yamlflag to make it customizable.Changes
- Add default SecurityContext to every new ksvc
Reference
Fixes #
Release Note
Add default SecurityContext to every new ksvc
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.
pkg/kn/flags/podspec_helper.go
Outdated
| @@ -378,6 +380,26 @@ func UpdateImagePullPolicy(spec *corev1.PodSpec, imagePullPolicy string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func UpdateDefaultSecurityContext(spec *corev1.PodSpec) { | |||
There was a problem hiding this comment.
Golint comments: exported function UpdateDefaultSecurityContext should have comment or be unexported. More info.
pkg/kn/flags/podspec_helper.go
Outdated
| } | ||
| } | ||
|
|
||
| func DefaultSecCon() *corev1.SecurityContext { |
There was a problem hiding this comment.
Golint comments: exported function DefaultSecCon should have comment or be unexported. More info.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1821 +/- ##
==========================================
+ Coverage 79.70% 79.74% +0.03%
==========================================
Files 179 179
Lines 13850 13891 +41
==========================================
+ Hits 11039 11077 +38
- Misses 2050 2052 +2
- Partials 761 762 +1
☔ View full report in Codecov by Sentry. |
| func DefaultSecCon() *corev1.SecurityContext { | ||
| return &corev1.SecurityContext{ | ||
| AllowPrivilegeEscalation: pointer.Bool(false), | ||
| RunAsNonRoot: pointer.Bool(true), |
There was a problem hiding this comment.
My question would be if this does not break the existing setup and whether this is not too restrictive. I mean, there are still tons of container images that can only run as root and/or need certain capabilities. Having such a strict default (that currently can't be changed), is quite drastic. I think we should only add this if there is at least an option to get back the previous behaviour, i.e. having no SecurityContext at all. This then could be documented as a change (which is still not backwards compatible). This option could be called --no-security-context or maybe even better something like --security-context=none (with the alternatives like --security-context=strict (the default) and --security-context=privilegeEscalation:allow,runAsNonRoot:true,dropCapabilities:NET for fine grained security context configuration (this we can add later but strict and none should be already along with this PR.
There was a problem hiding this comment.
Yep, I was actually thinking about --no-security-context to be added immediately. I like strict & none combination, with the further enhancement to towards fine grained options.
There was a problem hiding this comment.
That's at least ready with on/off switch. 0fcf800
|
Please see my comment https://github.com/knative/client/pull/1821/files#r1242163943 that we should already add basic option handling here. Otherwise, I think it would break too much. Also note the linter warnings. |
|
/retest |
| @@ -234,6 +236,10 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string { | |||
| flagNames = append(flagNames, "pull-secret") | |||
| flagset.Int64VarP(&p.User, "user", "", 0, "The user ID to run the container (e.g., 1001).") | |||
| flagNames = append(flagNames, "user") | |||
|
|
|||
| flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Security Context definition to be added the service. Accepted values: strict | none.") | |||
There was a problem hiding this comment.
| flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Security Context definition to be added the service. Accepted values: strict | none.") | |
| flagset.StringVar(&p.SecurityContext, "security-context", "strict", "Predefined security context for the service. Accepted values: 'none' for no security context and 'strict' for dropping all capabilities, running as non-root, and no privilege escalation.") |
| // Add or update Security Context to default strict | ||
| container.SecurityContext = DefaultStrictSecCon() | ||
| case "": | ||
| // Add default strict SC flag is not used, hence empty value |
There was a problem hiding this comment.
Can this be the case if a default value is set to "strict" ?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, rhuss The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description
This's handling a warning from API-server validation about SecurityContext being empty for
kn service createand related cmds.I'll look into a few further improvements. E.g. either adding SecurityContext builder pattern or at least plain
--security-context seccon.yamlflag to make it customizable./cc @rhuss @vyasgun
Changes
Reference
Fixes #
Release Note