added support for private Docker registries #1666

RichhLi · 2023-06-21T18:41:06Z

Usage:

envd bootstrap --registry-config path_to_json_file

Format:

Note that the "name" field is required. The other fields are all optional.

This will allow users to specify private Docker registries and the respective credentials. Under the hood, we parse these credentials and create a corresponding buildkit TOML file, as well as set up Docker bindings. This enables users to pull and push images from private Docker registries.

Signed-off-by: Richard Li <ricli@linkedin.com>

kemingy · 2023-06-22T02:22:34Z

.DS_Store

Please delete this file.

kemingy · 2023-06-22T03:55:26Z

pkg/app/bootstrap.go

+		for _, reg := range strings.Split(clicontext.String("registry"), ";") {
+			// Split into parts
+			parts := strings.Split(reg, ",")
+			if len(parts) != 4 {


Is it mandatory to set CA for the private registry?

Yes. From our findings, ca is required. Interestingly, cert and key weren't necessary for us, but we wanted to keep the existing structure as generic as possible. We assume the original writers added the cert and key values for a reason.

kemingy · 2023-06-22T06:23:41Z

pkg/app/bootstrap.go

@@ -315,3 +331,29 @@ func buildkit(clicontext *cli.Context) error {

 	return nil
 }
+
+func updateConfigWithRegistry(clicontext *cli.Context, config *buildkitutil.BuildkitConfig) error {


Should this be a method of the buildkitutil.BuildkitConfig?

kemingy · 2023-06-22T06:26:01Z

pkg/app/bootstrap.go

+		config.Registry = append(config.Registry, strings.Split(parts[0], "=")[1])
+		caPath := strings.Split(parts[1], "=")[1]
+		config.Ca = append(config.Ca, caPath)
+		config.Key = append(config.Key, strings.Split(parts[2], "=")[1])
+		config.Cert = append(config.Cert, strings.Split(parts[3], "=")[1])


This has an assumption: the order has to be register,ca,key,cert but this has not been validated.

BTW, it seems these 3 files have to be in the same directory as the CA path. Otherwise the bind won't work.

yeah so i think passing a json string might be better

ByronHsu

General suggestions:

I think naming as registry-credential makes more sense then registry because the main purpose is to pass credential to enable auth with the registry.
I feel the string parsing logic is a bit overwhelming now. Users have to write bunch of stuff with ; and ,, which is kinda error-prone. I feel passing the whole thing as a json string would be easier to handle and validate. For example:

envd bootstrap --registry-credential='{
  "Registry": ["reg1", "reg2"],
  "Ca": ["ca1, "ca2"],
   ...etc
}'

or

envd bootstrap --registry-credentials='{
  "Credentials": [
     {
       "register": "reg1",
       "ca": "ca1",
     }
     {
       "register": "reg2",
       "ca": "ca2",
     }
  ]
}'

and you can use golang json to unmarshal the string like

type MyData struct {
	List []string `json:"list"`
}

func main() {
	app := &cli.App{
		Name:  "myapp",
		Usage: "An example CLI",
		Flags: []cli.Flag{
			&cli.StringFlag{
				Name:  "json",
				Usage: "JSON string argument",
			},
		},
		Action: func(c *cli.Context) error {
			jsonString := c.String("json")
			if jsonString != "" {
				var data MyData
				err := json.Unmarshal([]byte(jsonString), &data)
				if err != nil {
					return err
				}
				// Process the JSON data here
				fmt.Printf("List items: %+v\n", data.List)
			}
			return nil
		},
	}

	err := app.Run(os.Args)
	if err != nil {
		log.Fatal(err)
	}
}

Should use-http and dockerhub-mirror also be included in registry-credential? i see they are both for per registry

ByronHsu · 2023-06-22T06:05:20Z

pkg/app/bootstrap.go

+					}
+
+					// Generate file path
+					path, err := fileutil.ConfigFile(fmt.Sprintf("registry_%s.pem", kv[0]))


I am curious why do we have to read and write the pem files? can't we just use the original one?

This is copied from the original implementation. The main purpose is to make sure these 3 files are in the same directory since we will mount this directory to the buildkitd container.

So we basically copy 3 pems to the new dir? If that's the case, do we still need binding to allow the permission to /etc or other folders? I think buildkit will have permission to the new dir?

What do you mean the original impl? and Why not just use io.Copy instead of read and write?

Why not just use io.Copy instead of read and write?

That should be better.

So we basically copy 3 pems to the new dir? If that's the case, do we still need binding to allow the permission to /etc or other folders? I think buildkit will have permission to the new dir?

Need to mount a directory to the buildkitd container. This directory is used to store the envd config, which should be safe.

This PR supports multiple registries which means it needs different CAs for different registries. It might need to differentiate the file names.

ByronHsu · 2023-06-22T06:05:47Z

pkg/app/bootstrap.go

+			// Split into parts
+			parts := strings.Split(reg, ",")
+			if len(parts) != 4 {
+				return fmt.Errorf("invalid registry string: %s", reg)


maybe use errors.Newf everywhere for consistency?

ByronHsu · 2023-06-22T06:10:20Z

pkg/app/bootstrap.go

@@ -315,3 +331,29 @@ func buildkit(clicontext *cli.Context) error {

 	return nil
 }
+
+func updateConfigWithRegistry(clicontext *cli.Context, config *buildkitutil.BuildkitConfig) error {


would be better to add a test to this func

ByronHsu · 2023-06-22T06:11:14Z

pkg/util/buildkitutil/buildkit.go

+{{- range $index, $value := .Registry }}
+  [registry."{{ if $value }}{{ $value }}{{ else }}docker.io{{ end }}"]
+  	{{- if $.UseHTTP }}
+    http = true


what if they want to specify different http/mirrors for different registries?

Maybe we should make the mirror a string slice.

@kemingy advised me to leave the dockerhub-mirror command as is, since Docker mirrors are unrelated to private registries. I can change it, but then the command will be removed (the mirror config will be in the JSON file instead).

cc @kemingy WDYT

ByronHsu · 2023-06-22T06:33:56Z

pkg/driver/docker/docker.go

 	hostConfig := &container.HostConfig{
 		Privileged: true,
 		AutoRemove: true,
 	}

-	if bc.SetCA {
-		hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{


Will removing this cause any side effect?

Signed-off-by: Richard Li <ricli@linkedin.com>

kemingy · 2023-06-25T08:47:29Z

pkg/app/bootstrap.go

-			Usage:   "Specify the registry to pull the image from",
-			Aliases: []string{"r"},
+			Name:  "registry-config",
+			Usage: "Path to a JSON file containing registry configuration",


If we're going to pass a JSON file, why not use the buildkit TOML file directly?

I think the main purpose of the CLI is to make it user-friendly. If the situation is complex, it's almost the same as setting up a buildkitd instance manually.

This decision will heavily affect the implementation. What do you think?

cc @gaocegege @RichhLi @ByronHsu

How about providing the buildkit TOML directly to envd?

Very good point. The problem with directly providing a TOML file is that the Docker mount won't be set up. I think this pr may not even be necessary because we might as well just set up buildkitd manually like you said. We will discuss this.

@kemingy @gaocegege Would it be possible to keep the current envd implementation, and the only change we'll make is adding a command that accepts JSON input? And we'll add a check where users can only use one of the commands, not both at the same time. So it will be completely backward compatible.

@kemingy @gaocegege Would it be possible to keep the current envd implementation, and the only change we'll make is adding a command that accepts JSON input? And we'll add a check where users can only use one of the commands, not both at the same time. So it will be completely backward compatible.

I'm okay with it.

LGTM!

Will you implement it in this PR?

Yes, or do you want it in different pr?

No, I am just asking. Keeping in this PR LGTM.

Hi @RichhLi Do you still need this PR?

No, I merged this one: #1680

kemingy · 2023-09-03T05:26:41Z

close due to #1680

RichhLi requested review from aseaday, zwpaper, kemingy, VoVAllen and gaocegege as code owners June 21, 2023 18:41

Richard Li added 5 commits June 21, 2023 22:55

added support for private registries

7ff96c5

Signed-off-by: Richard Li <ricli@linkedin.com>

pass tests

acde15d

Signed-off-by: Richard Li <ricli@linkedin.com>

pass lint test

0a78574

Signed-off-by: Richard Li <ricli@linkedin.com>

minor change

2a48b21

Signed-off-by: Richard Li <ricli@linkedin.com>

pass tests

b173e8d

Signed-off-by: Richard Li <ricli@linkedin.com>

RichhLi force-pushed the support_private_registries branch from f7b7425 to b173e8d Compare June 22, 2023 05:55

kemingy reviewed Jun 22, 2023

View reviewed changes

ByronHsu reviewed Jun 22, 2023

View reviewed changes

Richard Li added 5 commits June 23, 2023 16:42

changed registry input to json

d55812d

Signed-off-by: Richard Li <ricli@linkedin.com>

delete ds_store

73f49c6

Signed-off-by: Richard Li <ricli@linkedin.com>

fix default bootstrap behavior

a27816c

Signed-off-by: Richard Li <ricli@linkedin.com>

pass lint test

6bb8a46

Signed-off-by: Richard Li <ricli@linkedin.com>

command is not being used

3cf755e

Signed-off-by: Richard Li <ricli@linkedin.com>

kemingy reviewed Jun 25, 2023

View reviewed changes

kemingy closed this Sep 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for private Docker registries #1666

added support for private Docker registries #1666

RichhLi commented Jun 21, 2023 •

edited

Loading

kemingy Jun 22, 2023

kemingy Jun 22, 2023

RichhLi Jun 22, 2023

kemingy Jun 22, 2023

kemingy Jun 22, 2023

ByronHsu Jun 22, 2023

ByronHsu left a comment •

edited

Loading

ByronHsu Jun 22, 2023

kemingy Jun 22, 2023

ByronHsu Jun 22, 2023

ByronHsu Jun 22, 2023

kemingy Jun 22, 2023

ByronHsu Jun 22, 2023

ByronHsu Jun 22, 2023

ByronHsu Jun 22, 2023

gaocegege Jun 25, 2023

RichhLi Jun 25, 2023 •

edited

Loading

gaocegege Jun 25, 2023

ByronHsu Jun 22, 2023

kemingy Jun 25, 2023

gaocegege Jun 25, 2023

RichhLi Jun 25, 2023

RichhLi Jun 27, 2023

kemingy Jun 27, 2023

gaocegege Jun 27, 2023 •

edited

Loading

RichhLi Jun 27, 2023

gaocegege Jun 27, 2023

gaocegege Sep 3, 2023

RichhLi Sep 3, 2023

kemingy commented Sep 3, 2023

added support for private Docker registries #1666

added support for private Docker registries #1666

Conversation

RichhLi commented Jun 21, 2023 • 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

ByronHsu left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RichhLi Jun 25, 2023 • 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

Choose a reason for hiding this comment

gaocegege Jun 27, 2023 • 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

kemingy commented Sep 3, 2023

RichhLi commented Jun 21, 2023 •

edited

Loading

ByronHsu left a comment •

edited

Loading

RichhLi Jun 25, 2023 •

edited

Loading

gaocegege Jun 27, 2023 •

edited

Loading