fn: cleanup of docker private registry code (#1130)

* fn: cleanup of docker private registry code

Start using URL parsed ServerAddress and its subdomains
for easier image ensure/pull in docker driver. Previous
code to lookup substrings was faulty without proper
URL parse and hostname tokenization. When searching
for a registry config, if image name does not contain
a registry and if there's a private registry configured,
then search for hub.docker.com and index.docker.io. This
is similar to previous code but with correct subdomain
matching.

* fn-dataplane: take port into account in auth configs

api/agent/drivers/docker/docker.go

@@ -45,6 +45,11 @@ type runResult struct {
 	status string
 }
 
+type driverAuthConfig struct {
+	auth       docker.AuthConfiguration
+	subdomains map[string]bool
+}
+
 func (r *runResult) Error() error   { return r.err }
 func (r *runResult) Status() string { return r.status }
 
@@ -52,7 +57,7 @@ type DockerDriver struct {
 	conf     drivers.Config
 	docker   dockerClient // retries on *docker.Client, restricts ad hoc *docker.Client usage / retries
 	hostname string
-	auths    map[string]docker.AuthConfiguration
+	auths    map[string]driverAuthConfig
 	pool     DockerPool
 	// protects networks map
 	networksLock sync.Mutex
@@ -66,11 +71,16 @@ func NewDocker(conf drivers.Config) *DockerDriver {
 		logrus.WithError(err).Fatal("couldn't resolve hostname")
 	}
 
+	auths, err := registryFromEnv()
+	if err != nil {
+		logrus.WithError(err).Fatal("couldn't initialize registry")
+	}
+
 	driver := &DockerDriver{
 		conf:     conf,
 		docker:   newClient(),
 		hostname: hostname,
-		auths:    registryFromEnv(),
+		auths:    auths,
 	}
 
 	if conf.ServerVersion != "" {
@@ -131,23 +141,6 @@ func loadDockerImages(driver *DockerDriver, filePath string) error {
 	return driver.docker.LoadImages(ctx, filePath)
 }
 
-func registryFromEnv() map[string]docker.AuthConfiguration {
-	var auths *docker.AuthConfigurations
-	var err error
-	if reg := os.Getenv("DOCKER_AUTH"); reg != "" {
-		// TODO docker does not use this itself, we should get rid of env docker config (nor is this documented..)
-		auths, err = docker.NewAuthConfigurations(strings.NewReader(reg))
-	} else {
-		auths, err = docker.NewAuthConfigurationsFromDockerCfg()
-	}
-
-	if err != nil {
-		logrus.WithError(err).Info("no docker auths from config files found (this is fine)")
-		return nil
-	}
-	return auths.Configs
-}
-
 func (drv *DockerDriver) Close() error {
 	var err error
 	if drv.pool != nil {
@@ -304,63 +297,41 @@ func (drv *DockerDriver) removeContainer(ctx context.Context, container string)
 }
 
 func (drv *DockerDriver) ensureImage(ctx context.Context, task drivers.ContainerTask) error {
-	reg, _, _ := drivers.ParseImage(task.Image())
+	reg, repo, tag := drivers.ParseImage(task.Image())
 
 	// ask for docker creds before looking for image, as the tasker may need to
 	// validate creds even if the image is downloaded.
 
-	var config docker.AuthConfiguration // default, tries docker hub w/o user/pass
+	config := findRegistryConfig(reg, drv.auths)
-
-	// if any configured host auths match task registry, try them (task docker auth can override)
-	// TODO this is still a little hairy using suffix, we should probably try to parse it as a
-	// url and extract the host (from both the config file & image)
-	for _, v := range drv.auths {
-		if reg != "" && strings.HasSuffix(v.ServerAddress, reg) {
-			config = v
-			break
-		}
-	}
 
 	if task, ok := task.(Auther); ok {
 		var err error
 		_, span := trace.StartSpan(ctx, "docker_auth")
-		config, err = task.DockerAuth()
+		authConfig, err := task.DockerAuth()
 		span.End()
 		if err != nil {
 			return err
 		}
+		config = &authConfig
 	}
 
-	if reg != "" {
+	globalRepo := path.Join(reg, repo)
-		config.ServerAddress = reg
-	}
 
 	// see if we already have it, if not, pull it
 	_, err := drv.docker.InspectImage(ctx, task.Image())
 	if err == docker.ErrNoSuchImage {
-		err = drv.pullImage(ctx, task, config)
+		err = drv.pullImage(ctx, task, *config, globalRepo, tag)
 	}
 
 	return err
 }
 
-func (drv *DockerDriver) pullImage(ctx context.Context, task drivers.ContainerTask, config docker.AuthConfiguration) error {
+func (drv *DockerDriver) pullImage(ctx context.Context, task drivers.ContainerTask, config docker.AuthConfiguration, globalRepo, tag string) error {
 	log := common.Logger(ctx)
-	reg, repo, tag := drivers.ParseImage(task.Image())
-	globalRepo := path.Join(reg, repo)
-	if reg != "" {
-		config.ServerAddress = reg
-	}
-
-	var err error
-	config.ServerAddress, err = registryURL(config.ServerAddress)
-	if err != nil {
-		return err
-	}
 
 	log.WithFields(logrus.Fields{"registry": config.ServerAddress, "username": config.Username, "image": task.Image()}).Info("Pulling image")
 
-	err = drv.docker.PullImage(docker.PullImageOptions{Repository: globalRepo, Tag: tag, Context: ctx}, config)
+	err := drv.docker.PullImage(docker.PullImageOptions{Repository: globalRepo, Tag: tag, Context: ctx}, config)
 	if err != nil {
 		log.WithFields(logrus.Fields{"registry": config.ServerAddress, "username": config.Username, "image": task.Image()}).WithError(err).Error("Failed to pull image")
 

api/agent/drivers/docker/registry.go

@@ -2,26 +2,96 @@ package docker
 
 import (
 	"net/url"
+	"os"
 	"strings"
+
+	"github.com/fsouza/go-dockerclient"
+	"github.com/sirupsen/logrus"
 )
 
-const hubURL = "https://registry.hub.docker.com"
+var (
+	defaultPrivateRegistries = []string{"hub.docker.com", "index.docker.io"}
+)
 
-func registryURL(addr string) (string, error) {
+func registryFromEnv() (map[string]driverAuthConfig, error) {
-	if addr == "" || strings.Contains(addr, "hub.docker.com") || strings.Contains(addr, "index.docker.io") {
+	drvAuths := make(map[string]driverAuthConfig)
-		return hubURL, nil
+
+	var auths *docker.AuthConfigurations
+	var err error
+	if reg := os.Getenv("DOCKER_AUTH"); reg != "" {
+		// TODO docker does not use this itself, we should get rid of env docker config (nor is this documented..)
+		auths, err = docker.NewAuthConfigurations(strings.NewReader(reg))
+	} else {
+		auths, err = docker.NewAuthConfigurationsFromDockerCfg()
 	}
 
-	uri, err := url.Parse(addr)
 	if err != nil {
-		return "", err
+		logrus.WithError(err).Info("no docker auths from config files found (this is fine)")
+		return drvAuths, nil
 	}
 
-	if uri.Scheme == "" {
+	for key, v := range auths.Configs {
-		uri.Scheme = "https"
+
+		u, err := url.Parse(v.ServerAddress)
+		if err != nil {
+			return drvAuths, err
 		}
-	uri.Path = strings.TrimSuffix(uri.Path, "/")
+
-	uri.Path = strings.TrimPrefix(uri.Path, "/v2")
+		drvAuths[key] = driverAuthConfig{
-	uri.Path = strings.TrimPrefix(uri.Path, "/v1") // just try this, if it fails it fails, not supporting v1
+			auth:       v,
-	return uri.String(), nil
+			subdomains: getSubdomains(u.Host),
+		}
+	}
+
+	return drvAuths, nil
+}
+
+func getSubdomains(hostname string) map[string]bool {
+
+	subdomains := make(map[string]bool)
+	tokens := strings.Split(hostname, ".")
+
+	if len(tokens) <= 2 {
+		subdomains[hostname] = true
+	} else {
+		for i := 0; i <= len(tokens)-2; i++ {
+			joined := strings.Join(tokens[i:], ".")
+			subdomains[joined] = true
+		}
+	}
+
+	return subdomains
+}
+
+func findRegistryConfig(reg string, configs map[string]driverAuthConfig) *docker.AuthConfiguration {
+	var config docker.AuthConfiguration
+
+	if reg != "" {
+		res := lookupRegistryConfig(reg, configs)
+		if res != nil {
+			return res
+		}
+	} else {
+		for _, reg := range defaultPrivateRegistries {
+			res := lookupRegistryConfig(reg, configs)
+			if res != nil {
+				return res
+			}
+		}
+	}
+
+	return &config
+}
+
+func lookupRegistryConfig(reg string, configs map[string]driverAuthConfig) *docker.AuthConfiguration {
+
+	// if any configured host auths match task registry, try them (task docker auth can override)
+	for _, v := range configs {
+		_, ok := v.subdomains[reg]
+		if ok {
+			return &v.auth
+		}
+	}
+
+	return nil
 }

api/agent/drivers/docker/registry_test.go

@@ -0,0 +1,59 @@
+package docker
+
+import (
+	"testing"
+)
+
+func verify(expected []string, checks map[string]bool) bool {
+	if len(expected) != len(checks) {
+		return false
+	}
+	for _, v := range expected {
+		_, ok := checks[v]
+		if !ok {
+			return false
+		}
+	}
+	return true
+}
+
+func TestRegistrySubDomains(t *testing.T) {
+	var exp []string
+	var res map[string]bool
+
+	exp = []string{"google.com"}
+	res = getSubdomains("google.com")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+
+	exp = []string{"top.google.com", "google.com"}
+	res = getSubdomains("top.google.com")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+
+	exp = []string{"top.google.com:443", "google.com:443"}
+	res = getSubdomains("top.google.com:443")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+
+	exp = []string{"top.top.google.com", "top.google.com", "google.com"}
+	res = getSubdomains("top.top.google.com")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+
+	exp = []string{"docker"}
+	res = getSubdomains("docker")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+
+	exp = []string{""}
+	res = getSubdomains("")
+	if !verify(exp, res) {
+		t.Fatalf("subdomain results failed expected[%+v] != results[%+v]", exp, res)
+	}
+}