CAP_NET_RAW should be NET_RAW by MHBauer · Pull Request #95613 · kubernetes/kubernetes

MHBauer · 2020-10-15T17:19:05Z

Format of Capability adds CAP_ prefix automatically.
Add commentary as test progress output.
Clear up some existing comments.

What type of PR is this?
/kind bug
/kind failing-test

What this PR does / why we need it:
Test currently fails 100% of the time with Error: failed to create containerd task: OCI runtime create failed: container_linux.go:370: starting container process caused: unknown capability "CAP_CAP_NET_RAW": unknown

Which issue(s) this PR fixes:
Fixes #95612

Special notes for your reviewer:
I have no idea where this behavior is documented, I'm reversing my understanding of the test output.

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Format of Capability adds CAP_ prefix automatically. Add commentary as test progress output. Clear up some existing comments.

MHBauer · 2020-10-15T17:20:08Z

/sig node

MHBauer · 2020-10-15T17:20:34Z

Creator of code segment
/cc @harche

k8s-ci-robot · 2020-10-15T17:20:35Z

@MHBauer: GitHub didn't allow me to request PR reviews from the following users: harche.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

Creator of code segment
/cc @harche

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.

MHBauer · 2020-10-15T17:57:10Z

Triage link https://storage.googleapis.com/k8s-gubernator/triage/index.html?test=when%20querying%20%2Fstats%2Fsummary#fd0d2dc5e869a43662b9

harche · 2020-10-15T19:06:59Z

It had worked fine on CRIO, but I can test it again on Monday.

MHBauer · 2020-10-15T21:47:53Z

Alright, so, the documentation for kube seems to inherit the behavior of docker.

https://kubernetes.io/docs/concepts/policy/pod-security-policy/#capabilities
The following fields take a list of capabilities, specified as the capability name in ALL_CAPS without the CAP_ prefix.

Right above this link
https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#set-the-seccomp-profile-for-a-container
Note: Linux capability constants have the form CAP_XXX. But when you list capabilities in your Container manifest, you must omit the CAP_ portion of the constant. For example, to add CAP_SYS_TIME, include SYS_TIME in your list of capabilities.

Docker, where the first support comes from
https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities

MHBauer · 2020-10-15T21:50:31Z

 						SecurityContext: &v1.SecurityContext{
 							Capabilities: &v1.Capabilities{
-								Add: []v1.Capability{"CAP_NET_RAW"},
+								Add: []v1.Capability{"NET_RAW"},


This is the main change.

MHBauer · 2020-10-15T21:57:58Z

/cc @dims
maybe interested?

mrunalp · 2020-10-15T22:01:29Z

@MHBauer This looks good to me (CRI-O maintainer).

mrunalp · 2020-10-15T22:02:10Z

/lgtm

MHBauer · 2020-10-15T22:07:17Z

As part of a NodeConformance test, I'm concerned about the original change assuming our defaults are lacking CAP_NET_DEFAULT.

Reading deeper into the link:
DefaultAddCapabilities - The capabilities which are added to containers by default, in addition to the runtime defaults. See the Docker documentation for the default list of capabilities when using the Docker runtime.
https://docs.docker.com/engine/reference/run/#runtime-privilege-and-linux-capabilities
By default, Docker has a default list of capabilities that are kept. The following table lists the Linux capability options which are allowed by default and can be dropped.
In that table is : NET_RAW | Use RAW and PACKET sockets.

~~I think we need to roll back #95321~~

MHBauer · 2020-10-20T13:58:37Z

Reversing my feelings on this, there may be a bunch of tests where we're assuming docker permissions, and it would be good to have them all explicitly set as requirements for the test to run.

mikebrow · 2020-10-20T14:22:22Z

Reversing my feelings on this, there may be a bunch of tests where we're assuming docker permissions, and it would be good to have them all explicitly set as requirements for the test to run.

^ depends what you are testing the defaults, or that you can run tests in spite of default deltas... The problem is if you fix the test cases, customers will break if they don't change their specs like we did for our tests.

Suggestions: 1) test if a container runs with all necessary caps explicitly set 2) test that a container runs as it did in prior releases using defaults (e.g. on docker shim when migrating to CRI runtimes) 3) test that a container fails to run as it failed in prior releases without additional caps set due to docker default 4) and if we are going to change the k8s expected defaults, or accept different defaults for particular runtimes, document and test the differences

MHBauer · 2020-10-20T17:18:39Z

I'll make some long term issues to follow up on setting Capabilities in tests and updating the documentation beyond "when using docker, we use docker defaults." Seems implied that "when using a specific runtime we use the specific runtime defaults". Other things is obviously that the defaults can be configured for each runtime on each node, so consistency is probably needed on a cluster basis. It makes me wonder if there is already some explicit setting of Capabilities in kubelet and whether that can be configured to ensure that Capabilities are set consistently over a cluster without needing to explicitly set them on each node.

Calling some other containerd people from https://github.com/containerd/project/blob/master/MAINTAINERS
/cc @AkihiroSuda @crosbymichael @estesp @dchen1107

mikebrow · 2020-11-18T18:19:57Z

@dims still broke here.. should be an easy merge to bring us back on-line

dims · 2020-11-19T16:51:50Z

/assign @sjenning @mrunalp

Seth, Mrunal,
One line change essentially. Can you please approve?

dims · 2020-11-19T16:52:46Z

/triage accepted
/priority critical-urgent

mrunalp · 2020-11-19T16:55:28Z

/approve

k8s-ci-robot · 2020-11-19T16:55:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MHBauer, mrunalp

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:

~~test/e2e_node/OWNERS~~ [mrunalp]

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

harche · 2020-11-19T16:59:02Z

/test pull-kubernetes-node-crio-e2e

harche · 2020-11-19T17:01:05Z

Please let this pull-kubernetes-node-crio-e2e pass, just to make sure we aren't breaking anything on CRIO.

harche · 2020-11-19T17:16:41Z

/test pull-kubernetes-node-crio-e2e

harche · 2020-11-19T17:33:22Z

pull-kubernetes-node-crio-e2e recently started passing on PRs, https://prow.k8s.io/job-history/kubernetes-jenkins/pr-logs/directory/pull-kubernetes-node-crio-e2e?buildId=

But here, it's failing even before starting any tests.

harche · 2020-11-19T17:34:10Z

I just started that test job on this PR, #96716, let's see how does that go.

harche · 2020-11-19T18:13:55Z

@MHBauer after checking out your PR locally and rebasing to master, the job is passing fine.

make test-e2e-node REMOTE=true RUNTIME=remote IMAGE_CONFIG_FILE="/home/harshal/Downloads/image-config-cgrpv1.yaml" INSTANCE_PREFIX="harpatil" CLEANUP=false CONTAINER_RUNTIME_ENDPOINT="unix:///var/run/crio/crio.sock" FOCUS="\[NodeConformance\]|\[NodeFeature:.+\]" SKIP="\[Flaky\]|\[Slow\]|\[Serial\]" TEST_ARGS='--kubelet-flags="--cgroup-driver=systemd --cgroups-per-qos=true --cgroup-root=/ --runtime-cgroups=/system.slice/crio.service --non-masquerade-cidr=0.0.0.0/0" --extra-log="{\"name\": \"crio.log\", \"journalctl\": [\"-u\", \"crio\"]}"'

Ran 196 of 336 Specs in 727.346 seconds
SUCCESS! -- 196 Passed | 0 Failed | 0 Pending | 140 Skipped

I am guessing rebasing will help it pass here too.

harche · 2020-11-19T18:14:11Z

I just started that test job on this PR, #96716, let's see how does that go.

It passed there.

fejta-bot · 2020-11-19T20:30:23Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2020-11-19T22:57:24Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

MHBauer · 2020-12-04T17:59:49Z

Can I put it into the milestone or do I need a release manager?

/milestone v1.20

k8s-ci-robot · 2020-12-04T17:59:51Z

@MHBauer: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

Details

In response to this:

Can I put it into the milestone or do I need a release manager?

/milestone v1.20

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.

MHBauer · 2020-12-04T18:00:11Z

Phew, good.

mikebrow · 2020-12-09T16:11:06Z

need this revert pr on 1.20 as well can we get a cherry pick?

CAP_NET_RAW should be NET_RAW

1ac20e9

Format of Capability adds CAP_ prefix automatically. Add commentary as test progress output. Clear up some existing comments.

k8s-ci-robot requested review from bart0sh and odinuge October 15, 2020 17:19

k8s-ci-robot added area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 15, 2020

MHBauer mentioned this pull request Oct 15, 2020

update README to clarify the current status of this repo containerd/cri#1597

Merged

MHBauer commented Oct 15, 2020

View reviewed changes

k8s-ci-robot requested a review from dims October 15, 2020 21:58

k8s-ci-robot assigned mrunalp Oct 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2020

BenTheElder mentioned this pull request Oct 19, 2020

Capability set incorrectly. CAP_NET_RAW should be NET_RAW. #95612

Closed

k8s-ci-robot assigned sjenning Nov 19, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 19, 2020

k8s-ci-robot merged commit 5cdc3e6 into kubernetes:master Dec 9, 2020

k8s-ci-robot added this to the v1.21 milestone Dec 9, 2020

Conversation

MHBauer commented Oct 15, 2020

Uh oh!

MHBauer commented Oct 15, 2020

Uh oh!

MHBauer commented Oct 15, 2020

Uh oh!

k8s-ci-robot commented Oct 15, 2020

Uh oh!

MHBauer commented Oct 15, 2020

Uh oh!

harche commented Oct 15, 2020

Uh oh!

MHBauer commented Oct 15, 2020

Uh oh!

MHBauer Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

MHBauer commented Oct 15, 2020

Uh oh!

mrunalp commented Oct 15, 2020

Uh oh!

mrunalp commented Oct 15, 2020

Uh oh!

MHBauer commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MHBauer commented Oct 20, 2020

Uh oh!

mikebrow commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MHBauer commented Oct 20, 2020

Uh oh!

mikebrow commented Nov 18, 2020

Uh oh!

dims commented Nov 19, 2020

Uh oh!

dims commented Nov 19, 2020

Uh oh!

mrunalp commented Nov 19, 2020

Uh oh!

k8s-ci-robot commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

harche commented Nov 19, 2020

Uh oh!

fejta-bot commented Nov 19, 2020

Uh oh!

fejta-bot commented Nov 19, 2020

Uh oh!

MHBauer commented Dec 4, 2020

Uh oh!

k8s-ci-robot commented Dec 4, 2020

Uh oh!

MHBauer commented Dec 4, 2020

Uh oh!

mikebrow commented Dec 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

MHBauer commented Oct 15, 2020 •

edited

Loading

mikebrow commented Oct 20, 2020 •

edited

Loading