Initialize workspaces with additional file content #4428

svenefftinge · 2021-06-08T10:59:47Z

To allow starting workspaces based on a config that doesn't sit in Git, this PR introduces the possibility to pass file contents when creating a workspace, which overlays/overwrites the file system.

This is going to be used for passing configuration stored in projects. A prefix context additionalcontent (used below) is introduced for testing purposes, only.

Testing

A link that puts a .gitpod.yml and .gitpod.Dockerfile on top: https://se-external-config.staging.gitpod-dev.com/#/additionalcontent/eyIuZ2l0cG9kLnltbCI6ImltYWdlOlxuICBmaWxlOiAuZ2l0cG9kLkRvY2tlcmZpbGVcblxudGFza3M6XG4gIC0gY29tbWFuZDogZWNobyBcInRlc3RcIj5teWZpbGUudHh0IiwiLmdpdHBvZC5Eb2NrZXJmaWxlIjoiRlJPTSBnaXRwb2Qvd29ya3NwYWNlLWZ1bGxcblxuUlVOIC9ob21lL2dpdHBvZC8uY2FyZ28vYmluL2NhcmdvIGluc3RhbGwgbWRib29rIC0tdmVycyAwLjEuN1xuIn0=/https://github.com/gitpod-io/template-sveltejs

Same additional content but different repo, which triggers an image build on its own because the repo is different: https://se-external-config.staging.gitpod-dev.com/#/additionalcontent/eyIuZ2l0cG9kLnltbCI6ImltYWdlOlxuICBmaWxlOiAuZ2l0cG9kLkRvY2tlcmZpbGVcblxudGFza3M6XG4gIC0gY29tbWFuZDogZWNobyBcInRlc3RcIj5teWZpbGUudHh0IiwiLmdpdHBvZC5Eb2NrZXJmaWxlIjoiRlJPTSBnaXRwb2Qvd29ya3NwYWNlLWZ1bGxcblxuUlVOIC9ob21lL2dpdHBvZC8uY2FyZ28vYmluL2NhcmdvIGluc3RhbGwgbWRib29rIC0tdmVycyAwLjEuN1xuIn0=/https://github.com/gitpod-io/spring-petclinic

svenefftinge · 2021-06-09T11:44:50Z

/werft run

👍 started the job as gitpod-build-se-external-config.9

svenefftinge · 2021-06-10T06:25:12Z

/werft run

👍 started the job as gitpod-build-se-external-config.17

svenefftinge · 2021-06-10T09:56:53Z

components/server/src/workspace/config-provider.ts

+            }
+            if (!customConfig) {
+
+                // try and find config file in the context repo or remote in


for the reviewers: section below hasn't changed besides the indent because of the if.

Tip: Adding ?w=1 at the end of the review URL ignores all whitespace changes (i.e. re-indented blocks will appear unchanged).

svenefftinge · 2021-06-10T10:01:15Z

components/server/src/workspace/image-source-provider.ts

                const hostContext = this.hostContextProvider.get(context.repository.host);
                if (!hostContext || !hostContext.services) {
                    throw new Error(`Cannot fetch workspace image source for host: ${context.repository.host}`);
                }
-                const lastDockerFileSha = await hostContext.services.fileProvider.getLastChangeRevision(context.repository, context.revision, user, imgcfg.file);
+                const lastDockerFileShaPromise = hostContext.services.fileProvider.getLastChangeRevision(context.repository, context.revision, user, imgcfg.file);


we need to make two requests for the time being, to support the current resolution of image refs. Once we have rebuilt most images, we can remove the lastChangeRevision part.

If we resolved the base image here already we could check which case (lastDockerFileSha or this.getContentSHA(dockerFileContent)) resolves to a built image. This behaviour is what I referred to as "move the legacy behaviour to server" above.

csweichel · 2021-06-10T13:01:16Z

How are we dealing with environment variables and tasks? Right now the env vars assume that the workspace their pattern matches for can be trusted. With this change if I can make someone click on a link I can quite easily get their secrets because I can bring my own tasks.

svenefftinge · 2021-06-10T13:20:51Z

How are we dealing with environment variables and tasks? Right now the env vars assume that the workspace their pattern matches for can be trusted. With this change if I can make someone click on a link I can quite easily get their secrets because I can bring my own tasks.

I think this is a general problem that exists with all our workspace start URLs. The underlying issue is that supervisor runs processes and it is not obvious what those processes will be from looking at a link. I suggest we introduce something where we ask users if they trust certain sources to allow task execution.

csweichel · 2021-06-11T06:08:15Z

How are we dealing with environment variables and tasks? Right now the env vars assume that the workspace their pattern matches for can be trusted. With this change if I can make someone click on a link I can quite easily get their secrets because I can bring my own tasks.

I think this is a general problem that exists with all our workspace start URLs. The underlying issue is that supervisor runs processes and it is not obvious what those processes will be from looking at a link. I suggest we introduce something where we ask users if they trust certain sources to allow task execution.

Indeed it does. What the additionalContent context URL does though is to explicitly circumvent the "security model" of environment variables/secrets (namely repository patterns). The assumption underpinning environment variables is that you trust the repository you specify using the repository pattern. There's a chance you trust this repo because you trust everyone who can modify its content, hence its tasks.
If we now allow arbitrary tasks to be executed in arbitrary repositories for arbitrary users (all you need is to click a link), that undermines this - already somewhat weak - idea of "trust in a repo".

Asking users if they want to execute the task might be one way - and is indeed a trade-off between friction and security.
Alternatively we could re-consider the Workspace Secrets RFC which would alleviate this problem also.

components/content-service-api/initializer.proto

csweichel · 2021-06-11T06:20:03Z

components/image-builder-api/imgbuilder.proto

 message BuildSourceDockerfile {
    contentservice.WorkspaceInitializer source = 1;
    string dockerfile_version = 2;
    string dockerfile_path = 3;
    string context_path = 4;
+    string dockerfile_content_sha = 5;


We could re-use the dockerfile_version - I don't think we need to change the protocol here.
The only difference is that before the version was the commit hash, now it's the SHA hash.

If we moved the "legacy" behaviour to server, that is.

csweichel · 2021-06-11T06:23:29Z

components/image-builder/pkg/builder/docker.go

@@ -224,6 +224,10 @@ func (b *DockerBuilder) getBaseImageRef(ctx context.Context, bs *api.BuildSource
 		return b.getAbsoluteImageRef(ctx, src.Ref.Ref, allowedAuth)

 	case *api.BuildSource_File:
+		if src.File.DockerfileContentSha != "" {
+			return fmt.Sprintf("%s:%x", b.Config.BaseImageRepository, src.File.DockerfileContentSha), nil


The manifest approach below takes more into account than just the Dockerfile version. Using just the content hash ignores the context entirely - the same Dockerfile from two separate repositories with different context would get the same "build".

This could even leak sensitive details because one user might get the build of another (same Dockerfile content, but different context including repo).

Note: because GitHub doesn't let me put a comment in line 238 ... you'd have to expand the if to include the CompositeInitializer etc.

csweichel · 2021-06-11T06:30:44Z

components/server/src/workspace/image-source-provider.ts

                const hostContext = this.hostContextProvider.get(context.repository.host);
                if (!hostContext || !hostContext.services) {
                    throw new Error(`Cannot fetch workspace image source for host: ${context.repository.host}`);
                }
-                const lastDockerFileSha = await hostContext.services.fileProvider.getLastChangeRevision(context.repository, context.revision, user, imgcfg.file);
+                const lastDockerFileShaPromise = hostContext.services.fileProvider.getLastChangeRevision(context.repository, context.revision, user, imgcfg.file);


If we resolved the base image here already we could check which case (lastDockerFileSha or this.getContentSHA(dockerFileContent)) resolves to a built image. This behaviour is what I referred to as "move the legacy behaviour to server" above.

csweichel · 2021-06-11T06:32:26Z

components/server/src/workspace/workspace-starter.ts

+                    source = initializer;
+                    disp.push(disposable);
+                } else {
+                    //TODO we cannot change this initializer structure now because it is part of how baserefs are computed in image-builder.


nit:

Suggested change

//TODO we cannot change this initializer structure now because it is part of how baserefs are computed in image-builder.

// TODO(se): we cannot change this initializer structure now because it is part of how baserefs are computed in image-builder.

see https://www.notion.so/gitpod/How-we-develop-e8ce7e562478458a9223eb9b797415b2#86e669f8e7df4f1289c9bede0855b5f9

I'd propose we update the behaviour in image builder to accompany the new initializer. Also, the current change on how the baseRef is computed ignores the initializer entirely.

csweichel · 2021-06-11T06:34:13Z

components/server/src/workspace/workspace-starter.ts

+                let contextPath = checkoutLocation;
+                let dockerFilePath = checkoutLocation + '/' + imgsrc.dockerFilePath;


This seems to be at odds with the comment above. Either the comment or the code is wrong :)

svenefftinge · 2021-06-11T10:24:57Z

/werft run

👍 started the job as gitpod-build-se-external-config.26

svenefftinge · 2021-06-12T08:03:55Z

/werft run

👍 started the job as gitpod-build-se-external-config.28

Introduces an AdditionalContentContext and the corresponding Initializer.

svenefftinge · 2021-06-12T11:27:46Z

Thanks, @csweichel! I have updated the PR as discussed, could you have a look again?

svenefftinge · 2021-06-14T14:18:23Z

/werft run

👍 started the job as gitpod-build-se-external-config.36

svenefftinge · 2021-06-15T05:42:13Z

/werft run

👍 started the job as gitpod-build-se-external-config.37

csweichel · 2021-06-15T11:00:49Z

I added unit tests and implemented the digest check in the content initializer. Unfortunately the digest sent by the server does not match what we actually download:

svenefftinge · 2021-06-15T11:16:36Z

components/image-builder/pkg/builder/docker.go

 		}
+		manifest[prefix+"Files"] = fmt.Sprintf("%x", hash.Sum(nil))


Wouldn't this trigger docker builds on every change in .gitpod.yml (or other files)? I believe this would be too aggressive.

to the new FileDownloadInitializer

csweichel

LGTM

csweichel · 2021-06-14T14:30:14Z

components/content-service-api/initializer.proto

+       string digest = 3;
+    }
+    repeated FileInfo files = 1;
+    string target_location = 2;


beware that all initializer are executed relative to a target location themselves, i.e. all paths in an initializer are always relative to that location.

The comment in line 30 implies that target_location can be absolute, when in reality it cannot.

csweichel · 2021-06-15T06:39:02Z

components/content-service/pkg/initializer/download.go

+	for _, info := range ws.FilesInfos {
+		contents, err := downloadFile(ctx, info.url)
+		if err != nil {
+			tracing.LogError(span, xerrors.Errorf("cannot download file '%s' from '%s': %w", info.filePath, info.url, err))


Is it intentional that when downloading a file fails the content init does not fail?

csweichel · 2021-06-15T06:39:43Z

components/content-service/pkg/initializer/download.go

+	return src, nil
+}
+
+func downloadFile(ctx context.Context, url string) (content []byte, err error) {


downloadFile should write the file to disk immediately and not keep it in RAM first.

also, we need to respect the digest

csweichel · 2021-06-15T07:39:37Z

components/content-service/pkg/initializer/initializer.go

+func (e *CompositeInitializer) Run(ctx context.Context, mappings []archive.IDMapping) (csapi.WorkspaceInitSource, error) {
+	_, ctx = opentracing.StartSpanFromContext(ctx, "CompositeInitializer.Run")
+	for _, init := range e.Initializer {
+		init.Run(ctx, mappings)


if one initializer fails the others should fail, too

svenefftinge force-pushed the se/external-config branch from d10df5e to 1cb3a6f Compare June 8, 2021 11:00

geropl mentioned this pull request Jun 8, 2021

Load Gitpod setup through URL params #3746

Closed

svenefftinge force-pushed the se/external-config branch 3 times, most recently from f3076b8 to 490fd65 Compare June 9, 2021 10:55

svenefftinge force-pushed the se/external-config branch 3 times, most recently from 97ea581 to 221da19 Compare June 9, 2021 13:20

svenefftinge changed the title ~~[WIP] Initialize workspaces with additional file content~~ Initialize workspaces with additional file content Jun 9, 2021

svenefftinge force-pushed the se/external-config branch 2 times, most recently from 35886e4 to 1959053 Compare June 9, 2021 16:07

svenefftinge force-pushed the se/external-config branch 3 times, most recently from 0234ebf to 9bdd722 Compare June 10, 2021 09:19

svenefftinge marked this pull request as ready for review June 10, 2021 09:42

svenefftinge force-pushed the se/external-config branch from 9bdd722 to 2a14026 Compare June 10, 2021 09:50

svenefftinge commented Jun 10, 2021

View reviewed changes

svenefftinge force-pushed the se/external-config branch from 2a14026 to b97272b Compare June 10, 2021 10:24

csweichel reviewed Jun 11, 2021

View reviewed changes

svenefftinge force-pushed the se/external-config branch from b97272b to 09a343c Compare June 11, 2021 13:26

svenefftinge force-pushed the se/external-config branch from d2ec3fa to 3b8cf5a Compare June 12, 2021 11:03

initialize workspaces with additional file content

c10fb46

Introduces an AdditionalContentContext and the corresponding Initializer.

svenefftinge force-pushed the se/external-config branch from 3b8cf5a to c10fb46 Compare June 12, 2021 11:27

csweichel force-pushed the se/external-config branch 2 times, most recently from a6cdde6 to 3fa9438 Compare June 15, 2021 10:36

svenefftinge commented Jun 15, 2021

View reviewed changes

csweichel force-pushed the se/external-config branch from 3fa9438 to a7c2638 Compare June 15, 2021 11:27

[initializer] Add digest support and unit tests

5436968

to the new FileDownloadInitializer

csweichel force-pushed the se/external-config branch from a7c2638 to 5436968 Compare June 15, 2021 11:36

csweichel approved these changes Jun 15, 2021

View reviewed changes

svenefftinge merged commit 8cc1e52 into main Jun 15, 2021

svenefftinge deleted the se/external-config branch June 15, 2021 12:18

cart mentioned this pull request Jun 27, 2022

gitpod support bevyengine/bevy#4700

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize workspaces with additional file content #4428

Initialize workspaces with additional file content #4428

svenefftinge commented Jun 8, 2021 •

edited

Loading

svenefftinge commented Jun 9, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge Jun 10, 2021

jankeromnes Jun 25, 2021

svenefftinge Jun 10, 2021

csweichel Jun 11, 2021

csweichel commented Jun 10, 2021

svenefftinge commented Jun 10, 2021

csweichel commented Jun 11, 2021 •

edited

Loading

csweichel Jun 11, 2021

csweichel Jun 11, 2021

csweichel Jun 11, 2021

csweichel Jun 11, 2021

csweichel Jun 11, 2021

csweichel Jun 11, 2021

csweichel Jun 11, 2021

svenefftinge commented Jun 11, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 12, 2021 •

edited

Loading

svenefftinge commented Jun 14, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 15, 2021 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Jun 15, 2021

svenefftinge Jun 15, 2021 •

edited

Loading

csweichel left a comment

csweichel Jun 14, 2021

csweichel Jun 15, 2021

csweichel Jun 15, 2021

csweichel Jun 15, 2021

csweichel Jun 15, 2021

	//TODO we cannot change this initializer structure now because it is part of how baserefs are computed in image-builder.
	// TODO(se): we cannot change this initializer structure now because it is part of how baserefs are computed in image-builder.

		let contextPath = checkoutLocation;
		let dockerFilePath = checkoutLocation + '/' + imgsrc.dockerFilePath;

		}
		manifest[prefix+"Files"] = fmt.Sprintf("%x", hash.Sum(nil))

Initialize workspaces with additional file content #4428

Initialize workspaces with additional file content #4428

Conversation

svenefftinge commented Jun 8, 2021 • edited Loading

Testing

svenefftinge commented Jun 9, 2021 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Jun 10, 2021 • edited by werft-gitpod-dev-com bot 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

csweichel commented Jun 10, 2021

svenefftinge commented Jun 10, 2021

csweichel commented Jun 11, 2021 • 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

svenefftinge commented Jun 11, 2021 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Jun 12, 2021 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Jun 12, 2021 • edited Loading

svenefftinge commented Jun 14, 2021 • edited by werft-gitpod-dev-com bot Loading

svenefftinge commented Jun 15, 2021 • edited by werft-gitpod-dev-com bot Loading

csweichel commented Jun 15, 2021

svenefftinge Jun 15, 2021 • edited Loading

Choose a reason for hiding this comment

csweichel 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

svenefftinge commented Jun 8, 2021 •

edited

Loading

svenefftinge commented Jun 9, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 10, 2021 •

edited by werft-gitpod-dev-com bot

Loading

csweichel commented Jun 11, 2021 •

edited

Loading

svenefftinge commented Jun 11, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 12, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 12, 2021 •

edited

Loading

svenefftinge commented Jun 14, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge commented Jun 15, 2021 •

edited by werft-gitpod-dev-com bot

Loading

svenefftinge Jun 15, 2021 •

edited

Loading