Skip to content
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.

Sign up for GitHub

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

Jump to bottom

added support for private Docker registries #1666

Closed
wants to merge 10 commits into from
Closed

added support for private Docker registries #1666

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7ff96c5
added support for private registries
Jun 21, 2023
acde15d
pass tests
Jun 21, 2023
0a78574
pass lint test
Jun 21, 2023
2a48b21
minor change
Jun 22, 2023
b173e8d
pass tests
Jun 22, 2023
d55812d
changed registry input to json
Jun 23, 2023
73f49c6
delete ds_store
Jun 24, 2023
a27816c
fix default bootstrap behavior
Jun 24, 2023
6bb8a46
pass lint test
Jun 24, 2023
3cf755e
command is not being used
Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added .DS_Store
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please delete this file.

Binary file not shown.
160 changes: 101 additions & 59 deletions pkg/app/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,6 @@ var CommandBootstrap = &cli.Command{
Usage: "Dockerhub mirror to use",
Aliases: []string{"m"},
},
&cli.StringFlag{
Name: "registry-ca-keypair",
Usage: "Specify the ca/key/cert file path for the private registry (format: 'ca=/etc/config/ca.pem,key=/etc/config/key.pem,cert=/etc/config/cert.pem')",
Aliases: []string{"ca"},
},
&cli.StringSliceFlag{
Name: "ssh-keypair",
Usage: fmt.Sprintf("Manually specify ssh key pair as `publicKey,privateKey`. Envd will generate a keypair at %s and %s if not specified",
Expand All @@ -73,7 +68,7 @@ var CommandBootstrap = &cli.Command{
},
&cli.StringFlag{
Name: "registry",
Usage: "Specify the registry to pull the image from",
Usage: "Specify private registries and their respective ca/key/cert file paths. Each group is separated by a semicolon. Format: --registry 'registry=my-registry,ca=/etc/config/ca.pem,key=/etc/config/key.pem,cert=/etc/config/cert.pem;registry=my-registry2,ca=/etc/config/ca2.pem,key=/etc/config/key2.pem,cert=/etc/config/cert2.pem'",
Aliases: []string{"r"},
},
},
Expand All @@ -89,7 +84,7 @@ func bootstrap(clicontext *cli.Context) error {
"SSH Key",
sshKey,
}, {
"registry CA keypair",
"registry",
registryCA,
}, {
"autocomplete",
Expand All @@ -112,56 +107,70 @@ func bootstrap(clicontext *cli.Context) error {
}

func registryCA(clicontext *cli.Context) error {
ca := clicontext.String("registry-ca-keypair")
if len(ca) == 0 {
return nil
}
mirror := clicontext.String("dockerhub-mirror")
if len(mirror) == 0 {
return errors.New("`registry-ca-keypair` should be used with `dockerhub-mirror`")
}
// Loop over all registry strings
if clicontext.IsSet("registry") {
for _, reg := range strings.Split(clicontext.String("registry"), ";") {
// Split into parts
parts := strings.Split(reg, ",")
if len(parts) != 4 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory to set CA for the private registry?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return fmt.Errorf("invalid registry string: %s", reg)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe use errors.Newf everywhere for consistency?

}

// parse ca/key/cert
kvPairs := strings.Split(ca, ",")
if len(kvPairs) != 3 {
return errors.New("`registry-ca-keypair` requires ca/key/cert 3 part separated by ','")
}
names := []string{"ca", "cert", "key"}
for _, pair := range kvPairs {
kv := strings.SplitN(pair, "=", 2)
index := -1
for i, name := range names {
if name == kv[0] {
index = i
break
// Parse registry and ca/key/cert parts
names := []string{"registry", "ca", "key", "cert"}
for _, part := range parts {
kv := strings.SplitN(part, "=", 2)
if len(kv) != 2 {
return fmt.Errorf("invalid part: %s", part)
}

// Check for valid key
index := -1
for i, name := range names {
if name == kv[0] {
index = i
break
}
}
if index == -1 {
return errors.Newf("parse error: `%s` is not a valid ca/key/cert key or it's duplicated")
}

// Read file if not registry part
if kv[0] != "registry" {
exist, err := fileutil.FileExists(kv[1])
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to parse file path %s", part))
}
if !exist {
return fmt.Errorf("file %s doesn't exist", kv[1])
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil {
return errors.Wrap(err, "failed to get the envd config file path")
}

// Read and write file content
content, err := os.ReadFile(kv[1])
if err != nil {
return errors.Wrap(err, "failed to read the file")
}
if err = os.WriteFile(path, content, 0644); err != nil {
return errors.Wrap(err, "failed to store the CA file")
}
}

// Remove key from names
names = append(names[:index], names[index+1:]...)
}
}
if index == -1 {
return errors.Newf("parse error: `%s` is not a valid ca/key/cert key or it's duplicated")
}
exist, err := fileutil.FileExists(kv[1])
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to parse file path %s", pair))
}
if !exist {
return errors.Newf("file %s doesn't exist", kv[1])
}
path, err := fileutil.ConfigFile(fmt.Sprintf("registry_%s.pem", kv[0]))
if err != nil {
return errors.Wrap(err, "failed to get the envd config file path")
}
content, err := os.ReadFile(kv[1])
if err != nil {
return errors.Wrap(err, "failed to read the file")
}
if err = os.WriteFile(path, content, 0644); err != nil {
return errors.Wrap(err, "failed to store the CA file")
}
names = append(names[:index], names[index+1:]...)
}

if len(names) != 0 {
return errors.Newf("registry %s are not provided", names)
// Check if any parts were not provided
if len(names) != 0 {
return fmt.Errorf("%s parts are not provided", names)
}
}
}
return nil
}
Expand Down Expand Up @@ -289,14 +298,21 @@ func buildkit(clicontext *cli.Context) error {
}

logrus.Debug("bootstrap the buildkitd container")
var bkClient buildkitd.Client

config := buildkitutil.BuildkitConfig{
Registry: clicontext.String("registry"),
Mirror: clicontext.String("dockerhub-mirror"),
UseHTTP: clicontext.Bool("use-http"),
SetCA: clicontext.IsSet("registry-ca-keypair"),
Mirror: clicontext.String("dockerhub-mirror"),
UseHTTP: clicontext.Bool("use-http"),
}

// Update the config with the registry info if needed
if clicontext.IsSet("registry") {
err = updateConfigWithRegistry(clicontext, &config)
if err != nil {
return err
}
}

var bkClient buildkitd.Client
if c.Builder == types.BuilderTypeMoby {
bkClient, err = buildkitd.NewMobyClient(clicontext.Context,
c.Builder, c.BuilderAddress, &config)
Expand All @@ -315,3 +331,29 @@ func buildkit(clicontext *cli.Context) error {

return nil
}

func updateConfigWithRegistry(clicontext *cli.Context, config *buildkitutil.BuildkitConfig) error {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be better to add a test to this func

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a method of the buildkitutil.BuildkitConfig?

// Parse registry strings
dirSet := make(map[string]bool)
for _, reg := range strings.Split(clicontext.String("registry"), ";") {
parts := strings.Split(reg, ",")
if len(parts) != 4 {
return fmt.Errorf("invalid registry string: %s", reg)
}

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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah so i think passing a json string might be better


// Add bindings to config
dir := filepath.Dir(caPath)
if !dirSet[dir] {
dirSet[dir] = true
config.Bindings = append(config.Bindings, fmt.Sprintf("%s:%s", dir, dir))
}
}

return nil
}
16 changes: 6 additions & 10 deletions pkg/driver/docker/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,15 @@ import (
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/filters"
"github.com/docker/docker/api/types/mount"
"github.com/docker/docker/client"
"github.com/docker/docker/pkg/jsonmessage"
"github.com/moby/term"
"github.com/sirupsen/logrus"

"github.com/tensorchord/envd/pkg/driver"
"github.com/tensorchord/envd/pkg/util/buildkitutil"
"github.com/tensorchord/envd/pkg/util/fileutil"
)

const buildkitdCertPath = "/etc/registry"

var (
anchoredIdentifierRegexp = regexp.MustCompile(`^([a-f0-9]{64})$`)
waitingInterval = 1 * time.Second
Expand Down Expand Up @@ -202,22 +198,22 @@ func (c dockerClient) StartBuildkitd(ctx context.Context, tag, name string, bc *
config := &container.Config{
Image: tag,
}

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will removing this cause any side effect?

Type: mount.TypeBind,
Source: fileutil.DefaultConfigDir,
Target: buildkitdCertPath,
})
// Attach bindings to Docker container. Bind mounts allow the container to access certain directories or files from the host machine.
if len(bc.Bindings) > 0 {
hostConfig.Binds = bc.Bindings
}

cfg, err := bc.String()
if err != nil {
return "", errors.Wrap(err, "failed to generate buildkit config")
}

config.Entrypoint = []string{
"/bin/sh",
"-c",
Expand Down
2 changes: 1 addition & 1 deletion pkg/driver/nerdctl/nerdctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (nc *nerdctlClient) StartBuildkitd(ctx context.Context, tag, name string, b
if !existed {
buildkitdCmd := "buildkitd"
// TODO: support mirror CA keypair
if bc.Registry != "" || bc.Mirror != "" || bc.UseHTTP {
if len(bc.Registry) != 0 || bc.Mirror != "" || bc.UseHTTP {
cfg, err := bc.String()
if err != nil {
return "", errors.Wrap(err, "failed to generate buildkit config")
Expand Down
36 changes: 25 additions & 11 deletions pkg/util/buildkitutil/buildkit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,31 @@ import (
)

const buildkitConfigTemplate = `
[registry."{{ if .Registry }}{{ .Registry }}{{ else }}docker.io{{ end }}"]{{ if .Mirror }}
mirrors = ["{{ .Mirror }}"]{{ end }}
http = {{ .UseHTTP }}
{{ if .SetCA}}ca=["/etc/registry/ca.pem"]
[[registry."{{ if .Registry }}{{ .Registry }}{{ else }}docker.io{{ end }}".keypair]]
key="/etc/registry/key.pem"
cert="/etc/registry/cert.pem"
{{ end }}
[registry]
{{- range $index, $value := .Registry }}
[registry."{{ if $value }}{{ $value }}{{ else }}docker.io{{ end }}"]
{{- if $.UseHTTP }}
http = true
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should make the mirror a string slice.

Copy link
Contributor Author

@RichhLi RichhLi Jun 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @kemingy WDYT

{{- end }}
{{- if $.Mirror }}
mirrors = ["{{ $.Mirror }}"]
{{- end }}
{{- if $.Ca }}
ca=["{{ index $.Ca $index }}"]
key=["{{ index $.Key $index }}"]
cert=["{{ index $.Cert $index }}"]
{{- end }}
{{- end }}
`

type BuildkitConfig struct {
Registry string
Mirror string
UseHTTP bool
SetCA bool
Registry []string
Ca []string
Cert []string
Key []string
Bindings []string
}

func (c *BuildkitConfig) String() (string, error) {
Expand All @@ -44,5 +54,9 @@ func (c *BuildkitConfig) String() (string, error) {
}
var config strings.Builder
err = tmpl.Execute(&config, c)
return config.String(), err
if err != nil {
return "", err
}

return config.String(), nil
}
60 changes: 44 additions & 16 deletions pkg/util/buildkitutil/buildkit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,64 @@ func TestBuildkitWithRegistry(t *testing.T) {
}{
{
BuildkitConfig{
Registry: "registry.example.com",
Registry: []string{"registry.example.com"},
Mirror: "https://mirror.example.com",
UseHTTP: true,
},
`
[registry."registry.example.com"]
mirrors = ["https://mirror.example.com"]
http = true
[registry]
[registry."registry.example.com"]
http = true
mirrors = ["https://mirror.example.com"]
`,
},
{
BuildkitConfig{
Registry: "registry.example.com",
SetCA: true,
Registry: []string{"registry.example.com"},
Ca: []string{"/etc/registry/ca.pem"},
Key: []string{"/etc/registry/key.pem"},
Cert: []string{"/etc/registry/cert.pem"},
},
`
[registry."registry.example.com"]
http = false
ca=["/etc/registry/ca.pem"]
[[registry."registry.example.com".keypair]]
key="/etc/registry/key.pem"
cert="/etc/registry/cert.pem"
[registry]
[registry."registry.example.com"]
ca=["/etc/registry/ca.pem"]
key=["/etc/registry/key.pem"]
cert=["/etc/registry/cert.pem"]
`,
},
{
BuildkitConfig{},
BuildkitConfig{
Registry: []string{},
},
`
[registry."docker.io"]
http = false
`,
[registry]
`,
},
{
BuildkitConfig{
Registry: []string{"registry.example.com", "registry.example2.com"},
Mirror: "https://mirror.example.com",
UseHTTP: true,
Ca: []string{"/etc/registry/ca.pem", "/etc/registry2/ca.pem"},
Key: []string{"/etc/registry/key.pem", "/etc/registry2/key.pem"},
Cert: []string{"/etc/registry/cert.pem", "/etc/registry2/cert.pem"},
},
`
[registry]
[registry."registry.example.com"]
http = true
mirrors = ["https://mirror.example.com"]
ca=["/etc/registry/ca.pem"]
key=["/etc/registry/key.pem"]
cert=["/etc/registry/cert.pem"]
[registry."registry.example2.com"]
http = true
mirrors = ["https://mirror.example.com"]
ca=["/etc/registry2/ca.pem"]
key=["/etc/registry2/key.pem"]
cert=["/etc/registry2/cert.pem"]
`,
},
}

Expand Down