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.

Sign up for GitHub

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

Jump to bottom

Adding setcap image so that capabilities can be applied to #1684

Merged
merged 1 commit into from
Feb 12, 2021
Merged

Adding setcap image so that capabilities can be applied to #1684

merged 1 commit into from
Feb 12, 2021

Conversation

vinayakankugoyal
Copy link
Contributor

@vinayakankugoyal vinayakankugoyal commented Nov 5, 2020 •
edited
Loading

kubernetes binaries.

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR creates an image based on debian-base with libcap2-bin installed so that we can apply capabilities to k8s binaries like kube-apiserver. This will allow kube-apiserver to bind to port lower than 1024 without requiring it to run as root.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

For reference please see: kubernetes/kubernetes#96134

Does this PR introduce a user-facing change?

Added a new setcap image, that can be used to apply capabilities to the k8s binaries when we build the server-images.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Nov 5, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @vinayakankugoyal!

It looks like this is your first PR to kubernetes/release 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/release has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 5, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @vinayakankugoyal. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/release-eng Issues or PRs related to the Release Engineering subproject labels Nov 5, 2020
@k8s-ci-robot k8s-ci-robot requested review from dougm and tpepper November 5, 2020 01:35
@k8s-ci-robot k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Nov 5, 2020
@vinayakankugoyal
Copy link
Contributor Author

/cc @justaugustus

@vinayakankugoyal vinayakankugoyal mentioned this pull request Nov 5, 2020
Merged
@saschagrunert
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 5, 2020
@vinayakankugoyal
Copy link
Contributor Author

/cc @BenTheElder

@vinayakankugoyal vinayakankugoyal force-pushed the setcap branch 2 times, most recently from 6c8bc20 to c79082c Compare November 7, 2020 00:24
@vinayakankugoyal
Copy link
Contributor Author

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt November 9, 2020 17:27
@liggitt
Copy link
Member

liggitt commented Nov 9, 2020

/cc @dekkagaijin

@vinayakankugoyal
Copy link
Contributor Author

/cc saschagrunert

@vinayakankugoyal
Copy link
Contributor Author

vinayakankugoyal commented Feb 11, 2021 •
edited
Loading

@vinayakankugoyal @BenTheElder so ... i got curious (which is typically dangerous!) and i dug in. Here's what i ended up with:

So one another option is to use this to write a small go based binary that we could use anywhere and not rely on the hoops we have to run through here just for debian. (may be throw this binary into go-runner? and/or other base images?)

Thanks @dims! A few thoughts (I am a newbie compared to all the people on the review so might be completely wrong)

  1. IMO installing a package on a base image is not an anti-pattern but probably the pattern that most images are created using?
  2. Writing the go-binary would basically give us a rudimentary version of what setcap already does. Won't that defeat the whole point of reusability?
  3. I did make sub-build-s390x under debian-iptables and that seems to fail too, so we would have to solve this anyways? Logs below:
k8s.io/release/images/build/debian-iptables   setcap  make sub-build-s390x                                                                                ✔  10:11:50 
make ARCH=s390x build
make[1]: Entering directory '<redacted>/go/src/k8s.io/release/images/build/debian-iptables'
cp -r ./buster /tmp/tmp.Vu7Szcxecx/
cd /tmp/tmp.Vu7Szcxecx/buster && sed -i "s|BASEIMAGE|k8s.gcr.io/build-image/debian-base-s390x:buster-v1.4.0|g" Dockerfile
# Register /usr/bin/qemu-ARCH-static as the handler for non-x86 binaries in the kernel
docker run --rm --privileged multiarch/qemu-user-static:5.2.0-2 --reset -p yes
Setting /usr/bin/qemu-alpha-static as binfmt interpreter for alpha
Setting /usr/bin/qemu-arm-static as binfmt interpreter for arm
Setting /usr/bin/qemu-armeb-static as binfmt interpreter for armeb
Setting /usr/bin/qemu-sparc-static as binfmt interpreter for sparc
Setting /usr/bin/qemu-sparc32plus-static as binfmt interpreter for sparc32plus
Setting /usr/bin/qemu-sparc64-static as binfmt interpreter for sparc64
Setting /usr/bin/qemu-ppc-static as binfmt interpreter for ppc
Setting /usr/bin/qemu-ppc64-static as binfmt interpreter for ppc64
Setting /usr/bin/qemu-ppc64le-static as binfmt interpreter for ppc64le
Setting /usr/bin/qemu-m68k-static as binfmt interpreter for m68k
Setting /usr/bin/qemu-mips-static as binfmt interpreter for mips
Setting /usr/bin/qemu-mipsel-static as binfmt interpreter for mipsel
Setting /usr/bin/qemu-mipsn32-static as binfmt interpreter for mipsn32
Setting /usr/bin/qemu-mipsn32el-static as binfmt interpreter for mipsn32el
Setting /usr/bin/qemu-mips64-static as binfmt interpreter for mips64
Setting /usr/bin/qemu-mips64el-static as binfmt interpreter for mips64el
Setting /usr/bin/qemu-sh4-static as binfmt interpreter for sh4
Setting /usr/bin/qemu-sh4eb-static as binfmt interpreter for sh4eb
Setting /usr/bin/qemu-s390x-static as binfmt interpreter for s390x
Setting /usr/bin/qemu-aarch64-static as binfmt interpreter for aarch64
Setting /usr/bin/qemu-aarch64_be-static as binfmt interpreter for aarch64_be
Setting /usr/bin/qemu-hppa-static as binfmt interpreter for hppa
Setting /usr/bin/qemu-riscv32-static as binfmt interpreter for riscv32
Setting /usr/bin/qemu-riscv64-static as binfmt interpreter for riscv64
Setting /usr/bin/qemu-xtensa-static as binfmt interpreter for xtensa
Setting /usr/bin/qemu-xtensaeb-static as binfmt interpreter for xtensaeb
Setting /usr/bin/qemu-microblaze-static as binfmt interpreter for microblaze
Setting /usr/bin/qemu-microblazeel-static as binfmt interpreter for microblazeel
Setting /usr/bin/qemu-or1k-static as binfmt interpreter for or1k
docker buildx version
github.com/docker/buildx v0.5.1-docker 11057da37336192bfc57d81e02359ba7ba848e4a
BUILDER=determined_hodgkin
docker buildx build \
        --pull \
        --load \
        --platform linux/s390x \
        -t "gcr.io/k8s-staging-build-image"/debian-iptables-s390x:buster-v1.5.0 \
        -t "gcr.io/k8s-staging-build-image"/debian-iptables-s390x:v0.7.0-65-g8350397e-buster \
        -t "gcr.io/k8s-staging-build-image"/debian-iptables-s390x:latest-buster \
        --build-arg=IPTABLES_VERSION=1.8.5 \
        /tmp/tmp.Vu7Szcxecx/buster
[+] Building 24.1s (7/11)                                                                                                                                                             
 => [internal] booting buildkit                                                                                                                                                  1.2s
 => => pulling image moby/buildkit:buildx-stable-1                                                                                                                               0.3s
 => => creating container buildx_buildkit_determined_hodgkin0                                                                                                                    0.9s
 => [internal] load build definition from Dockerfile                                                                                                                             0.1s
 => => transferring dockerfile: 1.82kB                                                                                                                                           0.0s
 => [internal] load .dockerignore                                                                                                                                                0.1s
 => => transferring context: 2B                                                                                                                                                  0.0s
 => [internal] load metadata for k8s.gcr.io/build-image/debian-base-s390x:buster-v1.4.0                                                                                          0.7s
 => [1/6] FROM k8s.gcr.io/build-image/debian-base-s390x:buster-v1.4.0@sha256:610526b047d4b528d9e14b4f15347aa4e37af0c47e1307a2f7aebf8745c8a323                                    1.6s
 => => resolve k8s.gcr.io/build-image/debian-base-s390x:buster-v1.4.0@sha256:610526b047d4b528d9e14b4f15347aa4e37af0c47e1307a2f7aebf8745c8a323                                    0.0s
 => => sha256:376d61ddc98ab78b241628e7663b118ecccbc700046029c73e2a333219d21bce 23.06MB / 23.06MB                                                                                 1.0s
 => => extracting sha256:376d61ddc98ab78b241628e7663b118ecccbc700046029c73e2a333219d21bce                                                                                        0.6s
 => [internal] load build context                                                                                                                                                0.1s
 => => transferring context: 1.73kB                                                                                                                                              0.0s
 => ERROR [2/6] RUN echo deb http://deb.debian.org/debian buster-backports main >> /etc/apt/sources.list     && apt-get update     && apt-get -t buster-backports -y --no-inst  19.9s
------                                                                                                                                                                                
 > [2/6] RUN echo deb http://deb.debian.org/debian buster-backports main >> /etc/apt/sources.list     && apt-get update     && apt-get -t buster-backports -y --no-install-recommends install         iptables=1.8.5*         ebtables:                                                                                                                                     
#5 0.618 Get:1 http://security.debian.org/debian-security buster/updates InRelease [65.4 kB]
#5 0.619 Get:2 http://deb.debian.org/debian buster InRelease [122 kB]
#5 0.715 Get:3 http://deb.debian.org/debian buster-updates InRelease [51.9 kB]
#5 0.715 Get:4 http://deb.debian.org/debian buster-backports InRelease [46.7 kB]
#5 2.880 Get:5 http://security.debian.org/debian-security buster/updates/main s390x Packages [238 kB]
#5 4.470 Get:6 http://deb.debian.org/debian buster/main s390x Packages [7624 kB]
#5 6.399 Get:7 http://deb.debian.org/debian buster-updates/main s390x Packages [9508 B]
#5 8.355 Get:8 http://deb.debian.org/debian buster-backports/main s390x Packages [406 kB]
#5 9.123 Fetched 8563 kB in 9s (1005 kB/s)
#5 9.123 Reading package lists...
#5 13.01 Reading package lists...
#5 16.73 Building dependency tree...
#5 17.45 Reading state information...
#5 18.12 The following additional packages will be installed:
#5 18.12   libip4tc2 libip6tc2 libmnl0 libnetfilter-conntrack3 libnfnetlink0 libnftnl11
#5 18.12   libxtables12 netbase
#5 18.13 Suggested packages:
#5 18.13   firewalld kmod
#5 18.13 Recommended packages:
#5 18.13   kmod nftables
#5 18.36 The following NEW packages will be installed:
#5 18.36   ebtables iptables libip4tc2 libip6tc2 libmnl0 libnetfilter-conntrack3
#5 18.37   libnfnetlink0 libnftnl11 libxtables12 netbase
#5 18.52 0 upgraded, 10 newly installed, 0 to remove and 7 not upgraded.
#5 18.52 Need to get 695 kB of archives.
#5 18.52 After this operation, 3106 kB of additional disk space will be used.
#5 18.52 Get:1 http://deb.debian.org/debian buster-backports/main s390x libip4tc2 s390x 1.8.5-3~bpo10+1 [33.8 kB]
#5 18.53 Get:2 http://deb.debian.org/debian buster-backports/main s390x libip6tc2 s390x 1.8.5-3~bpo10+1 [34.1 kB]
#5 18.54 Get:3 http://deb.debian.org/debian buster-backports/main s390x libxtables12 s390x 1.8.5-3~bpo10+1 [42.7 kB]
#5 18.54 Get:4 http://deb.debian.org/debian buster-backports/main s390x netbase all 6.1~bpo10+1 [19.9 kB]
#5 18.54 Get:5 http://deb.debian.org/debian buster/main s390x libmnl0 s390x 1.0.4-2 [12.4 kB]
#5 18.55 Get:6 http://deb.debian.org/debian buster/main s390x libnfnetlink0 s390x 1.0.1-3+b1 [13.7 kB]
#5 18.55 Get:7 http://deb.debian.org/debian buster/main s390x libnetfilter-conntrack3 s390x 1.0.7-1 [39.0 kB]
#5 18.55 Get:8 http://deb.debian.org/debian buster-backports/main s390x libnftnl11 s390x 1.1.7-1~bpo10+1 [54.8 kB]
#5 18.55 Get:9 http://deb.debian.org/debian buster-backports/main s390x iptables s390x 1.8.5-3~bpo10+1 [363 kB]
#5 18.60 Get:10 http://deb.debian.org/debian buster-backports/main s390x ebtables s390x 2.0.11-4~bpo10+1 [81.5 kB]
#5 19.41 debconf: delaying package configuration, since apt-utils is not installed
#5 19.52 Fetched 695 kB in 0s (3499 kB/s)
#5 19.58 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.58 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.58 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/0-libip4tc2_1.8.5-3~bpo10+1_s390x.deb (--unpack):
#5 19.58  dpkg-deb --control subprocess returned error exit status 1
#5 19.59 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.59 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.59 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/1-libip6tc2_1.8.5-3~bpo10+1_s390x.deb (--unpack):
#5 19.59  dpkg-deb --control subprocess returned error exit status 1
#5 19.60 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.60 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.60 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/2-libxtables12_1.8.5-3~bpo10+1_s390x.deb (--unpack):
#5 19.60  dpkg-deb --control subprocess returned error exit status 1
#5 19.60 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.60 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.60 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/3-netbase_6.1~bpo10+1_all.deb (--unpack):
#5 19.60  dpkg-deb --control subprocess returned error exit status 1
#5 19.61 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.61 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.61 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/4-libmnl0_1.0.4-2_s390x.deb (--unpack):
#5 19.61  dpkg-deb --control subprocess returned error exit status 1
#5 19.61 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.61 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.61 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/5-libnfnetlink0_1.0.1-3+b1_s390x.deb (--unpack):
#5 19.61  dpkg-deb --control subprocess returned error exit status 1
#5 19.62 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.62 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.62 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/6-libnetfilter-conntrack3_1.0.7-1_s390x.deb (--unpack):
#5 19.62  dpkg-deb --control subprocess returned error exit status 1
#5 19.63 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.63 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.63 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/7-libnftnl11_1.1.7-1~bpo10+1_s390x.deb (--unpack):
#5 19.63  dpkg-deb --control subprocess returned error exit status 1
#5 19.63 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.64 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.65 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/8-iptables_1.8.5-3~bpo10+1_s390x.deb (--unpack):
#5 19.65  dpkg-deb --control subprocess returned error exit status 1
#5 19.65 Error while loading /usr/sbin/dpkg-split: No such file or directory
#5 19.65 Error while loading /usr/sbin/dpkg-deb: No such file or directory
#5 19.65 dpkg: error processing archive /tmp/apt-dpkg-install-JLYVoM/9-ebtables_2.0.11-4~bpo10+1_s390x.deb (--unpack):
#5 19.65  dpkg-deb --control subprocess returned error exit status 1
#5 19.68 Errors were encountered while processing:
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/0-libip4tc2_1.8.5-3~bpo10+1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/1-libip6tc2_1.8.5-3~bpo10+1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/2-libxtables12_1.8.5-3~bpo10+1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/3-netbase_6.1~bpo10+1_all.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/4-libmnl0_1.0.4-2_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/5-libnfnetlink0_1.0.1-3+b1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/6-libnetfilter-conntrack3_1.0.7-1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/7-libnftnl11_1.1.7-1~bpo10+1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/8-iptables_1.8.5-3~bpo10+1_s390x.deb
#5 19.68  /tmp/apt-dpkg-install-JLYVoM/9-ebtables_2.0.11-4~bpo10+1_s390x.deb
#5 19.76 E: Sub-process /usr/bin/dpkg returned an error code (1)
------
Dockerfile:19
--------------------
  18 |     ARG IPTABLES_VERSION
  19 | >>> RUN echo deb http://deb.debian.org/debian buster-backports main >> /etc/apt/sources.list \
  20 | >>>     && apt-get update \
  21 | >>>     && apt-get -t buster-backports -y --no-install-recommends install \
  22 | >>>         iptables=${IPTABLES_VERSION}* \
  23 | >>>         ebtables
  24 |     
--------------------
error: failed to solve: rpc error: code = Unknown desc = executor failed running [/dev/.buildkit_qemu_emulator /bin/sh -c echo deb http://deb.debian.org/debian buster-backports main >> /etc/apt/sources.list     && apt-get update     && apt-get -t buster-backports -y --no-install-recommends install         iptables=${IPTABLES_VERSION}*         ebtables]: exit code: 100
make[1]: *** [Makefile:48: build] Error 1
make[1]: Leaving directory '<redacted>/go/src/k8s.io/release/images/build/debian-iptables'
make: *** [Makefile:70: sub-build-s390x] Error 2

@BenTheElder
Copy link
Member

So one another option is to use this to write a small go based binary that we could use anywhere and not rely on the hoops we have to run through here just for debian. (may be throw this binary into go-runner? and/or other base images?)

The binary sounds OK to me I suppose, but I think either way we want this in a build-time image only, it's sort of useless at runtime, just extra image size..?

It could be in say kube-cross if we don't want a new image? But I think? the build will perform better if it's in a small image for the FROM setcap; COPY foo /foo; RUN setcap /foo; FROM real-base; COPY --from=0 /foo /foo.

@vinayakankugoyal
Copy link
Contributor Author

Wondering if we could use this https://github.com/scionproto/docker-caps

@BenTheElder
Copy link
Member

We typically prefer to have all images hosted under our infra:

  • easier to keep track of / mirror
  • no rate limits

Though we have precedent for essentially just mirroring an image (we're doing this for some e2e images) so we can still pull from k8s.gcr.io without necessarily having to create a new image from scratch.

aside: the approach in that project's readme is not ideal, rm in a docker build will create a tombstone file, the image will still contain the binary from the COPY step unless --squash is used when building.

@BenTheElder
Copy link
Member

FWIW: I also think we can easily iterate on the implementation of the image as soon as we can agree on the existence of an image for this purpose. If we can get consensus on that I'm not super concerned with how it is implemented, it merely needs to provide the setcap binary in an image we can use for multi-stage build in k/k, that's a very simple contract to maintain.

@vinayakankugoyal
Copy link
Contributor Author

aside: the approach in that project's readme is not ideal, rm in a docker build will create a tombstone file, the image will still contain the binary from the COPY step unless --squash is used when building.

Good catch, although we would still be able to do, since we use buildx.

FROM docker-caps
COPY binary /binary
setcap /binary 

FROM actualbase
COPY --from=0 /binary /binary

@vinayakankugoyal
Copy link
Contributor Author

I personally like the idea of the having a separate image as its easy to reason about its purpose. Adding it to go-runner and kube-cross only increases their size unnecessarily as you mentioned.

saschagrunert
saschagrunert reviewed Feb 12, 2021
View reviewed changes
images/build/setcap/Makefile
export DOCKER_CLI_EXPERIMENTAL := enabled

build:
ifneq ($(ARCH),amd64)
Copy link
Member

Choose a reason for hiding this comment

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

Let's do not silently assume that we're running on amd64.

Suggested change
ifneq ($(ARCH),amd64)

Copy link
Member

@BenTheElder BenTheElder Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Do we build these images on another arch?
NOTE: this is from multiple other makefiles in this repo:

release/images/build/debian-iptables/Makefile

Line 45 in aab9759

ifneq ($(ARCH),amd64)

release/images/build/debian-hyperkube-base/Makefile

Line 75 in aab9759

ifneq ($(ARCH),amd64)

release/images/build/debian-base/Makefile

Line 115 in aab9759

ifneq ($(ARCH),amd64)

IMO if we consider this a problem this needs a cleanup orthogonal to this PR and the discussion around what way to use setcap

Copy link
Member

Choose a reason for hiding this comment

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

Do we build these images on another arch?

Not in the CI, but when developing those images locally different machines using different architectures would result in different outputs.

IMO if we consider this a problem this needs a cleanup orthogonal to this PR and the discussion around what way to use setcap

Right now I'm working on cleaning up the debian-base image in #1909. Sure, I'm happy to follow-up on that later on.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough! thanks :-)

@BenTheElder
Copy link
Member

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2021
@BenTheElder
Copy link
Member

/hold
for further discussion.
however I think the current approach is a very reasonable starting point.
regarding local cross build issues: we should follow-up with a cleanup around permissions/umask.

@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 Feb 12, 2021
@vinayakankugoyal vinayakankugoyal mentioned this pull request Feb 12, 2021
Merged
saschagrunert
saschagrunert approved these changes Feb 12, 2021
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

/lgtm

Code looks good from my point of view. I'm not sure if @justaugustus wants to chime in, I don't have the full context of the origin of this image. Feel free to lift the hold if we assume everyone is happy. 🙂

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, dekkagaijin, saschagrunert, vinayakankugoyal

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

@saschagrunert
Copy link
Member

saschagrunert commented Feb 12, 2021 •
edited
Loading

@vinayakankugoyal can you please edit the PR description to mention a release-note that we added this image?

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 12, 2021
@vinayakankugoyal
Copy link
Contributor Author

Forgot to add that I tested it with cloud build and that works.:

gcloud builds submit --config ./images/build/setcap/cloudbuild.yaml . --substitutions=_CONFIG="buster",_IMAGE_VERSION="buster-v1.4.0",_DEBIAN_BASE_VERSION="buster-v1.4.0"

Then used the image in a multistage docker build and verified the capabilities. (binary name and project location have been redacted)

FROM gcr.io/<redacted>/setcap:buster-v1.4.0
COPY BINARY /BINARY
RUN setcap cap_net_bind_service=+ep /BINARY

FROM ubuntu
COPY --from=0 /BINARY /usr/local/bin/BINARY
RUN apt-get update && apt-get -y --no-install-recommends install libcap2-bin

docker buildx build -t setcap-test . 

docker run --rm setcap-test:latest getcap /BINARY
/BINARY = cap_net_bind_service+ep

@BenTheElder
Copy link
Member

Stephen is OOO for about a week (until the 17th), I'm inclined to move forward for now, having this is a net improvement over alternative approaches in k/k, we can start iterating on the build logic xref #1909
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 12, 2021
@k8s-ci-robot k8s-ci-robot merged commit d32b2fc into kubernetes:master Feb 12, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Feb 12, 2021
This was referenced Feb 12, 2021
Merged
Merged
Merged
@justaugustus justaugustus mentioned this pull request Apr 9, 2021
Merged
@vinayakankugoyal vinayakankugoyal mentioned this pull request Jun 3, 2021
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guirish guirish guirish left review comments

@liggitt liggitt liggitt left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@dims dims dims left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@dougm dougm Awaiting requested review from dougm

@tpepper tpepper Awaiting requested review from tpepper

@justaugustus justaugustus Awaiting requested review from justaugustus

@dekkagaijin dekkagaijin Awaiting requested review from dekkagaijin

Assignees

@saschagrunert saschagrunert

@dekkagaijin dekkagaijin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/release Categorizes an issue or PR as relevant to SIG Release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.21
Development

Successfully merging this pull request may close these issues.
10 participants
@vinayakankugoyal @k8s-ci-robot @saschagrunert @liggitt @lasomethingsomething @BenTheElder @justaugustus @dekkagaijin @dims @guirish