Add undocumented /grpc endpoint and register BuildKit's controller #38990
Conversation
|
Such a straightforward change, so why didn't we do this sooner? Do you have an example of how the client will use |
|
@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
@@ Coverage Diff @@
## master #38990 +/- ##
=========================================
Coverage ? 36.9%
=========================================
Files ? 614
Lines ? 45421
Branches ? 0
=========================================
Hits ? 16763
Misses ? 26367
Partials ? 2291 |
Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: Tibor Vass <tibor@docker.com>
|
@tonistiigi @dmcgowan updated example, it works both with and without tls. |
|
LGTM |
|
TestAPISwarmLeaderElection is a known flaky test #32673 |
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. |
|
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. |
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 |
|
@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? |
This endpoint would be extremely useful to test future features in both BuildKit and containerd. We may remove or change the endpoint, so nobody should rely on it.
For instance this could allow in the future to access contentstore blobs and provide namespaced containerd features.