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

Rebase GLBC on alpine:3.5 #384

Merged
merged 1 commit into from
Mar 8, 2017
Merged

Conversation

timstclair
Copy link

For kubernetes/kubernetes#40248

Rebasing on busybox greatly reduces the size of the image. More importantly, it greatly reduces the management burden of keeping the image up-to-date in as all the packages within are updated.

@csbell @nicksardo

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 6, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@timstclair timstclair mentioned this pull request Mar 6, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 46.418% when pulling 1b0a8c18f06b9df49ce4a94ff73e460653dd2909 on timstclair:busybox into a6e3822 on kubernetes:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.04%) to 46.4% when pulling 2215e3b on timstclair:busybox into a6e3822 on kubernetes:master.

@csbell
Copy link
Contributor

csbell commented Mar 7, 2017

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.


controllers/gce/Dockerfile, line 15 at r1 (raw file):

# limitations under the License.

FROM busybox

Should we leave a MAINTAINER here?


controllers/gce/README.md, line 343 at r1 (raw file):

$ kubectl exec -it glbc-6m6b6 -- wget -q -O- http://localhost:8081/delete-all-and-quit

Just curious why wget is preferred.


Comments from Reviewable

@gianrubio
Copy link
Contributor

gianrubio commented Mar 7, 2017

@timstclair why not use alpine instead busybox?

@nicksardo
Copy link
Contributor

@timstclair Slipped my mind - we need the ca certs because the GLBC is calling out to the GCP api.

@glerchundi
Copy link
Contributor

glerchundi commented Mar 7, 2017

@nicksardo How is this usually solved? always include ca-certs in each image or just mount from the host through hostPath? CoreOS for example when it comes to launching kube-proxy or any k8s component, it mounts the hostPath for the ca-certs using host machine as the unique source of truth.

@timstclair
Copy link
Author

Review status: 0 of 8 files reviewed at latest revision, 2 unresolved discussions.


controllers/gce/Dockerfile, line 15 at r1 (raw file):

Previously, csbell (Christian Bell) wrote…

Should we leave a MAINTAINER here?

No, the MAINTAINER line is deprecated. We're gradually removing it from all our images.


controllers/gce/README.md, line 343 at r1 (raw file):

Previously, csbell (Christian Bell) wrote…

Just curious why wget is preferred.

busybox (and alpine) ship with wget, but not curl.


Comments from Reviewable

@timstclair
Copy link
Author

Rebased on alpine & installed ca-certs. I think we should probably prefer alpine for this type of image anyway, since we get better CVE notifications. Since there are no binary dependencies, I don't anticipate any of the alpine issues we've run into in the past (e.g. DNS problems).

@glerchundi I think this approach is preferable to mounting the host ca-certificates, since it keeps the environment contained, and doesn't add any host dependencies.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 46.129% when pulling b2a1f4b2d0d5875f7cb4cd9e6c8b26bf1e46df24 on timstclair:busybox into a6e3822 on kubernetes:master.

@timstclair timstclair changed the title Rebase GLBC on busybox Rebase GLBC on alpine:3.5 Mar 7, 2017
@timstclair
Copy link
Author

Squashed & rebased.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 46.321% when pulling 1023056 on timstclair:busybox into e1d1445 on kubernetes:master.

@nicksardo
Copy link
Contributor

All unit tests, e2e ingress tests, and several manual tests (with and without pre-shared certs) were successful. Merging and generating 0.9.2 image.

@nicksardo
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2017
@nicksardo nicksardo merged commit 31eab38 into kubernetes:master Mar 8, 2017
jwhonce pushed a commit to jwhonce/kubernetes that referenced this pull request Mar 9, 2017
Automatic merge from submit-queue (batch tested with PRs 42734, 42745, 42758, 42814, 42694)

Bump glbc version to 0.9.2

Follow up to kubernetes/ingress-nginx#384
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants