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

Promote sysctl annotations to fields #63717

Merged
merged 4 commits into from Jun 6, 2018

Conversation

@ingvagabund
Copy link
Contributor

ingvagabund commented May 11, 2018

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 - kubernetes/website#8804
// 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"`

This comment has been minimized.

@sttts

sttts May 11, 2018

Contributor

proto tags are missing

This comment has been minimized.

@liggitt

liggitt May 12, 2018

Member

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

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

Moved from the generated code.

@@ -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"`
//

This comment has been minimized.

@php-coder

php-coder May 11, 2018

Contributor

It shouldn't be empty, right?

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

Updated

@@ -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

This comment has been minimized.

@php-coder

php-coder May 11, 2018

Contributor

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

This comment has been minimized.

@php-coder

php-coder May 11, 2018

Contributor

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

This comment has been minimized.

@sttts

sttts May 14, 2018

Contributor

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" ?

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

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

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

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

Done

This comment has been minimized.

@liggitt

liggitt May 14, 2018

Member

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

This comment has been minimized.

@sttts

sttts May 15, 2018

Contributor

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?

@@ -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

This comment has been minimized.

@php-coder

php-coder May 11, 2018

Contributor

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

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

Done

@ncdc

This comment has been minimized.

Copy link
Member

ncdc commented May 11, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from ncdc May 11, 2018

@ingvagabund

This comment has been minimized.

Copy link
Contributor Author

ingvagabund commented May 12, 2018

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 :-|

// 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"`

This comment has been minimized.

@liggitt

liggitt May 12, 2018

Member

no json tags on internal types

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

Removed

// 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"`

This comment has been minimized.

@liggitt

liggitt May 12, 2018

Member

remove json tags from internal types

This comment has been minimized.

@ingvagabund

ingvagabund May 14, 2018

Author Contributor

Removed

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jun 5, 2018

lgtm also from my side.

@sjenning

This comment has been minimized.

Copy link
Contributor

sjenning commented Jun 5, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 5, 2018

[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:

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

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 5, 2018

[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

sjenning and others added some commits Mar 21, 2018

@ingvagabund ingvagabund force-pushed the ingvagabund:promote-sysctl-annotations-to-fields branch from ad243bb to b1b28f0 Jun 5, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 5, 2018

New changes are detected. LGTM label has been removed.

@ingvagabund

This comment has been minimized.

Copy link
Contributor Author

ingvagabund commented Jun 5, 2018

/test pull-kubernetes-e2e-gce

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 6, 2018

/retest

1 similar comment
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 6, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 6, 2018

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 6, 2018

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

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

17 of 18 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation ingvagabund authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@ingvagabund ingvagabund deleted the ingvagabund:promote-sysctl-annotations-to-fields branch Jun 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.