Ping @ijc @tonistiigi @dmcgowan @AkihiroSuda

Such a straightforward change, so why didn't we do this sooner?

Do you have an example of how the client will use HijackDialer to setup a GRPC connection? I would be curious to see a buildkit client using this.

@dmcgowan this is how I tested it:

package main

import (
	"context"
	"fmt"
	"log"
	"net"
	"time"

	"github.com/docker/docker/client"
	bkclient "github.com/moby/buildkit/client"
)

func main() {
	ctx := context.Background()
	// tls config is optional, but it's just to show I tested it and it works.
	cli, err := client.NewClientWithOpts(client.FromEnv, client.WithTLSClientConfig("localhost.pem", "", ""))
	if err != nil {
		log.Fatal(err)
	}
	bk, err := bkclient.New(ctx, "/grpc", bkclient.WithDialer(func(addr string, _ time.Duration) (net.Conn, error) {
		return cli.DialHijack(ctx, addr, "h2c", nil)
	}))
	if err != nil {
		log.Fatal(err)
	}
	defer bk.Close()
	w, err := bk.ListWorkers(ctx)
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(w)
}

Codecov Report

❗ No coverage uploaded for pull request base (master@0133041). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #38990   +/-   ##
=========================================
  Coverage          ?    36.9%           
=========================================
  Files             ?      614           
  Lines             ?    45421           
  Branches          ?        0           
=========================================
  Hits              ?    16763           
  Misses            ?    26367           
  Partials          ?     2291

@tonistiigi @dmcgowan updated example, it works both with and without tls.

LGTM

TestAPISwarmLeaderElection is a known flaky test #32673

We may remove or change the endpoint, so nobody should rely on it.

Late to the party, but can we document that this feature is undocumented and nobody should rely on it?

@AkihiroSuda if we document that it's undocumented, then it's documented :D I think it's fine for now.

Perhaps a mention in the changelog to be explicit would be good.

If this feature is for debugging purposes; should it be fenced by either --debug being on, or enabled through a features option? (#37593 / #37692)

Are there any additional security concerns from exposing this to the network? I couldn't quite tell from the code if it was opt-in or always on.

@ijc "additional" security concerns? no. You're already root once you access the API.
@thaJeztah not for debugging purposes, would allow for more features on the cli to iterate on outside engine release cycle.

"additional" security concerns? no. You're already root once you access the API.

Might still be worth mentioning, in case there's systems that restrict access to specific endpoints (and if they (🤷‍♂️) use a blacklist instead of a whitelist); this was also my line of thinking for fencing it through a features field

@thaJeztah fine for changelog. If people do blacklist, it's a big security issue they need to fix as P0. If it's gated, it drastically limits the people able to try out new cli features via cli plugins. Having the GRPC API on for now is not a worse for security than having the HTTP one, and it's only an implementation detail for future cli experiments.

What's current status for documenting security concerns?