set selinux labels on build docker containers when running pods in crio #17094

bparees · 2017-10-30T20:40:18Z

for bug https://bugzilla.redhat.com/show_bug.cgi?id=1507424

bparees · 2017-10-30T20:40:29Z

/test crio

openshift-merge-robot · 2017-10-30T20:40:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

bparees · 2017-10-30T20:40:57Z

/unassign @mfojtik @stevekuznetsov
/assign @mrunalp

bparees · 2017-10-30T20:51:29Z

/test crio

bparees · 2017-10-30T21:32:03Z

/test crio

bparees · 2017-10-30T21:58:05Z

/test crio

mrunalp · 2017-10-30T23:01:15Z

pkg/build/builder/util_linux.go

+			return fmt.Sprintf("label=level:%s", match[1]), nil
+		}
+	}
+	return "", nil


Do we want to return an error if we don't find a match or just skip adding SecurityOpt if we don't find it adding a warning in the log?

yeah i'll add a warning and skip.

Is this code being run within a container? Why not just use

if label, err := selinux.CurrentLabel(); err != nil { .... } level := selinux.NewContext("level")

import "github.com/opencontainers/selinux/go-selinux"

runcom · 2017-10-31T10:28:19Z

/test crio

runcom · 2017-10-31T10:42:05Z

/test crio

runcom · 2017-10-31T12:50:08Z

@bparees there are still build tests failing in the cri-o job in the CI

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/17094/test_pull_request_origin_extended_conformance_crio/76/

runcom · 2017-10-31T13:04:49Z

/test crio

pweil- · 2017-10-31T15:00:14Z

pkg/build/builder/util_linux.go

+		}
+		if match := selinuxLabelPattern.FindStringSubmatch(s.Text()); match != nil {
+			glog.V(6).Infof("found selinux labels: %v", match[1])
+			return fmt.Sprintf("label=level:%s", match[1]), nil


I thought we needed to match user/role/type as well here? What happens if there is a customer SELinux label on the container, no match?

s/customer/custom but in any case, response was explained here: #17094 (comment)

rhatdan · 2017-10-31T15:10:58Z

@pweil- It is a safe assumption that this is not the case. And you probably don't want to match the type. Most likely you are running a container that can talk to docker, which is probably a looser policy while you want to run the build with the traditional SELinux label.

In SELinux terms the User and Role are almost never used.

bparees · 2017-10-31T16:26:45Z

@bparees there are still build tests failing in the cri-o job in the CI

@runcom they look like networking problems now:

2017-10-31T14:43:28.274286308Z Fetching source index from https://rubygems.org/
2017-10-31T14:43:28.328875682Z Retrying source fetch due to error (2/3): Bundler::HTTPError Could not fetch specs from https://rubygems.org/
2017-10-31T14:43:28.340385595Z Retrying source fetch due to error (3/3): Bundler::HTTPError Could not fetch specs from https://rubygems.org/
2017-10-31T14:43:28.362562913Z Could not fetch specs from https://rubygems.org/
2017-10-31T14:43:28.726290541Z F1031 14:43:28.726242       1 helpers.go:120] error: build error: The command '/bin/sh -c scl enable rh-ruby22 "bundle install"' returned a non-zero code: 17

ashcrow · 2017-10-31T19:03:49Z

Travis is complaining on make verify-commits:

The following UPSTREAM commits modify Godeps repos other than the repo the commit declares:

[9d8e54aa7] UPSTREAM: 687: add securityopt arg support

  - pkg/build/vendor/github.com/fsouza/go-dockerclient/image.go

[ERROR] PID 2647: hack/lib/cmd.sh:241: `return "${return_code}"` exited with status 1.

ashcrow · 2017-10-31T20:05:27Z

extended_image_ecosystem:

Oct 31 19:21:09.014: INFO: At 2017-10-31 19:20:44 +0000 UTC - event for django-ex: {deployer-controller } FailedCreate: Error creating deployer pod: pods "django-ex-1-deploy" already exists

mrunalp · 2017-10-31T22:56:05Z

@bparees We preserved a test instance and I was able to logon and verify that regular pods have internet connectivity. If you can stop a build with assemble script on one of these then we can debug the networking issues tomorrow.

runcom · 2017-11-27T18:38:14Z

/retest

runcom · 2017-11-27T21:09:50Z

Cri-o wise, this PR looks good as it fixes build tests 👍

mrunalp · 2017-11-28T01:14:39Z

@bparees Can we get this merged?

bparees · 2017-11-28T01:24:42Z

@mrunalp it's fine with me, i assumed you wanted the crio tests passing.

bparees · 2017-11-28T01:25:04Z

also the "extended builds" suite is being skipped. i'd prefer to see it actually pass that suite.

bparees · 2017-11-28T01:25:16Z

(for which we need @stevekuznetsov to tell us why it's being skipped)

mrunalp · 2017-11-28T01:29:12Z

@bparees If we pass all the build tests here then we should be fine merging. Of the remaining 4 we have a PR for fixing one (#17198) and the other 3 are still being looked at.

runcom · 2017-11-28T16:04:14Z

/test crio

stevekuznetsov · 2017-11-28T18:54:37Z

@bparees the extended build test looks like it ran and failed -- what do you mean?

bparees · 2017-11-28T19:21:55Z

@stevekuznetsov wasn't the situation last night.

runcom · 2017-11-28T20:22:06Z

/test crio

bparees · 2017-11-28T22:25:13Z

/test extended_builds

runcom · 2017-11-28T22:31:25Z

/test extended_conformance_install

openshift-ci-robot · 2017-11-28T23:51:16Z

@bparees: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_conformance_install_update	`6744e6c`	link	`/test extended_conformance_install_update`
ci/openshift-jenkins/extended_conformance_crio	`bce732a`	link	`/test crio`
ci/openshift-jenkins/extended_builds	`bce732a`	link	`/test extended_builds`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

openshift-merge-robot · 2017-11-29T03:54:09Z

Automatic merge from submit-queue (batch tested with PRs 17476, 17143, 15115, 17094, 17500).

runcom · 2017-11-29T14:34:51Z

@bparees @mrunalp this should be back ported to release-1.7 branch as well

bparees · 2017-11-29T15:02:58Z

@runcom why?

runcom · 2017-11-29T15:05:08Z

@bparees to fix builds there as well?

bparees · 2017-11-29T15:09:48Z

@runcom we don't generally backport things to older origin releases. I assume we didn't make any claims about crio support in 3.7 (since we didn't get it working before 3.7 shipped) so i don't see why we'd backport it now.

mrunalp · 2017-11-29T15:09:59Z

We did ship cri-o in tech preview for 3.7.

runcom · 2017-11-29T16:00:30Z

@runcom we don't generally backport things to older origin releases. I assume we didn't make any claims about crio support in 3.7 (since we didn't get it working before 3.7 shipped) so i don't see why we'd backport it now.

as @mrunalp said, we shipped it as tech preview and we have this working now. It would be nice to have this backported so people can actually start using CRI-O if they want and we get feedback and all the rest

stevekuznetsov · 2017-11-29T16:05:25Z

We're not usually in the business of releasing point patches on top of old Origin releases. /cc @smarterclayton

runcom · 2017-11-29T16:13:31Z

We're not usually in the business of releasing point patches on top of old Origin releases. /cc @smarterclayton

ack ack, sorry for the noise :)

@smarterclayton

Automatic merge from submit-queue. [release-3.7] setup selinux labels for build containers back ports #17094 to `release-3.7` @smarterclayton PTAL

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 30, 2017

openshift-merge-robot assigned mfojtik Oct 30, 2017

openshift-merge-robot assigned stevekuznetsov Oct 30, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review vendor-update Touching vendor dir or related files labels Oct 30, 2017

openshift-ci-robot assigned mrunalp and unassigned mfojtik and stevekuznetsov Oct 30, 2017

bparees force-pushed the crio_selinux branch from 0c0ad58 to bd52969 Compare October 30, 2017 20:42

bparees force-pushed the crio_selinux branch from bd52969 to 24d2b87 Compare October 30, 2017 21:31

bparees force-pushed the crio_selinux branch from 24d2b87 to e545775 Compare October 30, 2017 21:57

mrunalp reviewed Oct 30, 2017

View reviewed changes

pweil- reviewed Oct 31, 2017

View reviewed changes

bparees force-pushed the crio_selinux branch from e545775 to f8c2cee Compare October 31, 2017 18:08

setup selinux labels for build containers

bce732a

bparees force-pushed the crio_selinux branch from c40d1ce to bce732a Compare November 28, 2017 14:36

bparees added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2017

openshift-merge-robot merged commit 2748ff8 into openshift:master Nov 29, 2017

bparees deleted the crio_selinux branch November 29, 2017 18:13

runcom mentioned this pull request Nov 30, 2017

[release-3.7] setup selinux labels for build containers #17546

Merged

openshift-merge-robot added a commit that referenced this pull request Feb 15, 2018

Merge pull request #17546 from runcom/backport-17094

a8deba5

Automatic merge from submit-queue. [release-3.7] setup selinux labels for build containers back ports #17094 to `release-3.7` @smarterclayton PTAL

set selinux labels on build docker containers when running pods in crio #17094

set selinux labels on build docker containers when running pods in crio #17094

Conversation

bparees commented Oct 30, 2017 • edited Loading

bparees commented Oct 30, 2017

openshift-merge-robot commented Oct 30, 2017

bparees commented Oct 30, 2017

bparees commented Oct 30, 2017

bparees commented Oct 30, 2017

bparees commented Oct 30, 2017

mrunalp Oct 30, 2017

Choose a reason for hiding this comment

bparees Oct 31, 2017

Choose a reason for hiding this comment

rhatdan Oct 31, 2017

Choose a reason for hiding this comment

rhatdan Oct 31, 2017

Choose a reason for hiding this comment

runcom commented Oct 31, 2017

runcom commented Oct 31, 2017

runcom commented Oct 31, 2017 • edited Loading

runcom commented Oct 31, 2017

pweil- Oct 31, 2017

Choose a reason for hiding this comment

pweil- Oct 31, 2017

Choose a reason for hiding this comment

rhatdan commented Oct 31, 2017

bparees commented Oct 31, 2017

ashcrow commented Oct 31, 2017

ashcrow commented Oct 31, 2017

mrunalp commented Oct 31, 2017 • edited Loading

runcom commented Nov 27, 2017

runcom commented Nov 27, 2017

mrunalp commented Nov 28, 2017

bparees commented Nov 28, 2017

bparees commented Nov 28, 2017

bparees commented Nov 28, 2017

mrunalp commented Nov 28, 2017

runcom commented Nov 28, 2017

stevekuznetsov commented Nov 28, 2017

bparees commented Nov 28, 2017

runcom commented Nov 28, 2017

bparees commented Nov 28, 2017

runcom commented Nov 28, 2017

openshift-ci-robot commented Nov 28, 2017

openshift-merge-robot commented Nov 29, 2017

runcom commented Nov 29, 2017

bparees commented Nov 29, 2017

runcom commented Nov 29, 2017

bparees commented Nov 29, 2017

mrunalp commented Nov 29, 2017

runcom commented Nov 29, 2017

stevekuznetsov commented Nov 29, 2017

runcom commented Nov 29, 2017

bparees commented Oct 30, 2017 •

edited

Loading

runcom commented Oct 31, 2017 •

edited

Loading

mrunalp commented Oct 31, 2017 •

edited

Loading