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

Use a forked copy of multiarch/qemu-user-static scripts instead of a privileged container #69820

Merged
merged 4 commits into from
Nov 8, 2018

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented Oct 15, 2018

What this PR does / why we need it: in several of our Makefiles, we run docker run --privileged multiarch/qemu-user-static:register to register qemu in the host kernel, but this is roughly equivalent to curl | sudo sh.

As we have to run these operations as root, we should at least run copies of the script that we can maintain and audit.

Release note:

NONE

/assign @cblecker @dims @mkumatag @tallclair

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 15, 2018
@k8s-ci-robot k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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 Oct 15, 2018
@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

/assign @lavalamp
for third_party

@cblecker
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2018
@tallclair
Copy link
Member

Thanks, /lgtm

# enable automatic i386/ARM/M68K/MIPS/SPARC/PPC/s390/HPPA/Xtensa/microblaze
# program execution by the kernel
#
# downloaded from https://raw.githubusercontent.com/qemu/qemu/master/scripts/qemu-binfmt-conf.sh
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this file really downloaded from here @ixdy ? (the one in the url looks different)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah thanks!

/hold cancel

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, this file was actually copied from qemu/qemu into multiarch/qemu-user-static, where this comment was added (see https://github.com/multiarch/qemu-user-static/blob/22b0013668d2aed4a2cfd21650e85c664b1f21c6/register/qemu-binfmt-conf.sh#L5).

@dims
Copy link
Member

dims commented Oct 15, 2018

/hold

one question on the script inline

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 15, 2018
@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

Of course our build image doesn't have sudo. Thinking about how to fix this.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 15, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2018
@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

/hold cancel

I added some Makefile one-liners to fix the sudo issue, PTAL.

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 15, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 15, 2018
@@ -27,6 +27,8 @@ ALL_ARCH = amd64 arm arm64 ppc64le s390x
TEMP_DIR:=$(shell mktemp -d)
QEMUVERSION=v2.9.1

SUDO=$(if $(filter 0,$(shell id -u)),,sudo)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To explain this: make doesn't let you do equality checks in variable assignment, so this is next best option. Basically, filter pattern,text returns whitespace-separated words from text that match the pattern, so here we're returning 0 when the user is root and empty string otherwise.

the if in variable assignment considers nonempty strings as true, empty strings as false. thus when the effective user id is 0 (i.e. you're root), filter returns 0, which evaluates to true, and so we don't use sudo. Otherwise we do.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this implementation was basically copied from https://stackoverflow.com/a/9008922/9723266.)

@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

(I pushed new changes to reverse the order of arguments to filter, since they were backwards before. No functionality change, though.)

@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

oh, this is failing because I think the cross job isn't running in a privileged container.

I guess we can switch that, but it's a little unfortunate... cc @BenTheElder

@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

I wonder if we can move symlink creation to the hyperkube base image, and then remove the need for the register call entirely.

@BenTheElder
Copy link
Member

[FYI we discussed offline and Jeff is trying the route mentioned above]

@ixdy
Copy link
Member Author

ixdy commented Oct 15, 2018

cross-linking: #69832 is the PR to move symlink creation to the hyperkube base image.

I think we still want this PR, since there are cases (like creation of the base images) where we need to register the qemu handlers, and I'd rather use a local script instead of a privileged container.

@ixdy
Copy link
Member Author

ixdy commented Oct 17, 2018

I've rebased now that #69832 has merged. I believe this should be good to go now.

@dims
Copy link
Member

dims commented Oct 17, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2018
@ixdy
Copy link
Member Author

ixdy commented Oct 17, 2018

/assign @smarterclayton

can you review/approve the third_party change?

@dims
Copy link
Member

dims commented Oct 23, 2018

@thockin @smarterclayton can you please approve the changes under third_party?

@ixdy
Copy link
Member Author

ixdy commented Oct 25, 2018

/assign @lavalamp

Can someone please approve for third_party? :)

@ixdy
Copy link
Member Author

ixdy commented Nov 7, 2018

ping?

@lavalamp
Copy link
Member

lavalamp commented Nov 7, 2018

/approve

sorry didn't see this in my email until now

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, ixdy, lavalamp

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2018
@k8s-ci-robot k8s-ci-robot merged commit 3bf41a2 into kubernetes:master Nov 8, 2018
@BenTheElder BenTheElder mentioned this pull request Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants