hack: Add :z to --volume mounts #174

wking · 2018-08-24T21:27:50Z

Like we did in bootkube.sh in 0fa4eb1 (#134). This gives us permission to access the mounted volume when SELinux is enabled (docs here).

I've also normalized these invocations for consistency between the various hack/ scripts:

Adding slash separators to put each option on its own line, excepting the final command being run in the container. This makes the long commands slightly easier to skim. It will also make it easier to track down motivation for an option with git blame, because commits touching options on other lines won't clutter the blame.
Use long-form options (-v -> --volume, etc.). This makes the options a bit more accessible to newcomers, and now that each option is on it's own line we have plenty of space.
Dropped single quotes from 'TRUE'. There are no shell-sensitive characters in TRUE, so there's no need to quote it.
Use ${PWD} consistently. It's in POSIX, so there's no need to execute a pwd process to get this value.
Drop -t. None of these commands should need a pseudoterminal.
Drop explicit rw --volume options. They're the default.

eparis · 2018-08-24T21:56:43Z

hack/test-bazel-build-tarball.sh

+  docker run --rm \
+    --env IS_CONTAINER=TRUE \
+    --volume "${PWD}:${PWD}:z" \
+    --volume /tmp:/tmp:z \


This seems dangerous... You might be relabling something on your local host that another process would be using...

This seems dangerous...

Are you only concerned about the host /tmp? Or are you also concerned about ${PWD}? I expect we can get this working without the /tmp mount, but I don't see a way around the ${PWD} mount.

only /tmp. GDM, X, systemd, and other things put files in /tmp and expect to be able to access them. If we use :z when mounting /tmp we will be relabeling those files and might remove permission for the proper owner to work. We will also be relabeling /tmp itself and messing up the labels that other programs create in /tmp...

${PWD} won't likely be used by (many) other programs in this case. So relabeling seems quite reasonable.

Does the container need some content in /tmp?

Does the container need some content in /tmp?

We may be able to drop the mount, since we didn't have it before. @sallyom, do you remember why you added it?

wking · 2018-08-25T21:47:25Z

/retest

wking · 2018-08-26T01:49:30Z

/test go-fmt

Exercising openshift/release#1283. I think @eparis did this earlier, but now I can't find his comment. And the failure I expect will block this PR, but this PR isn't particularly important.

wking · 2018-08-26T01:52:45Z

And here are the gofmt errors.

eparis · 2018-08-26T03:30:35Z

merging #181 should fix gofmt...

wking · 2018-08-27T18:14:09Z

I pushed a new commit dropping the /tmp mount from hack/test-bazel-build-tarball.sh (previous discussion here), and the Bazel tests still pass. So I think we're good to go here.

eparis · 2018-08-27T19:12:27Z

hack/tf-fmt.sh

+  docker run --rm \
+    --env IS_CONTAINER=TRUE \
+    --volume "${PWD}:${PWD}:ro,z" \
+    --volume /tmp:/tmp:z \


Here as well?

Here as well?

Ah, sorry. Fixed with 876f0af -> df614c4.

Like we did in bootkube.sh in 0fa4eb1 (Fix perm errors with selinux enabled, 2018-08-15, openshift#134). This gives us permission to access the mounted volume when SELinux is enabled (docs in [1]). I've also normalized these invocations for consistency between the various hack/ scripts: * Adding slash separators to put each option on its own line, excepting the final command being run in the container. This makes the long commands slightly easier to skim. It will also make it easier to track down motivation for an option with 'git blame', because commits touching options on other lines won't clutter the blame. * Use long-form options (-v -> --volume, etc.). This makes the options a bit more accessible to newcomers, and now that each option is on it's own line we have plenty of space. * Dropped single quotes from 'TRUE'. There are no shell-sensitive characters in TRUE, so there's no need to quote it. * Use ${PWD} consistently. It's in POSIX [2], so there's no need to execute a pwd process to get this value. * Drop -t. None of these commands should need a pseudoterminal. * Drop explicit rw --volume options. They're the default [3]. [1]: https://github.com/containers/libpod/blame/v0.8.3/docs/podman-run.1.md#L628 [2]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 [3]: https://github.com/containers/libpod/blame/v0.8.3/docs/podman-run.1.md#L646

Eric points out potential issues with relabeling /tmp [1], which is shared by several system-level consumers. For the Bazel script, the /tmp mount is just since c483f59 (Move bazel build tarball test to prow, 2018-08-08, openshift#117), so we can drop it to return to our previous approach. The Terraform container seems to run fine without /tmp as well, although there's no clear history to point to on this front because we used to use Bazel for this. See b8a9bbc (Remove bazel from test process, 2018-08-01, openshift#97). [1]: openshift#174 (comment)

wking · 2018-08-28T03:50:45Z

Rebased around #185 with df614c4 -> 4ba649b.

PWD is in POSIX [1], so there's no need to execute a pwd process to get this value. This fixes everthing found by: $ git grep '$pwd$' except for hack/*.sh, which is being addressed by [2]. I'm using $PWD instead of ${PWD} in the module files to avoid them being interpolated [3] on template rendering [4]. [1]: http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03 [2]: openshift#174 [3]: https://www.terraform.io/docs/configuration/interpolation.html [4]: https://www.terraform.io/docs/providers/template/d/file.html

yifan-gu

/lgtm

openshift-ci-robot · 2018-08-30T23:23:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wking, yifan-gu

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:

~~OWNERS~~ [wking,yifan-gu]

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

wking · 2018-08-30T23:37:29Z

The e2e-aws error was:

1 error(s) occurred:

* module.masters.aws_instance.master[1]: 1 error(s) occurred:

* aws_instance.master.1: Error waiting for instance (i-01bc66e5ec329945b) to become ready: unexpected state 'shutting-down', wanted target 'running'. last error: %!s(<nil>)

Dunno what's going on there.

/retest

wking · 2018-08-30T23:51:22Z

The e2e-aws error was:

4 error(s) occurred:

* module.vpc.aws_elb.console: 1 error(s) occurred:

* aws_elb.console: DuplicateLoadBalancerName: Load Balancer named ci-op-wnjsgc2k-68485-con already exists and it is configured with different parameters.
	status code: 400, request id: e26a590a-acad-11e8-891d-5582d52fa5a2
* module.vpc.aws_elb.api_external: 1 error(s) occurred:

* aws_elb.api_external: DuplicateLoadBalancerName: Load Balancer named ci-op-wnjsgc2k-68485-ext already exists and it is configured with different parameters.
	status code: 400, request id: e2722072-acad-11e8-a52b-95156fa00b0f
* module.vpc.aws_elb.tnc: 1 error(s) occurred:

* aws_elb.tnc: DuplicateLoadBalancerName: Load Balancer named ci-op-wnjsgc2k-68485-tnc already exists and it is configured with different parameters.
	status code: 400, request id: e2a0f93a-acad-11e8-9b83-879645239b41
* module.vpc.aws_elb.api_internal: 1 error(s) occurred:

* aws_elb.api_internal: DuplicateLoadBalancerName: Load Balancer named ci-op-wnjsgc2k-68485-int already exists and it is configured with different parameters.
	status code: 400, request id: e2a0f960-acad-11e8-a7e1-034ef6375317

I'll give things some time to calm down, reap leaked AWS resources from the Prow account, and kick this again after reaping.

openshift-bot · 2018-08-31T01:03:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from aaronlevy and smarterclayton August 24, 2018 21:27

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 24, 2018

wking force-pushed the hack-volume-z branch from 3f60aeb to 0d80dbc Compare August 24, 2018 21:28

eparis reviewed Aug 24, 2018

View reviewed changes

wking force-pushed the hack-volume-z branch from 0d80dbc to 876f0af Compare August 27, 2018 18:11

eparis reviewed Aug 27, 2018

View reviewed changes

wking force-pushed the hack-volume-z branch from 876f0af to df614c4 Compare August 27, 2018 19:22

wking added 2 commits August 27, 2018 20:49

wking force-pushed the hack-volume-z branch from df614c4 to 4ba649b Compare August 28, 2018 03:50

wking mentioned this pull request Aug 29, 2018

*: Replace '$(pwd)' with '${PWD}' #192

Merged

openshift-ci-robot assigned yifan-gu Aug 30, 2018

yifan-gu approved these changes Aug 30, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2018

openshift-merge-robot merged commit ce790c2 into openshift:master Aug 31, 2018

wking deleted the hack-volume-z branch August 31, 2018 16:39

wking mentioned this pull request Sep 15, 2018

hack/tf-fmt.sh doesn't work with selinux in enforcing #164

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hack: Add :z to --volume mounts #174

hack: Add :z to --volume mounts #174

wking commented Aug 24, 2018

eparis Aug 24, 2018

wking Aug 25, 2018

eparis Aug 25, 2018

rhatdan Aug 26, 2018

wking Aug 26, 2018

wking commented Aug 25, 2018

wking commented Aug 26, 2018

wking commented Aug 26, 2018

eparis commented Aug 26, 2018

wking commented Aug 27, 2018

eparis Aug 27, 2018

wking Aug 27, 2018

wking commented Aug 28, 2018

yifan-gu left a comment

openshift-ci-robot commented Aug 30, 2018

wking commented Aug 30, 2018

wking commented Aug 30, 2018

openshift-bot commented Aug 31, 2018

hack: Add :z to --volume mounts #174

hack: Add :z to --volume mounts #174

Conversation

wking commented Aug 24, 2018

eparis Aug 24, 2018

Choose a reason for hiding this comment

wking Aug 25, 2018

Choose a reason for hiding this comment

eparis Aug 25, 2018

Choose a reason for hiding this comment

rhatdan Aug 26, 2018

Choose a reason for hiding this comment

wking Aug 26, 2018

Choose a reason for hiding this comment

wking commented Aug 25, 2018

wking commented Aug 26, 2018

wking commented Aug 26, 2018

eparis commented Aug 26, 2018

wking commented Aug 27, 2018

eparis Aug 27, 2018

Choose a reason for hiding this comment

wking Aug 27, 2018

Choose a reason for hiding this comment

wking commented Aug 28, 2018

yifan-gu left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 30, 2018

wking commented Aug 30, 2018

wking commented Aug 30, 2018

openshift-bot commented Aug 31, 2018