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

various gcsweb updates #11176

Merged
merged 2 commits into from
Feb 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions gcsweb/Dockerfile.in
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

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


RUN apk update --no-cache && apk add ca-certificates
ADD passwd /etc/passwd
USER nobody:nobody

ADD bin/ARG_ARCH/ARG_BIN /ARG_BIN
ADD icons /icons
ADD styles /styles
RUN chmod -R go+r /icons /styles

USER nobody:nobody
ENTRYPOINT ["/ARG_BIN"]
57 changes: 25 additions & 32 deletions gcsweb/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -25,36 +25,22 @@ REGISTRY ?= staging-k8s.gcr.io
ARCH ?= amd64

# The version string.
VERSION := v1.0.6
VERSION := v1.1.0

###
### These variables should not need tweaking.
###

SRC_DIRS := cmd pkg # directories which hold app source (not vendored)

# Other architectures are not supported because I don't know how to get CA
# certs for busybox.
# arm arm64 ppc64le
ALL_ARCH := amd64

# Set default base image dynamically for each arch
ifeq ($(ARCH),amd64)
BASEIMAGE?=alpine
endif
ifeq ($(ARCH),arm)
BASEIMAGE?=armel/busybox
endif
ifeq ($(ARCH),arm64)
BASEIMAGE?=aarch64/busybox
endif
ifeq ($(ARCH),ppc64le)
BASEIMAGE?=ppc64le/busybox
endif

IMAGE := $(REGISTRY)/$(BIN)-$(ARCH)

BUILD_IMAGE ?= golang:1.9.3-alpine
ALL_ARCH := amd64 arm arm64 ppc64le s390x

IMAGE := $(REGISTRY)/$(BIN)

BUILD_IMAGE ?= golang:1.11.5

# This option is for running docker manifest command
export DOCKER_CLI_EXPERIMENTAL := enabled

# If you want to build all binaries, see the 'all-build' rule.
# If you want to build all containers, see the 'all-container' rule.
Expand All @@ -74,7 +60,7 @@ all-build: $(addprefix build-, $(ALL_ARCH))

all-container: $(addprefix container-, $(ALL_ARCH))

all-push: $(addprefix push-, $(ALL_ARCH))
all-push-images: $(addprefix push-, $(ALL_ARCH))

build: bin/$(ARCH)/$(BIN)

Expand All @@ -97,28 +83,28 @@ bin/$(ARCH)/$(BIN): build-dirs
./build/build.sh \
"

DOTFILE_IMAGE = $(subst /,_,$(IMAGE))-$(VERSION)
DOTFILE_IMAGE = $(subst /,_,$(IMAGE))-$(ARCH)-$(VERSION)

container: .container-$(DOTFILE_IMAGE) container-name
.container-$(DOTFILE_IMAGE): bin/$(ARCH)/$(BIN) Dockerfile.in
@sed \
-e 's|ARG_BIN|$(BIN)|g' \
-e 's|ARG_ARCH|$(ARCH)|g' \
-e 's|ARG_FROM|$(BASEIMAGE)|g' \
Dockerfile.in > .dockerfile-$(ARCH)
@docker build -t $(IMAGE):$(VERSION) -f .dockerfile-$(ARCH) .
@docker images -q $(IMAGE):$(VERSION) > $@
@chmod -R go+r icons styles
@docker build --pull -t $(IMAGE)-$(ARCH):$(VERSION) -f .dockerfile-$(ARCH) .
@docker images -q $(IMAGE)-$(ARCH):$(VERSION) > $@

container-name:
@echo "container: $(IMAGE):$(VERSION)"
@echo "container: $(IMAGE)-$(ARCH):$(VERSION)"

push: .push-$(DOTFILE_IMAGE) push-name
.push-$(DOTFILE_IMAGE): .container-$(DOTFILE_IMAGE)
@docker push $(IMAGE):$(VERSION)
@docker images -q $(IMAGE):$(VERSION) > $@
@docker push $(IMAGE)-$(ARCH):$(VERSION)
@docker images -q $(IMAGE)-$(ARCH):$(VERSION) > $@

push-name:
@echo "pushed: $(IMAGE):$(VERSION)"
@echo "pushed: $(IMAGE)-$(ARCH):$(VERSION)"

version:
@echo $(VERSION)
Expand Down Expand Up @@ -148,3 +134,10 @@ container-clean:

bin-clean:
rm -rf .go bin

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

push-manifest:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

docker manifest create --amend $(IMAGE):$(VERSION) $(shell echo $(ALL_ARCH) | sed -e "s~[^ ]*~$(IMAGE)\-&:$(VERSION)~g")
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@for arch in $(ALL_ARCH); do docker manifest annotate --arch $${arch} $(IMAGE):$(VERSION) $(IMAGE)-$${arch}:$(VERSION); done
docker manifest push --purge $(IMAGE):$(VERSION)
2 changes: 1 addition & 1 deletion gcsweb/build/build.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


# Copyright 2016 The Kubernetes Authors.
#
Expand Down
2 changes: 1 addition & 1 deletion gcsweb/build/test.sh
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#!/bin/sh
#!/usr/bin/env bash

# Copyright 2016 The Kubernetes Authors.
#
Expand Down
39 changes: 35 additions & 4 deletions gcsweb/cmd/gcsweb/gcsweb.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ var flPort = flag.Int("p", 8080, "port number on which to listen")
var flIcons = flag.String("i", "/icons", "path to the icons directory")
var flStyles = flag.String("s", "/styles", "path to the styles directory")
var flVersion = flag.Bool("version", false, "print version and exit")
var flUpgradeProxiedHTTPtoHTTPS = flag.Bool("upgrade-proxied-http-to-https", false,
"upgrade any proxied request (e.g. from GCLB) from http to https")

const (
iconFile = "/icons/file.png"
Expand Down Expand Up @@ -86,7 +88,7 @@ func main() {
log.Printf("allowing %s", bucket)
http.HandleFunc(bucket+"/", gcsRequest)
http.HandleFunc(bucket, func(w http.ResponseWriter, r *http.Request) {
http.Redirect(w, r, bucket+"/", http.StatusMovedPermanently)
http.Redirect(w, r, bucket+"/", http.StatusPermanentRedirect)
})
}
// Handle unknown buckets.
Expand All @@ -95,6 +97,9 @@ func main() {
// Serve icons and styles.
longCacheServer := func(h http.Handler) http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
if upgradeToHTTPS(w, r, newTxnLogger(r)) {
return
}
// Mark as never expiring as per https://www.ietf.org/rfc/rfc2616.txt
w.Header().Add("Cache-Control", "max-age=31536000")
h.ServeHTTP(w, r)
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

wow, no kidding.

// 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.)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that one's mine :+)

if *flUpgradeProxiedHTTPtoHTTPS && r.Header.Get("X-Forwarded-Proto") == "http" {
newURL := *r.URL
newURL.Scheme = "https"
if newURL.Host == "" {
newURL.Host = r.Host
}
logger.Printf("redirect to %s [https upgrade]", newURL.String())
http.Redirect(w, r, newURL.String(), http.StatusPermanentRedirect)
return true
}
return false
}

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

Expand All @@ -123,8 +142,11 @@ func healthzRequest(w http.ResponseWriter, r *http.Request) {
}

func robotsRequest(w http.ResponseWriter, r *http.Request) {
newTxnLogger(r)
logger := newTxnLogger(r)

if upgradeToHTTPS(w, r, logger) {
return
}
if r.Method != "GET" {
w.WriteHeader(http.StatusMethodNotAllowed)
return
Expand All @@ -134,8 +156,11 @@ func robotsRequest(w http.ResponseWriter, r *http.Request) {
}

func unknownBucketRequest(w http.ResponseWriter, r *http.Request) {
newTxnLogger(r)
logger := newTxnLogger(r)

if upgradeToHTTPS(w, r, logger) {
return
}
if r.Method != "GET" {
w.WriteHeader(http.StatusMethodNotAllowed)
return
Expand All @@ -154,13 +179,19 @@ func unknownBucketRequest(w http.ResponseWriter, r *http.Request) {
}

func otherRequest(w http.ResponseWriter, r *http.Request) {
newTxnLogger(r)
logger := newTxnLogger(r)
if upgradeToHTTPS(w, r, logger) {
return
}
http.NotFound(w, r)
}

func gcsRequest(w http.ResponseWriter, r *http.Request) {
logger := newTxnLogger(r)

if upgradeToHTTPS(w, r, logger) {
return
}
if r.Method != "GET" {
w.WriteHeader(http.StatusMethodNotAllowed)
return
Expand Down
1 change: 1 addition & 0 deletions gcsweb/passwd
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
nobody:x:65534:65534:nobody:/nonexistent:/usr/sbin/nologin
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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).