various gcsweb updates #11176

ixdy · 2019-02-07T20:29:39Z

Add an option to upgrade connections to HTTPS (tested on http://gcsweb.ixdyland.net)
Build with go1.11.5
Rebase on gcr.io/distroless/static (instead of alpine)
Produce a manifest list for multiarch support

…duce manifest list

BenTheElder · 2019-02-07T22:37:09Z

gcsweb/cmd/gcsweb/gcsweb.go

@@ -111,6 +116,20 @@ func main() {
 	log.Fatal(http.ListenAndServe(fmt.Sprintf(":%d", *flPort), nil))
 }

+func upgradeToHTTPS(w http.ResponseWriter, r *http.Request, logger txnLogger) bool {


side note: we should consider putting this in a util or finding one, we have a very similar method in deck

[not blocking for this PR, but something down the line maybe...]

wow, no kidding.

test-infra/prow/cmd/deck/main.go

Lines 382 to 403 in c3f2665

// optionally inject http->https redirect handler when behind loadbalancer

if o.redirectHTTPTo != "" {

redirectMux := http.NewServeMux()

redirectMux.Handle("/", func(oldMux *http.ServeMux, host string) http.HandlerFunc {

return func(w http.ResponseWriter, r *http.Request) {

if r.Header.Get("x-forwarded-proto") == "http" {

redirectURL, err := url.Parse(r.URL.String())

if err != nil {

logrus.Errorf("Failed to parse URL: %s.", r.URL.String())

http.Error(w, "Failed to perform https redirect.", http.StatusInternalServerError)

return

}

redirectURL.Scheme = "https"

redirectURL.Host = host

http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)

} else {

oldMux.ServeHTTP(w, r)

}

}

}(mux, o.redirectHTTPTo))

mux = redirectMux

}

I'm kinda impressed how I wrote almost exactly the same thing. (The deck implementation is cleverer by intercepting before any of the handler funcs are run, though. I couldn't quite figure out how to do that.)

Thanks, that one's mine :+)

BenTheElder · 2019-02-07T22:39:04Z

gcsweb/Dockerfile.in

@@ -12,15 +12,15 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.

-FROM ARG_FROM
+FROM gcr.io/distroless/static

 MAINTAINER Tim Hockin <thockin@google.com>


unrelated but MAINTAINER has long been deprecated, we just dropped these from the rest of our images in favor of OWNERS

BenTheElder

/lgtm/approve

BenTheElder · 2019-02-07T22:43:04Z

/lgtm
/approve

k8s-ci-robot · 2019-02-07T22:43:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, ixdy

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:

~~OWNERS~~ [BenTheElder,ixdy]

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 · 2019-02-07T22:43:11Z

LGTM label has been added.

Git tree hash: 07f937942896b7711d0ead5f9559b1d4929c41d9

thockin

minor points, mostly.

One option for protocol upgrade would be to write a tiny sidecar container that listens on port X, does protocol upgrade if needed, otherwise proxies to localhost port Y. That way, anyone could just throw that into a pod and not change their apps.

thockin · 2019-02-07T23:32:34Z

gcsweb/Makefile

+
+all-push: all-push-images push-manifest
+
+push-manifest:


I was (for a while) keeping this in sync with https://github.com/thockin/go-build-template which did a lot of what you have done here, but just epsilon differently. Would be nice to re-converge so fixes can be synced over time.

ah, I'd forgotten about that. can update to address.

thockin · 2019-02-07T23:33:22Z

gcsweb/Makefile

+all-push: all-push-images push-manifest
+
+push-manifest:
+	docker manifest create --amend $(IMAGE):$(VERSION) $(shell echo $(ALL_ARCH) | sed -e "s~[^ ]*~$(IMAGE)\-&:$(VERSION)~g")


docker manifest has bugs in some versions, which produce invalid manifests. Except docker proper didn;t validate manifests until recently. But containerd does...

We've been using this flow in k/k for a while without issue, though we do have guards over there in a few places to ensure that you're using docker 18.06.0 or newer (where the bug was fixed).

Google workstations all have new enough docker that I'm not super worried about this.

thockin · 2019-02-07T23:34:50Z

gcsweb/build/build.sh

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/usr/bin/env bash


why? does sh not exist? I would only say "bash" if we need bash-isms.

The golang image links /bin/sh to dash, which doesn't support things like set -o pipefail (apparently).

thockin · 2019-02-07T23:36:55Z

gcsweb/passwd

@@ -0,0 +1 @@
+nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin


Why not just echo this in Dockerfile - one less file in git? :)

The distroless image has no echo command. Also, running commands inside Dockerfiles gets trickier with multiarch (which is why I moved the chmod to the Makefile).

thockin · 2019-02-07T23:52:18Z

Interesting - I don't see problems on git-sync. Will have to look..

…

On Thu, Feb 7, 2019 at 3:51 PM Jeff Grafton ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In gcsweb/build/build.sh <#11176 (comment)> : > @@ -1,4 +1,4 @@ -#!/bin/sh +#!/usr/bin/env bash The golang image links /bin/sh to dash, which doesn't support things like set -o pipefail (apparently). — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#11176 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVKqrN9Nb452NV5UM4YP7xJ_fPElmks5vLLvrgaJpZM4apXP3> .

ixdy · 2019-02-07T23:56:01Z

I was considering adding a separate nginx pod which did the upgrade, but I wasn't sure how to get the GCLB to forward to a different pod. I hadn't considered using an nginx container, though that also feels a little heavyweight (GCLB -> nginx -> gcsweb binary).

might be worth looking into, though this approach here also works, so... 🤷

ixdy · 2019-02-07T23:57:45Z

the golang:alpine images link /bin/sh to busybox, which apparently supports the bashisms. The non-alpine golang images are based on debian, I believe.

gcsweb: add option to upgrade connections to HTTPS

6527eb5

k8s-ci-robot assigned thockin Feb 7, 2019

k8s-ci-robot requested review from fejta and krzyzacy February 7, 2019 20:29

ixdy mentioned this pull request Feb 7, 2019

Put gcsweb behind ingress and install a TLS cert for it kubernetes/k8s.io#38

Closed

gcsweb: update to go1.11.5, base on gcr.io/distroless/static, and pro…

e29358a

…duce manifest list

ixdy force-pushed the gcsweb-updates branch from 9d45389 to e29358a Compare February 7, 2019 20:42

BenTheElder reviewed Feb 7, 2019

View reviewed changes

BenTheElder approved these changes Feb 7, 2019

View reviewed changes

k8s-ci-robot assigned BenTheElder Feb 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2019

k8s-ci-robot merged commit 7d1d63c into kubernetes:master Feb 7, 2019

ixdy mentioned this pull request Feb 7, 2019

Remove maintainer from gcsweb image #11180

Merged

thockin reviewed Feb 7, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various gcsweb updates #11176

various gcsweb updates #11176

ixdy commented Feb 7, 2019 •

edited

BenTheElder Feb 7, 2019

BenTheElder Feb 7, 2019

ixdy Feb 8, 2019

BenTheElder Feb 12, 2019

BenTheElder Feb 7, 2019

BenTheElder left a comment

BenTheElder commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019

thockin left a comment

thockin Feb 7, 2019

ixdy Feb 7, 2019

thockin Feb 7, 2019

ixdy Feb 7, 2019

thockin Feb 7, 2019

ixdy Feb 7, 2019

thockin Feb 7, 2019

ixdy Feb 7, 2019

thockin commented Feb 7, 2019 via email

ixdy commented Feb 7, 2019

ixdy commented Feb 7, 2019

	// optionally inject http->https redirect handler when behind loadbalancer
	if o.redirectHTTPTo != "" {
	redirectMux := http.NewServeMux()
	redirectMux.Handle("/", func(oldMux *http.ServeMux, host string) http.HandlerFunc {
	return func(w http.ResponseWriter, r *http.Request) {
	if r.Header.Get("x-forwarded-proto") == "http" {
	redirectURL, err := url.Parse(r.URL.String())
	if err != nil {
	logrus.Errorf("Failed to parse URL: %s.", r.URL.String())
	http.Error(w, "Failed to perform https redirect.", http.StatusInternalServerError)
	return
	}
	redirectURL.Scheme = "https"
	redirectURL.Host = host
	http.Redirect(w, r, redirectURL.String(), http.StatusMovedPermanently)
	} else {
	oldMux.ServeHTTP(w, r)
	}
	}
	}(mux, o.redirectHTTPTo))
	mux = redirectMux
	}

		@@ -0,0 +1 @@
		nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin


		all-push: all-push-images push-manifest

		push-manifest:

various gcsweb updates #11176

various gcsweb updates #11176

Conversation

ixdy commented Feb 7, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder left a comment

Choose a reason for hiding this comment

BenTheElder commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 7, 2019 via email

ixdy commented Feb 7, 2019

ixdy commented Feb 7, 2019

ixdy commented Feb 7, 2019 •

edited