Enable selinux tags in make targets #87658

dims · 2020-01-29T17:49:29Z

In 24d1059, a fix was made in bazel
based builds to ensure that we add selinux tag when we build all
binaries especially the kubelet. We need to do the same for in our
hack scripts so things like make release will work properly as well.

What type of PR is this?
/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes kubernetes/release#1037

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

dims · 2020-01-29T17:49:49Z

/sig release
/sig testing

dims · 2020-01-29T17:50:04Z

/priority important-soon

dims · 2020-01-29T17:50:32Z

/assign @cblecker @liggitt

dims · 2020-01-29T17:58:03Z

/hold

for reviews

cblecker · 2020-01-29T18:01:10Z

Seems fine to me.

/lgtm
/approve

dims · 2020-01-29T19:35:05Z

/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-ipv6
/test pull-kubernetes-e2e-gce-device-plugin-gpu

BenTheElder · 2020-01-29T20:29:12Z

How do I add another tag for providerless then?
What about typecheck?

liggitt · 2020-01-29T21:38:58Z

How do I add another tag for providerless then?
What about typecheck?

I wondered the same thing

dims · 2020-01-29T21:49:05Z

@BenTheElder we could do GOTAGS similar to the other variables (and append selinux at the end) in a follow up PR.

dims · 2020-01-29T21:49:20Z

/retest

dims · 2020-01-29T21:51:21Z

/hold cancel

dims · 2020-01-30T21:31:56Z

/test pull-kubernetes-e2e-gce-100-performance

dims · 2020-01-31T00:13:21Z

/test pull-kubernetes-e2e-gce-100-performance

dims · 2020-01-31T02:28:28Z

/assign @BenTheElder

BenTheElder · 2020-01-31T05:13:00Z

hack/lib/golang.sh

@@ -785,6 +788,18 @@ kube::golang::build_binaries() {
    goasmflags="-trimpath=${KUBE_ROOT}"
    gogcflags="${GOGCFLAGS:-} -trimpath=${KUBE_ROOT}"

+    # extract tags if any specified in GOFLAGS
+    # shellcheck disable=SC2001
+    gotags=$(echo "${GOFLAGS:-}" | sed -e 's|.*-tags=\([^-]*\).*|\1|')


I think this doesn't work correctly for multiple tags.
If I want to set multiple tags via GOFLAGS to go I have to use:
export GOFLAGS='-tags=selinux -tags=providerless'

this command will give me:

echo "${GOFLAGS:-}" | sed -e 's|.*-tags=$[^-]*$.*|\1|' providerless

EDIT: potential fix

Suggested change

gotags=$(echo "${GOFLAGS:-}" | sed -e 's|.*-tags=$[^-]*$.*|\1|')

gotags=$(echo "${GOFLAGS:-}" | grep -oe '-tags=$[^- ]*$' | tr '\n' ' ' | sed -e 's|-tags=$[^-]*$|\1|g')

something like the above edit, I'm sure it can be done more cleanly ... 🤔

Thankfully GOFLAGS specifically expects a space separated set of -flag=value so there may not be too many other edge cases ...

GOFLAGS A space-separated list of -flag=value settings to apply to go commands by default, when the given flag is known by the current command. Each entry must be a standalone flag. Because the entries are space-separated, flag values must not contain spaces. Flags listed on the command line are applied after this list and therefore override it.

https://golang.org/cmd/go/#hdr-Environment_variables

I am not sure if we even need the quote stripping.

@BenTheElder from what i understood -tags=selinux -tags=providerless the last one is picked up. So if you do this today selinux would not kick in.

So the test cases i had were as follows (quoting from memory):

-tags=providerless

-tags='providerless'

-tags="providerless"

-abc -tags=providerless

-abc -tags="providerless"

-abc -tags='providerless'

-abc -tags=providerless -xyz

-abc -tags="providerless" -xyz

-abc -tags='providerless' -xyz

-abc -tags="providerless selinux" -xyz

-abc -tags='providerless selinux' -xyz

So GOFLAGS cannot contain quoted values and the value in -flag=value cannot contain a space.

This means this flag couldn't have more than one value:
golang/go#26849

Except in go1.13 it may be comma seperated for cmd/go
golang/go@80e7832

Need to do some tests when I get to a machine.

Ack. please let me know. Let's wrap this up with spaces first please as the first iteration. i can follow up with support for comma.

[bentheelder@bentheelder:~/junk·2020-01-31T12:31:41-0800] $ GOFLAGS='-tags=providerless,selinux' go build . [bentheelder@bentheelder:~/junk·2020-01-31T12:31:48-0800] $ cat ./a.go // +build providerless,selinux package main func main() { println("test") }

versus:

[bentheelder@bentheelder:~/junk·2020-01-31T12:32:03-0800] $ GOFLAGS='-tags="providerless selinux"' go build . go: parsing $GOFLAGS: non-flag "selinux\""

we can replace all of this processing with just:

gotags="selinux,$(echo "${GOFLAGS:-}" | sed -e 's|.*-tags=$[^-]*$.*|\1|')"

test/typecheck/main.go

BenTheElder · 2020-01-31T20:36:48Z

hack/lib/golang.sh

+    # strip single quotes
+    gotags=${gotags//\'}
+    # trim leading and trailing whitespaces
+    gotags=$(echo "$gotags" | awk '{ gsub(/^[ \t]+|[ \t]+$/, ""); print }')


leading and trailing whitespace is illegal, as are the quotes, we can drop this and just , join with selinux
requires go 1.13+ but we're on that

BenTheElder · 2020-01-31T20:37:28Z

hack/lib/golang.sh

+    # trim leading and trailing whitespaces
+    gotags=$(echo "$gotags" | awk '{ gsub(/^[ \t]+|[ \t]+$/, ""); print }')
+    # ensure selinux is specified in the list of tags
+    gotags="selinux $gotags"


comma instead of a space

Suggested change

gotags="selinux $gotags"

gotags="selinux,${gotags}"

In 24d1059, a fix was made in bazel based builds to ensure that we add `selinux` tag when we build all binaries especially the `kubelet`. We need to do the same for in our hack scripts so things like `make release` will work properly as well. Some scripts use `GOFLAGS=-tags=providerless` for example, So we should support the tags to be specified in GOFLAGS as well. We parse out the tags from there and ensure selinux is added to the list of tags we used for building the binaries. Note that we add our own `-tags` with the full set of tags and since we specify our parameter at the end, ours full list takes precendence

BenTheElder · 2020-01-31T20:55:58Z

/lgtm
/approve
thanks dims!

k8s-ci-robot · 2020-01-31T20:56:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, cblecker, dims

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:

~~hack/OWNERS~~ [BenTheElder,cblecker,dims]
~~test/typecheck/OWNERS~~ [BenTheElder,cblecker]

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

BenTheElder · 2020-01-31T22:29:25Z

flake:

[sig-cli] Kubectl client Simple pod should support inline execution and attach expand_more

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/87658/pull-kubernetes-e2e-kind/1223347807302193152

cross is super flaky now as well so :/
/retest

BenTheElder · 2020-01-31T22:37:20Z

GCE timed out...?
/retest

dims · 2020-02-01T02:11:13Z

/retest

fejta-bot · 2020-02-01T08:47:01Z

/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.

simonswine · 2020-02-03T11:39:45Z

@kubernetes/patch-release-team I think this should ideally get cherry-picked into 1.16 and 1.17

See PRs #87752 and #87753

liggitt · 2020-02-05T13:29:29Z

is it expected that https://testgrid.k8s.io/google-gce#gci-gce-flaky&sort-by-flakiness=&width=20&include-filter-by-regex=SELinux are still failing? do we have e2e tests that verify this is effective?

gnufied · 2020-02-06T19:18:02Z

@liggitt I think those tests are faulty. They are failing on - https://github.com/kubernetes/kubernetes/blob/master/test/e2e/node/security_context.go#L272 which checks for ability to not read file correctly if MCS label's don't match. It is throwing following error:

 I0206 18:29:40.492] Feb  6 18:29:40.487: INFO: Running '/workspace/kubernetes/platforms/linux/amd64/kubectl exec --namespace=security-context-2598 security-context-6409b558-55a7-4ae8-b57c-e34291c7c723 -c=
       │ test-container -- cat /mounted_volume/TEST'
 872   │ I0206 18:29:41.804] Feb  6 18:29:41.798: INFO: error running kubectl exec to read file: exit status 1
 873   │ I0206 18:29:41.804] stdout=
 874   │ I0206 18:29:41.804] stderr=cat: can't open '/mounted_volume/TEST': No such file or directory
 875   │ I0206 18:29:41.805] command terminated with exit code 1

Which is exactly as it should happen. The code that does check for ability of pod to read/write the file appears to be passing succuessfully.

gnufied · 2020-02-06T19:47:49Z

If we go to a version before @dims fix the test fails in different place:

home/hekumar/goal/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/framework/framework.go:697
  should support volume SELinux relabeling [Flaky] [LinuxOnly] [It]
  /home/hekumar/goal/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/node/security_context.go:136
                                                     
  Feb  6 14:46:29.944: Unexpected error:
      <*exec.ExitError | 0xc00302dbe0>: {                                                                                                                                                                            
          ProcessState: {     
              pid: 3483,                                                                                                                                                                                                           status: 256,                                                                                                                                                                                           
              rusage: {                                                                                                                                                                                              
                  Utime: {Sec: 0, Usec: 108541},                                                                                                                                                                     
                  Stime: {Sec: 0, Usec: 23003},                                                                                                                                                                      
                  Maxrss: 121484,                                                                                                                                                                                    
                  Ixrss: 0,                                                                                                                                                                                          
                  Idrss: 0,                                                                                                                                                                                                            Isrss: 0,                                                                                                                                                                                          
                  Minflt: 4608,                                                                                                                                                                                                        Majflt: 0,                                                                                                                                                                                                           Nswap: 0,                                                                                                                                                                                                            Inblock: 0,                                                                                                                                                                                                          Oublock: 1168,                                                                                                                                                                                                       Msgsnd: 0,                                                                                                                                                                                         
                  Msgrcv: 0,
                  Nsignals: 0,
                  Nvcsw: 1488,       
                  Nivcsw: 56,                                                                             
              },                                                                                                                                                                                                     
          },                                     
          Stderr: nil,                                                                                                                                                                                               
      }                           
      exit status 1                                                                                       
  occurred                                                                                                
                                                                                                          
  /home/hekumar/goal/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/test/e2e/node/security_context.go:220
------------------------------

Which means it does not work with plain emptydir

…7658-upstream-release-1.17 Automated cherry pick of #87658: Enable selinux tags in make targets

…7658-upstream-release-1.16 Automated cherry pick of #87658: Enable selinux tags in make targets

k8s-ci-robot added sig/release Categorizes an issue or PR as relevant to SIG Release. 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 Jan 29, 2020

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 29, 2020

k8s-ci-robot assigned cblecker and liggitt Jan 29, 2020

k8s-ci-robot requested review from BenTheElder and lavalamp January 29, 2020 17:51

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

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2020

k8s-ci-robot assigned BenTheElder Jan 31, 2020

BenTheElder reviewed Jan 31, 2020

View reviewed changes

test/typecheck/main.go Show resolved Hide resolved

BenTheElder reviewed Jan 31, 2020

View reviewed changes

dims force-pushed the enable-selinux-tags-in-make-targets branch from 4072793 to dfd8e4e Compare January 31, 2020 20:49

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

k8s-ci-robot merged commit b7a1d61 into kubernetes:master Feb 1, 2020

k8s-ci-robot added this to the v1.18 milestone Feb 1, 2020

This was referenced Feb 2, 2020

Automated cherry pick of #87658: Enable selinux tags in make targets #87752

Merged

Automated cherry pick of #87658: Enable selinux tags in make targets #87753

Merged

gnufied mentioned this pull request Feb 6, 2020

Tests in selinux security_context are failing #87901

Closed

kolyshkin mentioned this pull request Feb 17, 2020

test/e2e/node: fix selinux test failure #88145

Closed

k8s-ci-robot added a commit that referenced this pull request Feb 21, 2020

Merge pull request #87753 from simonswine/automated-cherry-pick-of-#8…

50940f7

…7658-upstream-release-1.17 Automated cherry pick of #87658: Enable selinux tags in make targets

k8s-ci-robot added a commit that referenced this pull request Feb 21, 2020

Merge pull request #87752 from simonswine/automated-cherry-pick-of-#8…

8234b2a

…7658-upstream-release-1.16 Automated cherry pick of #87658: Enable selinux tags in make targets

dims mentioned this pull request Apr 12, 2020

selinux not working under containerd with selinux-enable=true containerd/containerd#4180

Closed

	gotags=$(echo "${GOFLAGS:-}" \| sed -e 's\|.-tags=\([^-]\).*\|\1\|')
	gotags=$(echo "${GOFLAGS:-}" \| grep -oe '-tags=\([^- ]\)' \| tr '\n' ' ' \| sed -e 's\|-tags=\([^-]\)\|\1\|g')

Enable selinux tags in make targets #87658

Enable selinux tags in make targets #87658

Conversation

dims commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 29, 2020

cblecker commented Jan 29, 2020

dims commented Jan 29, 2020

BenTheElder commented Jan 29, 2020

liggitt commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 29, 2020

dims commented Jan 30, 2020

dims commented Jan 31, 2020

dims commented Jan 31, 2020

BenTheElder Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Jan 31, 2020 • edited Loading

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

BenTheElder Jan 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Jan 31, 2020

k8s-ci-robot commented Jan 31, 2020

BenTheElder commented Jan 31, 2020

BenTheElder commented Jan 31, 2020

dims commented Feb 1, 2020

fejta-bot commented Feb 1, 2020

simonswine commented Feb 3, 2020

liggitt commented Feb 5, 2020

gnufied commented Feb 6, 2020 • edited Loading

gnufied commented Feb 6, 2020

BenTheElder Jan 31, 2020 •

edited

Loading

BenTheElder Jan 31, 2020 •

edited

Loading

BenTheElder Jan 31, 2020 •

edited

Loading

gnufied commented Feb 6, 2020 •

edited

Loading