feat(frontend): do not embed frontend assets anymore by burmudar · Pull Request #63946 · sourcegraph/sourcegraph-public-snapshot

burmudar · 2024-07-19T10:38:09Z

Frontend no longer embeds the assets intead it reads from the local filesystem assets.
Generally the frontend and server cmd targets will use the //client/web/dist:copy_bundle target to create a tarball for the oci_image. copy_bundle puts all the assets at assets-dist
For integration tests, frontend and server have the no_client_bundle target variants. For these oci_images, instead of the tar_bundle which is just a tar'd copy_bundle we use the tar_dummy_manifest which is just a tar that contains a dummy manifest.
By default we expect assets to be at /assets-dist
Renamed DevProvider to DirProvider

Why

By 'breaking' the dependency of frontend requiring assets to be built we essentially stop a common cache invalidation scenario that happens:

someone makes a frontend change = assets need to be rebuilt

By decoupling assets from the frontend binary and moving the packing of assets to the building of the frontend and server images we will have a better cache hit rate (theoretically).

Thus with this change, when:

client/web is change and nothing else ... only assets will have to rebuilt and cached versions of the backend will be used
if only backend code has changed ... cached assets will be used

Closes DINF-115

Test plan

✅ sg start - web app opens and can search. Local dev assets get loaded
✅ sg test bazel-integration-test - server image gets built with only dummy web manifest. Also verified by running sg bazel run //cmd/server:no_client_bundle.image and then inspect container
✅ sg test bazel-e2e - server image gets built with bundle and all tests pass
✅ main dry run

Changelog

frontend: assets are no longer bundled with binary through go:embed. Instead assets are now added to the frontend container at assets-dist.

github-actions · 2024-07-19T10:39:59Z

Caution

License checking failed, please read: how to deal with third parties licensing.

keegancsmith

It's a little unclear how this all fits together. I guess I just wanna check the follow properties are true:

in development we reload web.manifest.json if it changes. Otherwise its only read once.
in prod you can't trick the location of the asset dir. I think that is the case and http.FileSystem won't allow access outside of the passed in dir.

What I'm a little unclear is why we allow customizing the value of the asset dir outside of dev. It seems like when constructing docker images we can just always hardcode the location of the asset dir?

keegancsmith · 2024-07-25T10:39:54Z

Should we not just assert this always is set and that it is an absolute path?

I have added the absolute path test, but didn't change it to check that it is always set, since there are places where it isn't set and set via other means like https://sourcegraph.com/github.com/sourcegraph/sourcegraph@bc4acd1fbd278b8bb414241f286420dc8c69c047/-/blob/cmd/frontend/shared/service.go?L26-28

eseliger

someone makes a client/web change = frontend needs to be rebuilt
someone makes a frontend change = assets need to be rebuilt

I do understand the first one, but how does a cmd/frontend change cause assets needing to be rebuilt? 🤔

Left a few comments here about ramdisk before and non-portability of this, which are both things I could be convinced to find ok, but initially they feel a bit of a loss to me.

eseliger · 2024-07-25T11:37:33Z

I assume the http.Dir always reads from disk? Right now it is a ram-disk. Not sure if that has performance implications or scalability implications since the disk that holds rootFS in kubernetes clusters is often quite small -> low IOPS from cloud providers. 🤔

burmudar · 2024-07-30T13:10:35Z

It's a little unclear how this all fits together. I guess I just wanna check the follow properties are true:
* in development we reload web.manifest.json if it changes. Otherwise its only read once.

* in prod you can't trick the location of the asset dir. I think that is the case and http.FileSystem won't allow access outside of the passed in dir.
What I'm a little unclear is why we allow customizing the value of the asset dir outside of dev. It seems like when constructing docker images we can just always hardcode the location of the asset dir?

Yup
You can technically trick it with arbitrary symlinks but that requries access to the filesystem already - according to Dir.Open. Otherwise it only serves from the passed directory. As a further protection the directory in the container is now set to be r-x.

As mentioned previously, this was mostly because Bazel didn't like two targets outputting to the same target - but this is now gone and it's the same value for the production image and for the integration image. As I am writing this I am realizing I can simplify this even more :)

burmudar · 2024-07-30T16:19:18Z

It's a little unclear how this all fits together. I guess I just wanna check the follow properties are true:
* in development we reload web.manifest.json if it changes. Otherwise its only read once.

* in prod you can't trick the location of the asset dir. I think that is the case and http.FileSystem won't allow access outside of the passed in dir.
What I'm a little unclear is why we allow customizing the value of the asset dir outside of dev. It seems like when constructing docker images we can just always hardcode the location of the asset dir?
1. Yup

2. You can technically trick it with arbitrary symlinks but that requries access to the filesystem already - according to [Dir.Open](https://cs.opensource.google/go/go/+/go1.22.5:src/net/http/fs.go;l=72). Otherwise it only serves from the passed directory. As a further protection the directory in the container is now set to be `r-x`.
As mentioned previously, this was mostly because Bazel didn't like two targets outputting to the same target - but this is now gone and it's the same value for the production image and for the integration image. As I am writing this I am realizing I can simplify this even more :)

Ok so made /assets-dist as constant which is used by default. Only other time is when we're in dev mode then it uses client/web/dist. No longer switching env vars to different directories

Strum355

So long bazel cquery 'somepath(//cmd/server:no_client_bundle.image, //client/web/dist:copy_bundle)' and bazel cquery 'somepath(//cmd/frontend:no_client_bundle.image, //client/web/dist:copy_bundle)' both return an empty set, this LGTM

burmudar · 2024-07-30T16:28:37Z

bazel cquery 'somepath(//cmd/frontend:no_client_bundle.image, //client/web/dist:copy_bundle)'

🥳

❯ bazel cquery 'somepath(//cmd/server:no_client_bundle.image, //client/web/dist:copy_bundle)'
WARNING: Build option --test_env has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed).
INFO: Analyzed 2 targets (5551 packages loaded, 82223 targets configured).
INFO: Found 2 targets...
INFO: Empty query results
INFO: Elapsed time: 25.931s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions

sourcegraph on  wb/frontend-assets [$] via 🐹 v1.22.4 via ❄️  impure (sourcegraph-dev-env) took 26s
❯ bazel cquery 'somepath(//cmd/frontend:no_client_bundle.image, //client/web/dist:copy_bundle)'
INFO: Analyzed 2 targets (47 packages loaded, 107 targets configured).
INFO: Found 2 targets...
INFO: Empty query results
INFO: Elapsed time: 0.598s, Critical Path: 0.00s
INFO: 0 processes.
INFO: Build completed successfully, 0 total actions

* Assets now get packaged with the frontend container that is built. * By default the Assets provider will look for an environment variable SRC_ASSETS_DIR and serve assets from there. * Renamed DevProvider to DirProvider

- set mode for assets-dist to 0655 - in integration test also use `assets-dist` dir - comments

In https://github.com/sourcegraph/sourcegraph/pull/63946 we reworked how we handle assets, but we overlooked updating the target in `sg.config.yaml`. ## Test plan CI + locally tested (made some changes on the home page, got reloaded as expected)

cla-bot Bot added the cla-signed label Jul 19, 2024

burmudar force-pushed the wb/frontend-assets branch 2 times, most recently from 6f41b74 to 0a41dee Compare July 24, 2024 14:19

burmudar requested a review from a team July 24, 2024 15:29

burmudar self-assigned this Jul 24, 2024

burmudar marked this pull request as ready for review July 24, 2024 15:29

Strum355 approved these changes Jul 24, 2024

View reviewed changes

burmudar force-pushed the wb/frontend-assets branch from e0193db to 3afddfb Compare July 25, 2024 09:27

burmudar commented Jul 25, 2024

View reviewed changes

Comment thread ui/assets/dev.go Outdated

burmudar commented Jul 25, 2024

View reviewed changes

Comment thread ui/assets/dev.go Outdated

keegancsmith reviewed Jul 25, 2024

View reviewed changes

eseliger reviewed Jul 25, 2024

View reviewed changes

Comment thread ui/assets/dev.go Outdated

eseliger reviewed Jul 25, 2024

View reviewed changes

burmudar commented Jul 30, 2024

View reviewed changes

Comment thread ui/assets/assets.go Outdated

burmudar commented Jul 30, 2024

View reviewed changes

Comment thread ui/assets/assets.go Outdated

burmudar commented Jul 30, 2024

View reviewed changes

Comment thread ui/assets/assets.go Outdated

burmudar requested review from eseliger, jhchabran and keegancsmith July 30, 2024 12:44

Strum355 reviewed Jul 30, 2024

View reviewed changes

Comment thread ui/assets/assets.go Outdated

Strum355 approved these changes Jul 30, 2024

View reviewed changes

burmudar force-pushed the wb/frontend-assets branch from 9a84e82 to 95b62ed Compare July 31, 2024 09:04

burmudar added 4 commits July 31, 2024 14:47

frontend: do not embed frontend assets anymore

69e6ae8

* Assets now get packaged with the frontend container that is built. * By default the Assets provider will look for an environment variable SRC_ASSETS_DIR and serve assets from there. * Renamed DevProvider to DirProvider

add ui/assets/dist

84057bd

use default asset path

4bed631

wip

8b58c0f

burmudar added 14 commits July 31, 2024 14:47

buildifier

9112020

tarception

bbab87e

make buildifier HAPPY

c075282

fixup

94fbb1a

add image_tests for assets

2151af3

use a sane default for assetsPath

f7e752c

Apply suggestions from code review

7f10609

cleanup

57b87c1

review comments

f2fbda7

- set mode for assets-dist to 0655 - in integration test also use `assets-dist` dir - comments

Apply suggestions from code review

37f8a8b

fixup

46d76ff

fixup

90adbd4

no longer use SRC_ASSETS_DIR

b80e363

update image tests

661c1f1

burmudar force-pushed the wb/frontend-assets branch from 95b62ed to 661c1f1 Compare July 31, 2024 12:49

burmudar merged commit 972ee32 into main Jul 31, 2024

burmudar deleted the wb/frontend-assets branch July 31, 2024 13:17

DaedalusG mentioned this pull request Aug 1, 2024

[Backport 5.5.x] fix(release): check for marker in container files instead of binary #64225

Closed

jhchabran mentioned this pull request Aug 9, 2024

chore(local): fix outdated frontend target being used #64382

Merged

Conversation

burmudar commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

Test plan

Changelog

Uh oh!

github-actions Bot commented Jul 19, 2024

Uh oh!

Uh oh!

Uh oh!

keegancsmith left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

keegancsmith Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

burmudar Jul 30, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eseliger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eseliger Jul 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

burmudar commented Jul 30, 2024

Uh oh!

burmudar commented Jul 30, 2024

Uh oh!

Uh oh!

Strum355 left a comment

Choose a reason for hiding this comment

Uh oh!

burmudar commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

burmudar commented Jul 19, 2024 •

edited

Loading