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

fix: replace deprecated bouk/staticfiles with native Go embed. fixes #11654 #11707

Merged
merged 1 commit into from
Dec 6, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ci-build.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ jobs:
go-version: "1.23"
cache: true
# windows run does not use makefile target because it does a lot more than just testing and is not cross-platform compatible
- run: go test -p 20 -covermode=atomic -coverprofile='coverage.out' $(go list ./... | select-string -Pattern 'github.com/argoproj/argo-workflows/v3/workflow/controller' , 'github.com/argoproj/argo-workflows/v3/server' -NotMatch)
- run: if (!(Test-Path "ui/dist/app/index.html")) { New-Item -ItemType Directory -Force -Path "ui/dist/app" | Out-Null; New-Item -ItemType File -Path "ui/dist/app/placeholder" | Out-Null }; go test -p 20 -covermode=atomic -coverprofile='coverage.out' $(go list ./... | select-string -Pattern 'github.com/argoproj/argo-workflows/v3/workflow/controller' , 'github.com/argoproj/argo-workflows/v3/server' -NotMatch)
env:
KUBECONFIG: /dev/null
- name: Upload coverage report
Expand Down
2 changes: 0 additions & 2 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -65,5 +65,3 @@ issues:
- sdks
- ui
- vendor
exclude-files:
- server/static/files.go
33 changes: 11 additions & 22 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ endif
HACK_PKG_FILES_AS_PKGS ?= false
ifeq ($(HACK_PKG_FILES_AS_PKGS),false)
ARGOEXEC_PKG_FILES := $(shell go list -f '{{ join .Deps "\n" }}' ./cmd/argoexec/ | grep 'argoproj/argo-workflows/v3/' | xargs go list -f '{{ range $$file := .GoFiles }}{{ print $$.ImportPath "/" $$file "\n" }}{{ end }}' | cut -c 39-)
CLI_PKG_FILES := $(shell go list -f '{{ join .Deps "\n" }}' ./cmd/argo/ | grep 'argoproj/argo-workflows/v3/' | xargs go list -f '{{ range $$file := .GoFiles }}{{ print $$.ImportPath "/" $$file "\n" }}{{ end }}' | cut -c 39-)
CLI_PKG_FILES := $(shell [ -f ui/dist/app/index.html ] || (mkdir -p ui/dist/app && touch ui/dist/app/placeholder); go list -f '{{ join .Deps "\n" }}' ./cmd/argo/ | grep 'argoproj/argo-workflows/v3/' | xargs go list -f '{{ range $$file := .GoFiles }}{{ print $$.ImportPath "/" $$file "\n" }}{{ end }}' | cut -c 39-)
sonbui00 marked this conversation as resolved.
Show resolved Hide resolved
CONTROLLER_PKG_FILES := $(shell go list -f '{{ join .Deps "\n" }}' ./cmd/workflow-controller/ | grep 'argoproj/argo-workflows/v3/' | xargs go list -f '{{ range $$file := .GoFiles }}{{ print $$.ImportPath "/" $$file "\n" }}{{ end }}' | cut -c 39-)
else
# Building argoexec on windows cannot rebuild the openapi, we need to fall back to the old
Expand Down Expand Up @@ -165,25 +165,14 @@ endef
cli: dist/argo

ui/dist/app/index.html: $(shell find ui/src -type f && find ui -maxdepth 1 -type f)
ifeq ($(STATIC_FILES),true)
# `yarn install` is fast (~2s), so you can call it safely.
JOBS=max yarn --cwd ui install
# `yarn build` is slow, so we guard it with a up-to-date check.
JOBS=max yarn --cwd ui build

$(GOPATH)/bin/staticfiles: Makefile
# update this in Nix when updating it here
ifneq ($(USE_NIX), true)
go install bou.ke/staticfiles@dd04075
endif

ifeq ($(STATIC_FILES),true)
server/static/files.go: $(GOPATH)/bin/staticfiles ui/dist/app/index.html
# Pack UI into a Go file
$(GOPATH)/bin/staticfiles -o server/static/files.go ui/dist/app
else
server/static/files.go:
# Building without static files
cp ./server/static/files.go.stub ./server/static/files.go
@mkdir -p ui/dist/app
touch ui/dist/app/index.html
endif

dist/argo-linux-amd64: GOARGS = GOOS=linux GOARCH=amd64
Expand All @@ -198,16 +187,16 @@ dist/argo-windows-amd64: GOARGS = GOOS=windows GOARCH=amd64
dist/argo-windows-%.gz: dist/argo-windows-%
gzip --force --keep dist/argo-windows-$*.exe

dist/argo-windows-%: server/static/files.go $(CLI_PKG_FILES) go.sum
dist/argo-windows-%: ui/dist/app/index.html $(CLI_PKG_FILES) go.sum
CGO_ENABLED=0 $(GOARGS) go build -v -gcflags '${GCFLAGS}' -ldflags '${LDFLAGS} -extldflags -static' -o $@.exe ./cmd/argo

dist/argo-%.gz: dist/argo-%
gzip --force --keep dist/argo-$*

dist/argo-%: server/static/files.go $(CLI_PKG_FILES) go.sum
dist/argo-%: ui/dist/app/index.html $(CLI_PKG_FILES) go.sum
CGO_ENABLED=0 $(GOARGS) go build -v -gcflags '${GCFLAGS}' -ldflags '${LDFLAGS} -extldflags -static' -o $@ ./cmd/argo

dist/argo: server/static/files.go $(CLI_PKG_FILES) go.sum
dist/argo: ui/dist/app/index.html $(CLI_PKG_FILES) go.sum
ifeq ($(shell uname -s),Darwin)
# if local, then build fast: use CGO and dynamic-linking
go build -v -gcflags '${GCFLAGS}' -ldflags '${LDFLAGS}' -o $@ ./cmd/argo
Expand Down Expand Up @@ -454,7 +443,7 @@ $(GOPATH)/bin/golangci-lint: Makefile
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b `go env GOPATH`/bin v1.61.0

.PHONY: lint
lint: server/static/files.go $(GOPATH)/bin/golangci-lint
lint: ui/dist/app/index.html $(GOPATH)/bin/golangci-lint
rm -Rf v3 vendor
# If you're using `woc.wf.Spec` or `woc.execWf.Status` your code probably won't work with WorkflowTemplate.
# * Change `woc.wf.Spec` to `woc.execWf.Spec`.
Expand All @@ -471,7 +460,7 @@ lint: server/static/files.go $(GOPATH)/bin/golangci-lint

# for local we have a faster target that prints to stdout, does not use json, and can cache because it has no coverage
.PHONY: test
test: server/static/files.go
test: ui/dist/app/index.html
go build ./...
env KUBECONFIG=/dev/null $(GOTEST) ./...
# marker file, based on it's modification time, we know how long ago this target was run
Expand Down Expand Up @@ -702,11 +691,11 @@ go-diagrams/diagram.dot: ./hack/docs/diagram.go
docs/assets/diagram.png: go-diagrams/diagram.dot
cd go-diagrams && dot -Tpng diagram.dot -o ../docs/assets/diagram.png

docs/fields.md: api/openapi-spec/swagger.json $(shell find examples -type f) hack/docs/fields.go
docs/fields.md: api/openapi-spec/swagger.json $(shell find examples -type f) ui/dist/app/index.html hack/docs/fields.go
env ARGO_SECURE=false ARGO_INSECURE_SKIP_VERIFY=false ARGO_SERVER= ARGO_INSTANCEID= go run ./hack/docs fields

# generates several other files
docs/cli/argo.md: $(CLI_PKG_FILES) go.sum server/static/files.go hack/docs/cli.go
docs/cli/argo.md: $(CLI_PKG_FILES) go.sum ui/dist/app/index.html hack/docs/cli.go
go run ./hack/docs cli

# docs
Expand Down
1 change: 0 additions & 1 deletion dev/nix/conf.nix
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
# Even then the buildFlags are not passed into Go, meaning you won't see the correct version info yet.
# This is only intended for quick developing at the moment, gradually more functionality will be pushed here.
rec {
staticFiles = false; # not acted upon
version = "latest";
env = {
DEFAULT_REQUEUE_TIME = "1s";
Expand Down
13 changes: 0 additions & 13 deletions dev/nix/flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -307,17 +307,6 @@
doCheck = false;
};

staticfiles = pkgs.buildGoPackage rec {
name = "staticfiles";
src = pkgs.fetchFromGitHub {
owner = "bouk";
repo = "staticfiles";
rev = "827d7f6389cd410d0aa3f3d472a4838557bf53dd";
sha256 = "0xarhmsqypl8036w96ssdzjv3k098p2d4mkmw5f6hkp1m3j67j61";
};

goPackagePath = "bou.ke/staticfiles";
};
default = config.packages.${package.name};
};

Expand All @@ -338,7 +327,6 @@
config.packages.k8sio-tools
config.packages.goreman
config.packages.stern
config.packages.staticfiles
config.packages.${package.name}
nodePackages.shell.nodeDependencies
gopls
Expand Down Expand Up @@ -368,7 +356,6 @@
config.packages.k8sio-tools
config.packages.goreman
config.packages.stern
config.packages.staticfiles
config.packages.${package.name}
nodePackages.shell.nodeDependencies
gopls
Expand Down
3 changes: 2 additions & 1 deletion server/apiserver/argoserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import (
"github.com/argoproj/argo-workflows/v3/server/workflow/store"
"github.com/argoproj/argo-workflows/v3/server/workflowarchive"
"github.com/argoproj/argo-workflows/v3/server/workflowtemplate"
"github.com/argoproj/argo-workflows/v3/ui"
grpcutil "github.com/argoproj/argo-workflows/v3/util/grpc"
"github.com/argoproj/argo-workflows/v3/util/instanceid"
"github.com/argoproj/argo-workflows/v3/util/json"
Expand Down Expand Up @@ -435,7 +436,7 @@ func (as *argoServer) newHTTPServer(ctx context.Context, port int, artifactServe

})
// we only enable HTST if we are secure mode, otherwise you would never be able access the UI
mux.HandleFunc("/", static.NewFilesServer(as.baseHRef, as.tlsConfig != nil && as.hsts, as.xframeOptions, as.accessControlAllowOrigin).ServerFiles)
mux.HandleFunc("/", static.NewFilesServer(as.baseHRef, as.tlsConfig != nil && as.hsts, as.xframeOptions, as.accessControlAllowOrigin, ui.Embedded).ServerFiles)
return &httpServer
}

Expand Down
8 changes: 0 additions & 8 deletions server/static/files.go

This file was deleted.

8 changes: 0 additions & 8 deletions server/static/files.go.stub

This file was deleted.

20 changes: 0 additions & 20 deletions server/static/response-rewriter.go

This file was deleted.

72 changes: 58 additions & 14 deletions server/static/static.go
Original file line number Diff line number Diff line change
@@ -1,33 +1,34 @@
package static

import (
"bytes"
"embed"
"fmt"
"io/fs"
"net/http"
"regexp"
"strings"
"time"

"github.com/argoproj/argo-workflows/v3"
"github.com/argoproj/argo-workflows/v3/ui"
)

type FilesServer struct {
baseHRef string
hsts bool
xframeOpts string
corsAllowOrigin string
staticAssets embed.FS
}

func NewFilesServer(baseHRef string, hsts bool, xframeOpts string, corsAllowOrigin string) *FilesServer {
return &FilesServer{baseHRef, hsts, xframeOpts, corsAllowOrigin}
var baseHRefRegex = regexp.MustCompile(`<base href="(.*?)">`)

func NewFilesServer(baseHRef string, hsts bool, xframeOpts string, corsAllowOrigin string, staticAssets embed.FS) *FilesServer {
return &FilesServer{baseHRef, hsts, xframeOpts, corsAllowOrigin, staticAssets}
}

func (s *FilesServer) ServerFiles(w http.ResponseWriter, r *http.Request) {
// If there is no stored static file, we'll redirect to the js app
if Hash(strings.TrimLeft(r.URL.Path, "/")) == "" {
r.URL.Path = "index.html"
}

if r.URL.Path == "index.html" {
// hack to prevent ServerHTTP from giving us gzipped content which we can do our search-and-replace on
r.Header.Del("Accept-Encoding")
w = &responseRewriter{ResponseWriter: w, old: []byte(`<base href="/">`), new: []byte(fmt.Sprintf(`<base href="%s">`, s.baseHRef))}
}

if s.xframeOpts != "" {
w.Header().Set("X-Frame-Options", s.xframeOpts)
Expand All @@ -50,6 +51,49 @@ func (s *FilesServer) ServerFiles(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Strict-Transport-Security", "max-age=31536000")
}

// in my IDE (IntelliJ) the next line is red for some reason - but this is fine
ServeHTTP(w, r)
if r.URL.Path == "/" || !s.uiAssetExists(r.URL.Path) {
data, err := s.getIndexData()
if err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
return
}
agilgur5 marked this conversation as resolved.
Show resolved Hide resolved

modTime, err := time.Parse(argo.GetVersion().BuildDate, time.RFC3339)
if err != nil {
modTime = time.Now()
}
http.ServeContent(w, r, "index.html", modTime, bytes.NewReader(data))
} else {
staticFS, _ := fs.Sub(s.staticAssets, ui.EMBED_PATH)
http.FileServer(http.FS(staticFS)).ServeHTTP(w, r)
}
}

func (s *FilesServer) getIndexData() ([]byte, error) {
data, err := s.staticAssets.ReadFile(ui.EMBED_PATH + "/index.html")
if err != nil {
return data, err
}
if s.baseHRef != "/" && s.baseHRef != "" {
data = []byte(replaceBaseHRef(string(data), fmt.Sprintf(`<base href="/%s/">`, strings.Trim(s.baseHRef, "/"))))
}

return data, nil
}

func (s *FilesServer) uiAssetExists(filename string) bool {
f, err := s.staticAssets.Open(ui.EMBED_PATH + filename)
if err != nil {
return false
}
defer f.Close()
stat, err := f.Stat()
if err != nil {
return false
}
return !stat.IsDir()
}

func replaceBaseHRef(data string, replaceWith string) string {
return baseHRefRegex.ReplaceAllString(data, replaceWith)
}
95 changes: 95 additions & 0 deletions server/static/static_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package static

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestReplaceBaseHRef(t *testing.T) {
testCases := []struct {
name string
data string
expected string
replaceWith string
}{
{
name: "non-root basepath",
data: `<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Argo</title>
<base href="/">
<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="robots" content="noindex">
<link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="assets/favicon/favicon-16x16.png" sizes="16x16">
<script defer="defer" src="main.js"></script>
</head>
<body>
<div id="app"></div>
</body>
</html>`,
expected: `<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Argo</title>
<base href="/path1/path2/path3/">
<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="robots" content="noindex">
<link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="assets/favicon/favicon-16x16.png" sizes="16x16">
<script defer="defer" src="main.js"></script>
</head>
<body>
<div id="app"></div>
</body>
</html>`,
replaceWith: `<base href="/path1/path2/path3/">`,
},
{
name: "root basepath",
data: `<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Argo</title>
<base href="/any/path/test/">
<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="robots" content="noindex">
<link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="assets/favicon/favicon-16x16.png" sizes="16x16">
<script defer="defer" src="main.js"></script>
</head>
<body>
<div id="app"></div>
</body>
</html>`,
expected: `<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>Argo</title>
<base href="/">
<meta name="viewport" content="width=device-width,initial-scale=1">
<meta name="robots" content="noindex">
<link rel="icon" type="image/png" href="assets/favicon/favicon-32x32.png" sizes="32x32">
<link rel="icon" type="image/png" href="assets/favicon/favicon-16x16.png" sizes="16x16">
<script defer="defer" src="main.js"></script>
</head>
<body>
<div id="app"></div>
</body>
</html>`,
replaceWith: `<base href="/">`,
},
}
for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
result := replaceBaseHRef(testCase.data, testCase.replaceWith)
assert.Equal(t, testCase.expected, result)
})
}
}
Loading
Loading