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

Make etcd world-executable in Docker image #79722

Merged
merged 5 commits into from
Aug 12, 2019
Merged

Make etcd world-executable in Docker image #79722

merged 5 commits into from
Aug 12, 2019

Conversation

randomvariable
Copy link
Member

What type of PR is this?
/kind bug
/area security
/sig release
/sig cluster-lifecycle

What this PR does / why we need it:
Makes the etcd binaries in the Docker image world-executable, which allows consumers to drop running the image as root.

Which issue(s) this PR fixes:

Fixes #79720

Special notes for your reviewer:
Have added fixes to the Makefile to run on SELinux enabled systems.

Does this PR introduce a user-facing change?:

etcd Docker image can be run as non-root

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. area/security sig/release Categorizes an issue or PR as relevant to SIG Release. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 3, 2019
@k8s-ci-robot k8s-ci-robot requested review from jpbetz and wenjiaswe July 3, 2019 12:33
@k8s-ci-robot k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 3, 2019
@randomvariable
Copy link
Member Author

/assign @jpbetz

dims
dims reviewed Jul 3, 2019
View reviewed changes
etcd: Ensure etcd binaries are world executable
42f566a 
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@randomvariable randomvariable force-pushed the etcd-world-executable branch from fd124e0 to b3f2902 Compare July 3, 2019 15:49
@randomvariable randomvariable mentioned this pull request Jul 3, 2019
Closed
etcd: Allow Makefile to be used on SELinux systems
3783aa5 
Adds check for SELinux and then adds the :z parameter to the volume
mounts in order to work on SELinux enabled systems such as Fedora.

Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@randomvariable randomvariable force-pushed the etcd-world-executable branch from dc48576 to 3783aa5 Compare July 3, 2019 22:28
BenTheElder
BenTheElder reviewed Jul 3, 2019
View reviewed changes
cluster/images/etcd/Dockerfile Outdated
@@ -16,4 +16,6 @@ FROM BASEIMAGE

EXPOSE 2379 2380 4001 7001
COPY etcd* etcdctl* /usr/local/bin/
RUN chmod +x /usr/local/bin/etcd* /usr/local/bin/etcdctl*
Copy link
Member

Choose a reason for hiding this comment

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

one more minor suggestion: isn't the executable bit preserved by COPY? not using RUN makes cross building simpler, I wonder if we can just ensure +x on the host before copying.

Copy link
Member Author

@randomvariable randomvariable Jul 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Moved to using install in Makefile.

Note that chmod is used in /cluster/images/etcd-empty-dir-cleanup/Dockerfile which was not modified in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

SGTM :-)

Naadir Jeewa added 2 commits July 4, 2019 10:05
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 4, 2019
etcd: Add comment re: SELinux
b1b7ea3 
Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@dims
Copy link
Member

dims commented Jul 8, 2019

/uncc

@yliaog
Copy link
Contributor

yliaog commented Jul 8, 2019

/assign @wenjiaswe

@neolit123
Copy link
Member

/retest

@wenjiaswe
Copy link
Contributor

/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 Jul 9, 2019
@wenjiaswe
Copy link
Contributor

Defer to @jpbetz for approval.

@wenjiaswe
Copy link
Contributor

/assign @jpbetz

@yastij
Copy link
Member

yastij commented Jul 12, 2019

/priority important-soon

@k8s-ci-robot 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 Jul 12, 2019
@dims
Copy link
Member

dims commented Jul 12, 2019

/milestone v1.16

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Jul 12, 2019
@justaugustus
Copy link
Member

/lgtm
/approve

@neolit123
Copy link
Member

bump
@jpbetz, PTAL for approval on this change when you have the time.

@jpbetz
Copy link
Contributor

jpbetz commented Aug 12, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpbetz, justaugustus, randomvariable, wenjiaswe

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 Aug 12, 2019
@k8s-ci-robot k8s-ci-robot merged commit 133f378 into kubernetes:master Aug 12, 2019
@randomvariable randomvariable deleted the etcd-world-executable branch August 12, 2019 22:28
@randomvariable randomvariable mentioned this pull request Dec 6, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dims dims dims left review comments

@BenTheElder BenTheElder BenTheElder left review comments

@jpbetz jpbetz Awaiting requested review from jpbetz

@wenjiaswe wenjiaswe Awaiting requested review from wenjiaswe

Assignees

@jpbetz jpbetz

@justaugustus justaugustus

@wenjiaswe wenjiaswe

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/security cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.16
Development

Successfully merging this pull request may close these issues.

etcd image should be world executable to allow running as non-root
10 participants
@randomvariable @dims @yliaog @neolit123 @wenjiaswe @yastij @justaugustus @jpbetz @k8s-ci-robot @BenTheElder