Promote sysctl annotations to fields by ingvagabund · Pull Request #63717 · kubernetes/kubernetes

ingvagabund · 2018-05-11T15:35:05Z

What this PR does / why we need it:

Promoting experimental sysctl feature from annotations to API fields.

Special notes for your reviewer:

Following sysctl KEP: kubernetes/community#2093

Release note:

The Sysctls experimental feature has been promoted to beta (enabled by default via the `Sysctls` feature flag). PodSecurityPolicy and Pod objects now have fields for specifying and controlling sysctls. Alpha sysctl annotations will be ignored by 1.11+ kubelets. All alpha sysctl annotations in existing deployments must be converted to API fields to be effective.

TODO:

- Promote sysctl annotation in Pod spec
- Promote sysctl annotation in PodSecuritySpec spec
- Feature gate the sysctl
- Promote from alpha to beta
- docs PR - Promote sysctls to Beta website#8804

sttts · 2018-05-11T17:02:01Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

proto tags are missing

they got added automatically in the generation commit, but having them in this commit makes them easier to review

Moved from the generated code.

php-coder · 2018-05-11T18:17:23Z

staging/src/k8s.io/api/core/v1/types.go

It shouldn't be empty, right?

php-coder · 2018-05-11T18:18:40Z

staging/src/k8s.io/api/extensions/v1beta1/types.go

Make member name lowercase as it will appear in the api doc: s/Sysctls/sysctls/

If we don't support old annotations, perhaps, we don't have to support PSP in the extensions API Group?

What is the semantics of empty lists vs. nil in the annotations? Does empty list mean "no sysctls allowed at all" vs. nil "use the default set" ?

The original functionality is respected. So

If the list is empty -> No sysctls are allowed

If the list is nil -> Use the default sysctls patterns

Make member name lowercase as it will appear in the api doc: s/Sysctls/sysctls/

Done

The nil/empty distinction is lost when roundtripping via serialized protobuf, and makes for a fairly confusing API

The nil/empty distinction is lost

Don't we test this in roundtrip tests? We should.

If we change the API semantics (I don't feel strongly, I tend to agree that it is confusing), we should doc that. Do we want to?

php-coder · 2018-05-11T18:19:38Z

staging/src/k8s.io/api/policy/v1beta1/types.go

Make member name lowercase as it will appear in the api doc: s/Sysctls/sysctls/

ncdc · 2018-05-11T18:22:15Z

/uncc

ingvagabund · 2018-05-12T21:48:39Z

Expected
    <string>: kernel.shm_rmid_forced = 0
    
to contain substring
    <string>: kernel.shm_rmid_forced = 1

Hmm, for some reason the pod.securityContext.Sysctl list is not decoded correctly. The body before decoding in the apiserver:

k8s\x00\n\t\n\x02v1\x12\x03Pod\x12\xe8\x01\n=\n+sysctl-fff5b3b8-562c-11e8-8c1d-507b9deefa09\x12\x00\x1a\x00\"\x00*\x002\x008\x00B\x00z\x00\x12\x96\x01\x12R\n\x0etest-container\x12\abusybox\x1a\v/bin/sysctl\x1a\x16kernel.shm_rmid_forced*\x00B\x00j\x00r\x00\x80\x01\x00\x88\x01\x00\x90\x01\x00\xa2\x01\x00\x1a\x05Never2\x00B\x00J\x00R\x00X\x00`\x00h\x00r\x1f:\x1d\n\x16kernel.shm_rmid_forced\x12\x011\x18\x00\x82\x01\x00\x8a\x01\x00\x9a\x01\x00\xc2\x01\x00\x1a\x0e\n\x00\x1a\x00\"\x00*\x002\x00J\x00Z\x00\x1a\x00\"\x00

Once can notice the kernel.shm_rmid_forced string is contained twice. One corresponding to the command, the other one to the sysctl.

After decoding the Sysctls:[]core.Sysctl(nil)

EDIT: So the byte stream is properly unmarshalled into v1.Pod. Though, v1.Pod -> core.Pod drops the Sysctls :-|

EDIT2. Grrr, forgot to update Convert_core_PodSecurityContext_To_v1_PodSecurityContext and Convert_v1_PodSecurityContext_To_core_PodSecurityContext methods :-|

liggitt · 2018-05-12T21:53:34Z

pkg/apis/core/types.go

no json tags on internal types

liggitt · 2018-05-12T21:54:06Z

pkg/apis/policy/types.go

remove json tags from internal types

ingvagabund · 2018-06-05T18:26:34Z

@jberkus thanks

ingvagabund · 2018-06-05T19:34:28Z

forbidden means forbidden

The main loop validating the sysctls during admission rewritten into:

for i, sysctl := range sysctls {
	switch {
	case s.isForbidden(sysctl.Name):
		..."sysctl %q is not allowed", sysctl.Name...
	case s.isSafe(sysctl.Name):
		continue
	case s.isAllowedUnsafe(sysctl.Name):
		continue
	default:
		..."unsafe sysctl %q is not allowed", sysctl.Name...
	}
}

Now, all sysctls (no matter if thery safe or unsafe) are subject to check if they are forbidden. If a sysctl is forbidden => goodbye. Then the default is safe and is allowed unsafe check. Otherwise rejected.

if something is matched by the forbiddenSysctls list, it is forbidden

Comment above

to prevent confusion, validation will prevent putting something in the allowedUnsafeSysctls list that is covered by something in the forbidden list

Done. Both lists are checked if there is any sysctl (either plain or a pattern) that is covered in other one and visa verse.

evaluation will be done as described in Promote sysctl annotations to fields #63717 (comment) (matched by forbiddenSysctls forbids, internal "safe" list allows, matched by allowedUnsafeSysctls allows)

Done.

@sttts @liggitt PTAL

liggitt · 2018-06-05T20:22:55Z

pkg/apis/policy/validation/validation.go

forbiddenSysctlsFldPath.Index(i) and forbiddenSysctls[i] below should be using j index saved here, not i

Copy-paste thing, shame on me. Fixed

liggitt · 2018-06-05T20:24:14Z

pkg/apis/policy/validation/validation.go

validatePodSecurityPolicySysctls is called unconditionally... returning an error unconditionally here means disabling the feature gate would block write access to PSPs

Right. This was the place with if sysctl not set -> return nil was. Removing that caused this. I will put it back.

liggitt · 2018-06-05T20:27:21Z

pkg/apis/policy/validation/validation.go

field.Invalid, "if '*' is present, must not specify other sysctls"

liggitt · 2018-06-05T20:37:29Z

three validation comments, two blocking (the invalid index one and the one unconditionally failing if the feature gate is disabled)

lgtm once those are resolved

sttts · 2018-06-05T20:50:18Z

pkg/apis/policy/types.go

follow-up: we should maybe also mention that the kubelet has to whitelist them explicitly.

follow-up: mention that these must not be forbidden.

can be formulated in a neutral way, like: "It depends on the node to accept these unsafe sysctl as well".

I will mention it.

sttts · 2018-06-05T21:02:57Z

pkg/apis/core/validation/validation.go

I guess the decoder will give us non-nil on an empty list? Do we want to reject an empty list?

nil or empty list has the same semantics here. I will update it to check the len.

sttts · 2018-06-05T21:04:36Z

pkg/kubelet/kubelet.go

so in server side validation we reject every sysctl, in the kubelet we ignore them. Is this intentional?

It's not intentional. I will fix that to reject on the kubelet side.

Oh, this one is actually intentional.

liggitt · 2018-06-05T22:11:06Z

API, validation, and PSP changes LGTM
/approve

sttts · 2018-06-05T22:11:47Z

lgtm also from my side.

sjenning · 2018-06-05T22:15:25Z

/lgtm

k8s-ci-robot · 2018-06-05T22:15:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, ingvagabund, liggitt, sjenning

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [liggitt]
~~cmd/kubelet/OWNERS~~ [derekwaynecarr]
~~docs/api-reference/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/features/OWNERS~~ [derekwaynecarr,liggitt]
~~pkg/kubelet/OWNERS~~ [derekwaynecarr]
~~pkg/security/podsecuritypolicy/OWNERS~~ [liggitt]
~~plugin/pkg/admission/security/podsecuritypolicy/OWNERS~~ [derekwaynecarr,liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~test/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-06-05T22:16:23Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@derekwaynecarr @ingvagabund @liggitt @sjenning @tallclair @vishh

Pull Request Labels

sig/auth sig/node: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

k8s-ci-robot · 2018-06-05T22:26:30Z

New changes are detected. LGTM label has been removed.

ingvagabund · 2018-06-05T23:27:53Z

/test pull-kubernetes-e2e-gce

liggitt · 2018-06-06T01:55:10Z

/retest

liggitt · 2018-06-06T04:37:38Z

/retest

k8s-github-robot · 2018-06-06T06:23:10Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-06T07:47:35Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from brendandburns, foxish, liggitt, luxas, madhusudancs, mwielgus, ncdc, php-coder, pweil-, pwittrock, rootfs, soltysh, tallclair and yujuhong May 11, 2018 15:35

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 11, 2018

ingvagabund mentioned this pull request May 11, 2018

Add sysctl support kubernetes/enhancements#34

Closed

20 tasks

ingvagabund requested a review from sttts May 11, 2018 16:13

sttts reviewed May 11, 2018

View reviewed changes

php-coder reviewed May 11, 2018

View reviewed changes

k8s-ci-robot removed the request for review from ncdc May 11, 2018 18:22

liggitt reviewed May 12, 2018

View reviewed changes

liggitt reviewed Jun 5, 2018

View reviewed changes

sttts reviewed Jun 5, 2018

View reviewed changes

Promote sysctl annotations to API fields

ab616a8

Run make update

3cc1536

sjenning and others added 2 commits June 6, 2018 00:23

sysctls: create feature gate to track promotion

6729add

Switch to Beta

b1b28f0

AishSundar mentioned this pull request Jun 6, 2018

[Failing Test] : [k8s.io] Sysctls tests failing release-upgrade test suite #64845

Closed

This was referenced Jun 18, 2018

Make kubectl describe psp describe the new sysctl field #65181

Closed

SCC: add {AllowedUnsafe,Forbidden}Sysctls openshift/api#60

Merged

rdrgmnzs mentioned this pull request Dec 7, 2018

ExperimentalAllowedUnsafeSysctls has moved to AllowedUnsafeSysctls in k8s 1.11 kubernetes/kops#6179

Merged

liggitt mentioned this pull request Feb 24, 2021

Add field-level warnings for deprecated / known bad values #94626

Open

20 tasks

SataQiu mentioned this pull request Sep 30, 2022

kubelet: remove the unused constant AnnotationInvalidReason since sysctl annotations are deprecated and migrated to fields #112796

Merged

Conversation

ingvagabund commented May 11, 2018 • edited by liggitt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncdc commented May 11, 2018

Uh oh!

ingvagabund commented May 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ingvagabund commented Jun 5, 2018

Uh oh!

ingvagabund commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Jun 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ingvagabund commented May 11, 2018 •

edited by liggitt

Loading

ingvagabund May 14, 2018 •

edited

Loading

ingvagabund commented May 12, 2018 •

edited

Loading