Promote sysctl annotations to fields #63717

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

+	// Sysctls is a white list of allowed sysctls in a pod spec. Each entry
+	// is either a plain sysctl name or ends in "*" in which case it is considered
+	// as a prefix of allowed sysctls.
+	Sysctls []string `json:"sysctls,omitempty"`


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

@@ -5001,6 +5005,8 @@ type Sysctl struct {
 	Name string `protobuf:"bytes,1,opt,name=name"`
 	// Value of a property to set
 	Value string `protobuf:"bytes,2,opt,name=value"`
+	//


It shouldn't be empty, right?

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

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

@@ -946,6 +946,10 @@ type PodSecurityPolicySpec struct {
 	// is allowed in the "volumes" field.
 	// +optional
 	AllowedFlexVolumes []AllowedFlexVolume `json:"allowedFlexVolumes,omitempty" protobuf:"bytes,18,rep,name=allowedFlexVolumes"`
+	// Sysctls is a white list of allowed sysctls in a pod spec. Each entry


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

@@ -201,6 +201,10 @@ type PodSecurityPolicySpec struct {
 	// is allowed in the "volumes" field.
 	// +optional
 	AllowedFlexVolumes []AllowedFlexVolume `json:"allowedFlexVolumes,omitempty" protobuf:"bytes,18,rep,name=allowedFlexVolumes"`
+	// Sysctls is a white list of allowed sysctls in a pod spec. Each entry


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

+	// Sysctls hold a list of namespaced sysctls used for the pod. Pods with unsupported
+	// sysctls (by the container runtime) might fail to launch.
+	// +optional
+	Sysctls []Sysctl `json:"sysctls,omitempty"`


no json tags on internal types

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

pkg/apis/policy/types.go

+	// Sysctls is a white list of allowed sysctls in a pod spec. Each entry
+	// is either a plain sysctl name or ends in "*" in which case it is considered
+	// as a prefix of allowed sysctls.
+	Sysctls []string `json:"sysctls,omitempty"`


remove json tags from internal types

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

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

Run make update

3cc1536

k8s-ci-robot assigned sjenning Jun 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

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

sysctls: create feature gate to track promotion

6729add

Switch to Beta

b1b28f0

ingvagabund force-pushed the promote-sysctl-annotations-to-fields branch from ad243bb to b1b28f0 Compare June 5, 2018 22:26

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

ingvagabund added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2018

liggitt added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 5, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Jun 5, 2018

k8s-github-robot merged commit b6f75ac into kubernetes:master Jun 6, 2018

ingvagabund deleted the promote-sysctl-annotations-to-fields branch June 6, 2018 08:42

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promote sysctl annotations to fields #63717

Promote sysctl annotations to fields #63717

ingvagabund commented May 11, 2018 •

edited by liggitt

sttts May 11, 2018

liggitt May 12, 2018

ingvagabund May 14, 2018

php-coder May 11, 2018

ingvagabund May 14, 2018

php-coder May 11, 2018

php-coder May 11, 2018

sttts May 14, 2018

ingvagabund May 14, 2018

ingvagabund May 14, 2018 •

edited

liggitt May 14, 2018

sttts May 15, 2018

php-coder May 11, 2018

ingvagabund May 14, 2018

ncdc commented May 11, 2018

ingvagabund commented May 12, 2018 •

edited

liggitt May 12, 2018

ingvagabund May 14, 2018

liggitt May 12, 2018

ingvagabund May 14, 2018

sttts commented Jun 5, 2018

sjenning commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

ingvagabund commented Jun 5, 2018

liggitt commented Jun 6, 2018

liggitt commented Jun 6, 2018

k8s-github-robot commented Jun 6, 2018

k8s-github-robot commented Jun 6, 2018

Promote sysctl annotations to fields #63717

Promote sysctl annotations to fields #63717

Conversation

ingvagabund commented May 11, 2018 • edited by liggitt

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingvagabund May 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented May 11, 2018

ingvagabund commented May 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jun 5, 2018

sjenning commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-ci-robot commented Jun 5, 2018

ingvagabund commented Jun 5, 2018

liggitt commented Jun 6, 2018

liggitt commented Jun 6, 2018

k8s-github-robot commented Jun 6, 2018

k8s-github-robot commented Jun 6, 2018

ingvagabund commented May 11, 2018 •

edited by liggitt

ingvagabund May 14, 2018 •

edited

ingvagabund commented May 12, 2018 •

edited