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.

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

added support for private Docker registries #1666

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
156 changes: 92 additions & 64 deletions pkg/app/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package app

import (
"encoding/json"
"fmt"
"os"
"path/filepath"
Expand Down Expand Up @@ -55,32 +56,33 @@ 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",
sshconfig.GetPublicKeyOrPanic(), sshconfig.GetPrivateKeyOrPanic()),
Aliases: []string{"k"},
},
&cli.BoolFlag{
Name: "use-http",
Usage: "Use HTTP instead of HTTPS for the registry",
Value: false,
},
&cli.StringFlag{
Name: "registry",
Usage: "Specify the registry to pull the image from",
Aliases: []string{"r"},
Name: "registry-config",
Usage: "Path to a JSON file containing registry configuration",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

How about providing the buildkit TOML directly to envd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@gaocegege gaocegege Jun 27, 2023

Choose a reason for hiding this comment

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

LGTM!

Will you implement it in this PR?

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, or do you want it in different pr?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Hi @RichhLi Do you still need this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I merged this one: #1680

},
},

Action: bootstrap,
}

type Registry struct {
Name string `json:"name"`
Ca string `json:"ca"`
Cert string `json:"cert"`
Key string `json:"key"`
UseHttp bool `json:"use_http"`
}

type RegistriesData struct {
Registries []Registry `json:"registries"`
}

func bootstrap(clicontext *cli.Context) error {
stages := []struct {
Name string
Expand All @@ -89,8 +91,8 @@ func bootstrap(clicontext *cli.Context) error {
"SSH Key",
sshKey,
}, {
"registry CA keypair",
registryCA,
"registry-config",
registryConfig,
}, {
"autocomplete",
autocomplete,
Expand All @@ -111,57 +113,65 @@ func bootstrap(clicontext *cli.Context) error {
return nil
}

func registryCA(clicontext *cli.Context) error {
ca := clicontext.String("registry-ca-keypair")
if len(ca) == 0 {
func registryConfig(clicontext *cli.Context) error {
configFile := clicontext.String("registry-config")
if len(configFile) == 0 {
return nil
}
mirror := clicontext.String("dockerhub-mirror")
if len(mirror) == 0 {
return errors.New("`registry-ca-keypair` should be used with `dockerhub-mirror`")

// Check if file exists
exist, err := fileutil.FileExists(configFile)
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to parse file path %s", configFile))
}
if !exist {
return errors.Newf("file %s doesn't exist", configFile)
}

// 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 ','")
var data RegistriesData
configJson, err := os.ReadFile(configFile)
if err != nil {
return errors.Wrap(err, "Failed to read registry config file")
}
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
}
}
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 err := json.Unmarshal(configJson, &data); err != nil {
return errors.Wrap(err, "Failed to parse registry config file")
}

if len(names) != 0 {
return errors.Newf("registry %s are not provided", names)
// Check for required keys in each registry
for i, registry := range data.Registries {
if registry.Name == "" {
return errors.Newf("`name` key is required in the config for registry at index %d", i)
}

// Check for optional keys and if they exist, ensure they point to existing files
optionalKeys := map[string]string{"ca": registry.Ca, "cert": registry.Cert, "key": registry.Key}
for key, value := range optionalKeys {
if value != "" {
exist, err := fileutil.FileExists(value)
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to parse file path %s", value))
}
if !exist {
return errors.Newf("file %s doesn't exist", value)
}

// Read the file
content, err := os.ReadFile(value)
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to read the %s file for registry %s", key, registry.Name))
}

// Write the content to the envd config directory
envdConfigPath, err := fileutil.ConfigFile(fmt.Sprintf("%s_%s.pem", registry.Name, key))
if err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to get the envd config file path for %s of registry %s", key, registry.Name))
}

if err = os.WriteFile(envdConfigPath, content, 0644); err != nil {
return errors.Wrap(err, fmt.Sprintf("failed to store the %s file for registry %s", key, registry.Name))
}
}
}
}
return nil
}
Expand Down Expand Up @@ -240,7 +250,7 @@ func sshKey(clicontext *cli.Context) error {
return nil

default:
return errors.Errorf("Invalid ssh-keypair flag")
return errors.Newf("Invalid ssh-keypair flag")
}
}

Expand Down Expand Up @@ -290,11 +300,29 @@ func buildkit(clicontext *cli.Context) error {

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

// Populate the BuildkitConfig struct
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"),
}

configFile := clicontext.String("registry-config")
if len(configFile) != 0 {
configJson, err := os.ReadFile(configFile)
if err != nil {
return errors.Wrap(err, "Failed to read registry config file")
}
if err := json.Unmarshal(configJson, &data); err != nil {
return errors.Wrap(err, "Failed to parse registry config file")
}
for _, registry := range data.Registries {
config.RegistryName = append(config.RegistryName, registry.Name)
config.CaPath = append(config.CaPath, registry.Ca)
config.CertPath = append(config.CertPath, registry.Cert)
config.KeyPath = append(config.KeyPath, registry.Key)
config.UseHTTP = append(config.UseHTTP, registry.UseHttp)
}
}

if c.Builder == types.BuilderTypeMoby {
Expand Down
3 changes: 2 additions & 1 deletion pkg/driver/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,14 @@ func (c dockerClient) StartBuildkitd(ctx context.Context, tag, name string, bc *
AutoRemove: true,
}

if bc.SetCA {
if len(bc.RegistryName) > 0 {
hostConfig.Mounts = append(hostConfig.Mounts, mount.Mount{

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,
})
}

cfg, err := bc.String()
if err != nil {
return "", errors.Wrap(err, "failed to generate buildkit config")
Expand Down
2 changes: 1 addition & 1 deletion pkg/driver/nerdctl/nerdctl.go
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.RegistryName) > 0 || bc.Mirror != "" || len(bc.UseHTTP) > 0 {
cfg, err := bc.String()
if err != nil {
return "", errors.Wrap(err, "failed to generate buildkit config")
Expand Down
36 changes: 24 additions & 12 deletions pkg/util/buildkitutil/buildkit.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,33 @@ 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 := .RegistryName }}
[registry."{{ if $value }}{{ $value }}{{ else }}docker.io{{ end }}"]
{{- if index $.UseHTTP $index }}
http = true

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

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 index $.CaPath $index }}
ca=["/etc/registry/{{ $value }}_ca.pem"]
{{- end }}
{{- if and (index $.CertPath $index) (index $.KeyPath $index) }}
[[registry."{{ if $value }}{{ $value }}{{ else }}docker.io{{ end }}".keypair]]
key="/etc/registry/{{ $value }}_key.pem"
cert="/etc/registry/{{ $value }}_cert.pem"
{{- end }}
{{- end }}
`

type BuildkitConfig struct {
Registry string
Mirror string
UseHTTP bool
SetCA bool
RegistryName []string
CaPath []string
CertPath []string
KeyPath []string
UseHTTP []bool
Mirror string
}

func (c *BuildkitConfig) String() (string, error) {
Expand Down
79 changes: 61 additions & 18 deletions pkg/util/buildkitutil/buildkit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,36 +28,79 @@ func TestBuildkitWithRegistry(t *testing.T) {
}{
{
BuildkitConfig{
Registry: "registry.example.com",
Mirror: "https://mirror.example.com",
UseHTTP: true,
RegistryName: []string{"registry.example.com"},
CaPath: []string{"/etc/registry/ca.pem"},
CertPath: []string{"/etc/registry/cert.pem"},
KeyPath: []string{"/etc/registry/key.pem"},
UseHTTP: []bool{false},
Mirror: "https://mirror.example.com",
},
`
[registry."registry.example.com"]
mirrors = ["https://mirror.example.com"]
http = true
[registry]
[registry."registry.example.com"]
mirrors = ["https://mirror.example.com"]
ca=["/etc/registry/registry.example.com_ca.pem"]
[[registry."registry.example.com".keypair]]
key="/etc/registry/registry.example.com_key.pem"
cert="/etc/registry/registry.example.com_cert.pem"
`,
},
{
BuildkitConfig{
Registry: "registry.example.com",
SetCA: true,
RegistryName: []string{"registry.example.com", "docker.io"},
CaPath: []string{"", ""},
CertPath: []string{"", ""},
KeyPath: []string{"", ""},
UseHTTP: []bool{true, false},
Mirror: "https://mirror.example.com",
},
`
[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"]
http = true
mirrors = ["https://mirror.example.com"]
[registry."docker.io"]
mirrors = ["https://mirror.example.com"]
`,
},
{
BuildkitConfig{},
BuildkitConfig{
RegistryName: []string{},
CaPath: []string{},
CertPath: []string{},
KeyPath: []string{},
UseHTTP: []bool{},
Mirror: "",
},
`
[registry."docker.io"]
http = false
`,
[registry]
`,
},
{
BuildkitConfig{
RegistryName: []string{"registry1.example.com", "registry2.example.com"},
CaPath: []string{"/etc/registry/ca1.pem", "/etc/registry/ca2.pem"},
CertPath: []string{"/etc/registry/cert1.pem", "/etc/registry/cert2.pem"},
KeyPath: []string{"/etc/registry/key1.pem", "/etc/registry/key2.pem"},
UseHTTP: []bool{true, false},
Mirror: "https://mirror.example.com",
},
`
[registry]
[registry."registry1.example.com"]
http = true
mirrors = ["https://mirror.example.com"]
ca=["/etc/registry/registry1.example.com_ca.pem"]
[[registry."registry1.example.com".keypair]]
key="/etc/registry/registry1.example.com_key.pem"
cert="/etc/registry/registry1.example.com_cert.pem"
[registry."registry2.example.com"]
mirrors = ["https://mirror.example.com"]
ca=["/etc/registry/registry2.example.com_ca.pem"]
[[registry."registry2.example.com".keypair]]
key="/etc/registry/registry2.example.com_key.pem"
cert="/etc/registry/registry2.example.com_cert.pem"
`,
},
}

Expand Down