Static assets endpoint #5063

egor-ryashin · 2024-06-11T19:27:55Z

Files can be accessed through http endpoint, the template request:

GET /v1/instances/{instance_id}/assets/{path...}

For example, if repo has the following structure:

openrtb/
├── README.md
├── dashboards
│   ├── auction.yaml
│   └── bids.yaml
├── models
│   ├── auction_data_model.sql
│   ├── bids_data_model.sql
│   ├── model.sql
│   └── model_1.sql
├── public
│   ├── 2.txt
│   ├── custom.geo.json
│   └── geo.json
├── rill.yaml
├── sources
│   ├── auction_data_raw.yaml
│   └── bids_data_raw.yaml
└── tmp
    ├── default
    │   └── tmp
    ├── main.db
    ├── main.db.wal
    └── meta.db

then requesting public/geo.json can be done with:

GET /v1/instances/{instance_id}/assets/public/geo.json

begelundmuller · 2024-06-20T12:18:18Z

runtime/compilers/rillv1/parse_rillyaml.go

+	Features    yaml.Node `yaml:"features"`
+	PublicPaths []string  `yaml:"public_paths"`


Suggested change

Features yaml.Node `yaml:"features"`

PublicPaths []string `yaml:"public_paths"`

Features yaml.Node `yaml:"features"`

// Paths to expose over HTTP (defaults to ./public)

PublicPaths []string `yaml:"public_paths"`

begelundmuller · 2024-06-20T12:19:31Z

runtime/compilers/rillv1/parse_rillyaml.go

+	if len(tmp.PublicPaths) == 0 {
+		tmp.PublicPaths = []string{"public"}
+	}


Move to around line 168 – it's easier to navigate if the properties are handled in the same order as they are declared

begelundmuller · 2024-06-20T12:39:04Z

runtime/drivers/admin/admin.go

+		cache := make(map[string][]byte)
+		for _, p := range h.cachedPaths {
+			p = filepath.Join(h.projPath, p)
+			_, err = os.Stat(p)
+			if err != nil {
+				if os.IsNotExist(err) {
+					continue
+				}
+				return nil, err
+			}
+
+			err := filepath.Walk(p, func(path string, info os.FileInfo, err error) error {
+				if err != nil {
+					return err
+				}
+				if !info.IsDir() {
+					b, err := os.ReadFile(path)
+					if err != nil {
+						return err
+					}
+
+					rel, err := filepath.Rel(h.projPath, path)
+					if err != nil {
+						return err
+					}
+					cache[strings.TrimLeft(rel, "/")] = b
+				}
+				return nil
+			})
+			if err != nil {
+				return nil, err
+			}
+		}
+		h.assetsCache = cache


This is not a useful cache. The place where caching is nice is to be able to serve assets immediately when the runtime starts, even if Github takes a few minutes to clone. This means the cached files would need to be stored on the persistent disk, and be readable before the first call to cloneOrPull.

Also, we should not eagerly cache the assets in memory – it can lead to excessive memory usage if there are larger files. Relying on the OS file system page cache is more than adequate for our performance requirements here.

I wonder how a user can get into a state where there's cache on disk but no cloned repo? Because cache can be created from a clone repo.

And why would user like to delete a cloned repo leaving just the cache on disk?

The repo is currently cloned into a temp directory which will not exist after a process restart:

rill/runtime/drivers/admin/admin.go

Line 356 in 1983fb1

h.repoPath, err = os.MkdirTemp(h.config.TempDir, "admin_driver_repo")

.

It might be fine to cache the full repo on the persistent disk – that just isn't what it's doing now.

begelundmuller · 2024-06-20T12:39:53Z

runtime/drivers/file/repo.go

+	cache := make(map[string][]byte)
+	for _, p := range c.cachedPaths {
+		p = filepath.Join(c.root, p)
+		_, err := os.Stat(p)
+		if err != nil {
+			if os.IsNotExist(err) {
+				continue
+			}
+			return err
+		}
+		err = filepath.Walk(p, func(path string, info os.FileInfo, err error) error {
+			if err != nil {
+				return err
+			}
+			if !info.IsDir() {
+				b, err := os.ReadFile(path)
+				if err != nil {
+					return err
+				}
+
+				rel, err := filepath.Rel(c.root, path)
+				if err != nil {
+					return err
+				}
+				cache[strings.TrimLeft(rel, "/")] = b
+			}
+			return nil
+		})
+		if err != nil {
+			return err
+		}
+	}
+	c.cacheMutex.Lock()
+	defer c.cacheMutex.Unlock()
+	c.assetsCache = cache


The file driver does not have a cloning latency like Github has, so there is no need for caching assets in it.

begelundmuller · 2024-06-20T12:41:38Z

runtime/reconcilers/project_parser.go

+	paths := parser.RillYAML.PublicPaths
+	if len(paths) == 0 {
+		paths = []string{"public"}
+	}
+	repo, release, err := r.C.Runtime.Repo(ctx, parser.InstanceID)
+	if err != nil {
+		return err
+	}
+	defer release()
+	repo.SetCachedPaths(paths)


This should be handled inside UpdateInstanceWithRillYAML

begelundmuller · 2024-06-20T12:48:57Z

runtime/server/assets.go

+	paths := repo.GetCachedPaths()
+	allowed := false
+	for _, p := range paths {


It's too implicit to rely on the repo's cached paths as an allow list. It only works as long as that feature is only used for PublicPaths.

Instead, I would suggest explicitly adding PublicPaths as a property on Instance, assigning it in UpdateInstanceWithRillYAML, and checking it here.

so you suggest adding an instance reference to repo, right? because there's no instance in the params list

No, it could just pass the paths to cache, no need to pass the full instance. But as discussed above, it might be fine just to cache the whole repo on a persistent disk.

I would still suggest to omit caching from this PR since it's not so important

begelundmuller · 2024-06-20T12:52:39Z

runtime/server/assets.go

+	paths := repo.GetCachedPaths()
+	allowed := false
+	for _, p := range paths {
+		if strings.HasPrefix(strings.TrimLeft(path, ","), strings.TrimLeft(p, ",")) {


This is not a safe way to check the path. For example, if /public is the public directory, it will match /publications/secret.txt.

Why is it necessary to trim commas from the paths?

begelundmuller · 2024-06-20T12:53:22Z

runtime/server/assets.go

+		}
+	}
+	if !allowed {
+		return fmt.Errorf("path is not allowed")


Should return a httputil.Error with an appropriate status code

begelundmuller · 2024-06-20T13:11:57Z

I would suggest completing this PR without caching support and optionally taking it up in a follow up PR.

begelundmuller · 2024-06-24T18:08:01Z

runtime/drivers/registry.go

+	// Paths to expose over HTTP (defaults to ./public)
+	PublicPaths []string `db:"public_paths"`


Need to add migrations and handling for it in runtime/drivers/sqlite (can search for feature_flags to find locations)

begelundmuller · 2024-06-24T18:08:29Z

runtime/reconcilers/project_parser.go

+	if len(parser.RillYAML.PublicPaths) == 0 {
+		parser.RillYAML.PublicPaths = []string{"public"}
+	}
+


Can be removed since the default is assigned in parse_rillyaml.go

begelundmuller · 2024-06-24T18:08:45Z

runtime/drivers/file/file.go

@@ -58,6 +58,7 @@ type configProperties struct {
 // a smaller subset of relevant parts of rill.yaml
 type rillYAML struct {
 	IgnorePaths []string `yaml:"ignore_paths"`
+	CachedPaths []string `yaml:"public_paths"`


Suggested change

CachedPaths []string `yaml:"public_paths"`

PublicPaths []string `yaml:"public_paths"`

begelundmuller · 2024-06-24T18:11:18Z

@egor-ryashin After this PR merges, can you also update in #team-applications about the new endpoint? So that they can wire it up to the custom dashboard components.

static assets endpoint

39c854c

egor-ryashin requested a review from begelundmuller June 11, 2024 19:28

Egor Ryashin added 3 commits June 12, 2024 13:36

static assets endpoint - cache

fce3b92

static assets endpoint - thread safety

e1a246c

static assets endpoint - admin repo

efa1ed1

egor-ryashin changed the title ~~static assets endpoint~~ Static assets endpoint Jun 12, 2024

egor-ryashin marked this pull request as ready for review June 12, 2024 16:28

Egor Ryashin added 5 commits June 12, 2024 22:04

static assets endpoint - admin repo

0a44559

static assets endpoint - admin repo

20ba5e3

static assets endpoint - admin repo

35ffa68

static assets endpoint - admin repo

cae0f53

static assets endpoint - admin repo

540559d

begelundmuller requested changes Jun 20, 2024

View reviewed changes

Egor Ryashin added 8 commits June 21, 2024 14:37

static assets endpoint - admin repo

4224452

static assets endpoint - admin repo

00c3900

static assets endpoint - admin repo

9929ca3

static assets endpoint - admin repo

623fdcd

static assets endpoint - removing cache

541b5bb

static assets endpoint - removing cache

4085510

static assets endpoint - removing cache

ebcc2f5

static assets endpoint - removing cache

09072c7

begelundmuller requested changes Jun 24, 2024

View reviewed changes

Egor Ryashin and others added 6 commits June 25, 2024 14:47

static assets endpoint - migration

3f168f9

static assets endpoint - migration

0d3415e

static assets: obscure root dir

df6610f

static assets: obscure root dir

4b1612b

static assets: obscure root dir

6df117f

static assets: obscure root dir

2e858c0

begelundmuller approved these changes Jun 28, 2024

View reviewed changes

begelundmuller merged commit 349403e into main Jun 28, 2024
4 checks passed

begelundmuller deleted the static-assets branch June 28, 2024 12:18

begelundmuller assigned egor-ryashin Jun 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Static assets endpoint #5063

Static assets endpoint #5063

egor-ryashin commented Jun 11, 2024 •

edited

Loading

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin Jun 21, 2024

begelundmuller Jun 21, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

egor-ryashin Jun 20, 2024

begelundmuller Jun 21, 2024

begelundmuller Jun 21, 2024

begelundmuller Jun 20, 2024

begelundmuller Jun 20, 2024

begelundmuller commented Jun 20, 2024

begelundmuller Jun 24, 2024

begelundmuller Jun 24, 2024

begelundmuller Jun 24, 2024

begelundmuller commented Jun 24, 2024

		Features yaml.Node `yaml:"features"`
		PublicPaths []string `yaml:"public_paths"`

		// Paths to expose over HTTP (defaults to ./public)
		PublicPaths []string `db:"public_paths"`

	CachedPaths []string `yaml:"public_paths"`
	PublicPaths []string `yaml:"public_paths"`

Static assets endpoint #5063

Static assets endpoint #5063

Conversation

egor-ryashin commented Jun 11, 2024 • edited Loading

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

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

begelundmuller commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

begelundmuller commented Jun 24, 2024

egor-ryashin commented Jun 11, 2024 •

edited

Loading