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

Setup user on docker image to run it as no root #983

Merged
merged 1 commit into from
Sep 19, 2019

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Sep 10, 2019

What:

  • Setup user on docker image to run it as rootless

Motivation:

Steps performed to check it locally

$ cd upimage/
$ kubebuilder init --domain up.image
$ kubebuilder create api --group kubeimage --version v1 --kind TestImg
$ make
$ oc login -u system:admin
$ make install
$ make docker-build docker-push IMG=cmacedo/upimage:test
$ make deploy IMG=cmacedo/upimage:test

Checked that it worked in Minishift/OCP as well.

Screenshot 2019-09-17 at 17 03 57

Screenshot 2019-09-17 at 17 03 40

Screenshot 2019-09-17 at 17 03 34

Screenshot 2019-09-17 at 17 03 27

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 10, 2019
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

there seem to be a bunch of unrelated changes, plus some shell scripts, which definitely won't work in the static image. Can you clarify a bit what's going on here?

pkg/scaffold/v2/dockerfile.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the change-image branch 4 times, most recently from 5c913bf to 6313e5e Compare September 11, 2019 07:54
@camilamacedo86 camilamacedo86 changed the title WIP: setup user on docker image to run it as no root Setup user on docker image to run it as no root Sep 11, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2019
@camilamacedo86

This comment has been minimized.

@mengqiy
Copy link
Member

mengqiy commented Sep 16, 2019

I do not know why it is not passing in 1 of the tests. See here.

The change probably breaks the v2 e2e test.

@DirectXMan12
Copy link
Contributor

yep, that's my suspicion as well. this needs to be overhauled re: my comment before we move forward

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 17, 2019
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Sep 17, 2019

Hi @DirectXMan12 and @mengqiy,

The error faced in the test is when it tries to delete the certificates.certmanager.k8s.io and show that it has been not found. However, has NOT any error in the step where it is applied and I cannot see what in this change could affect it. See here that the error in the uninstalling cert manager bundle in this line of code.

Also, if I understood properly this file shows inject by the webhooks and it is not added in the scaffold project by default.

Could you give me a hand to understand why the change here which is very small now still breaking the 1 of the tests?

@mengqiy
Copy link
Member

mengqiy commented Sep 17, 2019

kubectl -n e2e-xdvj-system apply -f config/samples/barxdvj_v1alpha1_fooxdvj.yaml failed with error: Error from server (InternalError): error when creating "config/samples/barxdvj_v1alpha1_fooxdvj.yaml": Internal error occurred: failed calling webhook "mfooxdvj.kb.io": Post https://e2e-xdvj-webhook-service.e2e-xdvj-system.svc:443/mutate-barxdvj-example-comxdvj-v1alpha1-fooxdvj?timeout=30s: dial tcp 10.96.220.66:443: connect: connection refused

It seems apiserver can't talk to the webhook. Let me take a closer look.

@mengqiy
Copy link
Member

mengqiy commented Sep 17, 2019

2019-09-17T18:45:06.408Z ERROR setup problem running manager {"error": "listen tcp :443: bind: permission denied"}

After looking at the container log, it seems it no longer have permissions to bind to 443, which is the default port for webhook.

@mengqiy
Copy link
Member

mengqiy commented Sep 17, 2019

To fix this issue, you will need to change the scaffolding of

  • main.go: set Port in manager.Options.
  • config/default/manager_webhook_patch.yaml: change containerPort.
  • config/webhook/service.yaml: change targetPort.

@DirectXMan12
Copy link
Contributor

ah, yeah. non-root and privileged ports and all that.

@camilamacedo86 camilamacedo86 force-pushed the change-image branch 2 times, most recently from 46fe044 to 539cbc9 Compare September 17, 2019 22:10
@k8s-ci-robot k8s-ci-robot removed the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 17, 2019
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

Please don't change anything related to kubebuilder v1.
e.g. testdata/gopath/project-v1 and pkg/scaffold/v1

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 18, 2019
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Sep 18, 2019

Hi @mengqiy,

Really tks for your help with. It is done as you suggested but the same issue still been faced. See here. It still trying to connect in the 433 port which is not allowed to the user.

pkg/scaffold/v2/dockerfile.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/dockerfile.go Outdated Show resolved Hide resolved
pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

Please also update testdata/project-v2/main.go.
We have a test to ensure /testdata director is always up-to-date. But it seems it's broken now. We will fix that. @DirectXMan12
It should not block you.

Other pieces LGTM. Please squash commits.

pkg/scaffold/v2/main.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 force-pushed the change-image branch 2 times, most recently from d0c1899 to 9ddaa7f Compare September 19, 2019 00:07
…oless/static:nonroot and the targetPort 9843 for webhoocks
@camilamacedo86
Copy link
Member Author

Hi @mengqiy,

Shows that all requests are done. Really tks for the help, to tell how to solve the test issue and time spend in the review.

@mengqiy
Copy link
Member

mengqiy commented Sep 19, 2019

/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 Sep 19, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, mengqiy

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 Sep 19, 2019
@k8s-ci-robot k8s-ci-robot merged commit c7a28a2 into kubernetes-sigs:master Sep 19, 2019
@camilamacedo86 camilamacedo86 deleted the change-image branch January 11, 2021 19:28
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants