-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New gateway exporter #6393
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
base: master
Are you sure you want to change the base?
New gateway exporter #6393
Conversation
92b531c to
040ea7f
Compare
| var foundFiles []string | ||
| var foundDescs []ocispecs.Descriptor | ||
| export := func(ctx context.Context, c gateway.Client, conn *grpc.ClientConn, _ exptypes.ExporterTarget, result *gateway.Result) error { | ||
| entries, err := result.Ref.ReadDir(ctx, gateway.ReadDirRequest{Path: "/"}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| for _, entry := range entries { | ||
| foundFiles = append(foundFiles, entry.Path) | ||
| } | ||
|
|
||
| store := contentproxy.NewContentStore(conn) | ||
| descs, err := result.Ref.GetRemote(ctx) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| foundDescs = filterAvailableDescriptors(ctx, store, descs) | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| _, err = c.BuildExport(sb.Context(), client.SolveOpt{}, "", frontend, export, nil) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a fun interface to me - allowing the client to easily do its own local export. This actually works entirely without any server-side changes - it's just calling the exporter function right after a build, using the same gateway.
We have access to the entire buildkit content store (so there's no isolation here, but this is client-trusted code).
exporter/exporter_test.go
Outdated
| func buildSampleExporter(ctx context.Context, c *client.Client, dest string) error { | ||
| gatewayDir, err := fsutil.NewFS(integration.BuildkitSourcePath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't work out a good test strategy that doesn't involve doing this 😢
To actually test the gateway exporter codepath + the isolation it provides, we actually need to build one so we can test it. It feels weird to do this in a test (as @tonistiigi points out in #3633 (comment)). But not sure what the alternative is 🤷 Some tests in exporter_test.go might not need it, but could otherwise just build it in bake, and connect it in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to remember writing this code before - but can't remember when.
@tonistiigi is there another implementation of this that you're aware of?
| return exporter.NewConfig() | ||
| } | ||
|
|
||
| func (e *gatewayExporterInstance) Export(ctx context.Context, llbBridge frontend.FrontendLLBBridge, exec executor.Executor, src *exporter.Source, inlineCache exptypes.InlineCache, sessionID string) (_ map[string]string, descref exporter.DescriptorReference, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the gateway exporter allow returning an arbitrary exporter response map? I haven't added that yet.
Seems like it would be nice to allow it:
- Could either reuse the existing
Returngateway API- New exporter response field
- Use the result metadata somehow
- Or create a new API (hoping to avoid this)
| // Target indicates the target type of the exporter | ||
| ExporterTarget Target = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is required to support the new file/dir keys (pointing out, since I didn't realize this in #3037 (comment)).
Without something like this, the exporter can't actually know what type was attached (since we strip out the attribute itself to avoid leaking it to the server).
040ea7f to
c188224
Compare
c188224 to
af8ac15
Compare
45e4d38 to
a930686
Compare
a930686 to
9a0d0c7
Compare
9a0d0c7 to
af16b82
Compare
|
Getting really close now, the only things I need to get done are:
Edit: done! This PR is now ready for review. |
Signed-off-by: Justin Chadwell <[email protected]>
This allows the client to be explicit as to what client-side inputs the client has provided (instead of implying this through the use of exporter attributes). Signed-off-by: Justin Chadwell <[email protected]>
Additionally, simplify some of the result loading/stashing logic into helpers. Signed-off-by: Justin Chadwell <[email protected]>
We'll need this in the exporter package, so break it up a little bit so it can be used there later. Signed-off-by: Justin Chadwell <[email protected]>
This wasn't previously used *at all*. However, now, we need to be able to do this, since exporters will speak the gateway api, and we want to be able to access provenance attestations. Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
Signed-off-by: Justin Chadwell <[email protected]>
af16b82 to
9e30a4c
Compare
|
Will it be a breaking change? How can I figure out if a buildkit demon support BuildFromEnvironment or ExportFromEnvironment supported As a gateway frontend? |
|
This will not be a breaking change.
|
Fixes #3037.
Follow-ups: