controller: strongly type the controller api #1614

jedevc · 2023-02-09T15:48:46Z

Strongly typing the API allows us to perform all command line parsing fully on the client-side, where we have access to the client local directory and all the client environment variables, which may not be available on the remote server.

Additionally, the controller api starts to look a lot like build.Options, so at some point in the future there may be an oppportunity to merge the two, which would allow both build and bake to execute through the controller, instead of needing to maintain multiple code paths. Maybe? I'm not sure - I think it depends on how stdin is handled.

@ktock, I think this would make something like #1581 significantly easier to implement, am I right? We wouldn't need to dance around parsing and formatting CSVs, we could just edit the structs directly.

ktock

Thank you. Overall LGTM.

ktock · 2023-02-11T11:19:29Z

build/build.go

+	Session []session.Attachable
+


nit: Why do we need to change the order?

There's no real reason for this - except to try and match up with the controllerapi.BuildOptions struct, that I'd like to eventually merge into the controller API.

In conversation with @tonistiigi, we were talking about the possibility of having all buildx builds go through the controller interface, including with bake, etc, so as to reduce the number of possible code paths, though I think we need to keep playing around to try and see if that's possible.

tonistiigi · 2023-02-14T00:39:06Z

controller/pb/controller.proto

-  string NetworkMode = 14;
-  repeated string NoCacheFilter = 15;
-  repeated string Outputs = 16;
+  map<string, string> NamedContexts = 4;


We should try to group the things that belong together. Either in substructs where they make sense or at least reserved ID ranges.

Currently, everything is randomly mixed up. Parameters sent to frontend, input contexts with print flags, imageID/quiet that are client-side options(quiet probably shouldn't be here at all. Nice if things that rely on session would be separate as well. CommonOpts is not a good name and I don't understand the property split there.

We should try to group the things that belong together. Either in substructs where they make sense or at least reserved ID ranges.

Agreed. Can we do this as a follow-up, so as to not block #1581? Substructs would be my preference wherever possible.

imageID/quiet that are client-side options(quiet probably shouldn't be here at all)

Have pushed a commit that removes the quiet option entirely from the controller api - we can also extract the imageID file writing from the controller as well as a follow-up. To do this, we have the api return the solve response of the build, so we can extract the image id (which is needed because --quiet prints the image id, functionality that was previously broken on the remote controller).

CommonOpts is not a good name and I don't understand the property split there.

Currently, this is because these are command line options and are entirely shared with bake. This is another thing to refactor, but it's kind of all rolled up into "how do we get bake calling the controller api" - would prefer to address this a bit later, but it's on my todo list!

tonistiigi · 2023-02-14T00:40:29Z

controller/pb/controller.proto

+  map<string, string> Attrs = 2;
+}
+
+message SSH {


We need to think how the things that use session API work. Should everything in here have (optional) sessionID defined or at least one in BuildOptions.

I think for now, this is probably alright as is? AFAICT, each invocation to the build API should be in a separate session (except where we can perform future optimizations to merge groups of requests into a single session to improve performance)?

Maybe I'm missing something about future enhancements, but IMO not worth blocking on for now.

Strongly typing the API allows us to perform all command line parsing fully on the client-side, where we have access to the client local directory and all the client environment variables, which may not be available on the remote server. Additionally, the controller api starts to look a lot like build.Options, so at some point in the future there may be an oppportunity to merge the two, which would allow both build and bake to execute through the controller, instead of needing to maintain multiple code paths. Signed-off-by: Justin Chadwell <me@jedevc.com>

Now clients can access the result of the solve, specifically the image id output. This is a useful refactor, as well as being required if we want to allow bake to invoke through the controller api. This also allows us to remove the quiet option from the API, since we can compute the required progress type outside of the controller, and can print the image id from the result of the solve. As a follow-up, we should also be able to remove the image id file output from the controller api, now that the client has access to it. Signed-off-by: Justin Chadwell <me@jedevc.com>

Signed-off-by: Justin Chadwell <me@jedevc.com>

We can perform all attestation processing, handling how the sbom and provenance arguments interact on the client, while applying defaults on the server. Additionally, this allows us to start pulling fields out of CommonOpts. Signed-off-by: Justin Chadwell <me@jedevc.com>

tonistiigi

Comments for follow-ups

tonistiigi · 2023-02-23T18:31:13Z

controller/control/controller.go

 )

 type BuildxController interface {
-	Invoke(ctx context.Context, ref string, options controllerapi.ContainerConfig, ioIn io.ReadCloser, ioOut io.WriteCloser, ioErr io.WriteCloser) error
-	Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, w io.Writer, out console.File, progressMode string) (ref string, err error)
+	Build(ctx context.Context, options controllerapi.BuildOptions, in io.ReadCloser, w io.Writer, out console.File, progressMode string) (ref string, resp *client.SolveResponse, err error)


Why is progressmode part of controller interface.

Maybe this is semantics. Is Controller interface that different implementation implement (eg. direct/remote)? Or is it a high-level client interface a caller would use to invoke builds. I don't think it is the same thing.

cc @ktock, do you have additional context for this?

This is because remote/local controllers have different implementations for status handling (local: printed by (controller/build).RunBuild vs remote: printed by (controller/remote).Client.Build()). But I think we can now refactor this API to forward chan *client.SolveStatus instead of passing progressMode via that arg.

Can you open a follow-up for this one @ktock? 🙏

Opened #1671

tonistiigi · 2023-02-23T18:32:53Z

controller/pb/controller.proto

+  string Target = 21;
+  UlimitOpt Ulimits = 22;
+
+  CommonOptions Opts = 24;


Something needs to be done with this Opts inside Opts

jedevc requested review from tonistiigi and ktock February 9, 2023 15:48

jedevc added the area/debug label Feb 9, 2023

jedevc mentioned this pull request Feb 9, 2023

debug: remote controller inspects environment variables of server instead of client #1616

Open

ktock reviewed Feb 11, 2023

View reviewed changes

ktock approved these changes Feb 14, 2023

View reviewed changes

tonistiigi reviewed Feb 14, 2023

View reviewed changes

jedevc force-pushed the typed-controller-api branch from f6728d5 to 92404d4 Compare February 15, 2023 12:15

jedevc added 4 commits February 23, 2023 15:43

controller: move image id file writing to client

0b8f026

Signed-off-by: Justin Chadwell <me@jedevc.com>

jedevc force-pushed the typed-controller-api branch from bcf7057 to c2e1119 Compare February 23, 2023 15:49

tonistiigi approved these changes Feb 23, 2023

View reviewed changes

jedevc merged commit f3a4cd5 into docker:master Feb 24, 2023

jedevc mentioned this pull request Feb 24, 2023

monitor: resolve paths arguments in client #1581

Merged

ktock mentioned this pull request Mar 9, 2023

controller: followup refactoring on Build API #1671

Closed

jedevc mentioned this pull request Mar 28, 2023

[v0.10] Fix attestation overrides with duplicate types #1700

Closed

ktock mentioned this pull request Apr 3, 2023

controller: Extract nested CommonOptions on controller API #1713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

controller: strongly type the controller api #1614

controller: strongly type the controller api #1614

Uh oh!

jedevc commented Feb 9, 2023

Uh oh!

ktock left a comment

Uh oh!

ktock Feb 11, 2023

Uh oh!

jedevc Feb 13, 2023

Uh oh!

tonistiigi Feb 14, 2023

Uh oh!

jedevc Feb 14, 2023

Uh oh!

tonistiigi Feb 14, 2023

Uh oh!

jedevc Feb 14, 2023

Uh oh!

tonistiigi left a comment

Uh oh!

tonistiigi Feb 23, 2023

Uh oh!

jedevc Feb 24, 2023

Uh oh!

ktock Feb 24, 2023

Uh oh!

jedevc Mar 8, 2023

Uh oh!

ktock Mar 9, 2023

Uh oh!

tonistiigi Feb 23, 2023

Uh oh!

Uh oh!

controller: strongly type the controller api #1614

controller: strongly type the controller api #1614

Uh oh!

Conversation

jedevc commented Feb 9, 2023

Uh oh!

ktock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!