Improve generation for container runtime configuration (#1174)

* Improve handling for container runtime configuration

Signed-off-by: Waleed Malik <[email protected]>

* Update fixtures

Signed-off-by: Waleed Malik <[email protected]>

* Fix linting errors

* Handle PR feedback

Signed-off-by: Waleed Malik <[email protected]>

Author: ahmedwaleedmalik

cmd/machine-controller/main.go

@@ -23,7 +23,6 @@ import (
 	"net"
 	"net/http"
 	"net/http/pprof"
-	"net/url"
 	"strings"
 	"time"
 
@@ -84,7 +83,7 @@ var (
 	podCidr                       string
 	nodePortRange                 string
 	nodeRegistryCredentialsSecret string
-	nodeContainerdRegistryMirrors = registryMirrorsFlags{}
+	nodeContainerdRegistryMirrors = containerruntime.RegistryMirrorsFlags{}
 )
 
 const (
@@ -237,37 +236,17 @@ func main() {
 	ctrlMetrics := machinecontroller.NewMachineControllerMetrics()
 	ctrlMetrics.MustRegister(metrics.Registry)
 
-	var insecureRegistries []string
-	for _, registry := range strings.Split(nodeInsecureRegistries, ",") {
-		if trimmedRegistry := strings.TrimSpace(registry); trimmedRegistry != "" {
-			insecureRegistries = append(insecureRegistries, trimmedRegistry)
-		}
-	}
-
-	var registryMirrors []string
-	for _, mirror := range strings.Split(nodeRegistryMirrors, ",") {
-		if trimmedMirror := strings.TrimSpace(mirror); trimmedMirror != "" {
-			if !strings.HasPrefix(mirror, "http") {
-				trimmedMirror = "https://" + mirror
-			}
-
-			_, err := url.Parse(trimmedMirror)
-			if err != nil {
-				klog.Fatalf("incorrect mirror provided: %v", err)
-			}
-
-			registryMirrors = append(registryMirrors, trimmedMirror)
-		}
+	containerRuntimeOpts := containerruntime.Opts{
+		ContainerRuntime:          nodeContainerRuntime,
+		ContainerdRegistryMirrors: nodeContainerdRegistryMirrors,
+		InsecureRegistries:        nodeInsecureRegistries,
+		PauseImage:                nodePauseImage,
+		RegistryMirrors:           nodeRegistryMirrors,
+		RegistryCredentialsSecret: nodeRegistryCredentialsSecret,
 	}
-
-	if len(registryMirrors) > 0 {
-		nodeContainerdRegistryMirrors["docker.io"] = registryMirrors
-	}
-
-	if nodeRegistryCredentialsSecret != "" {
-		if secRef := strings.Split(nodeRegistryCredentialsSecret, "/"); len(secRef) != 2 {
-			klog.Fatalf("-node-registry-credentials-secret is in incorrect format %q, should be in 'namespace/secretname'", nodeRegistryCredentialsSecret)
-		}
+	containerRuntimeConfig, err := containerruntime.BuildConfig(containerRuntimeOpts)
+	if err != nil {
+		klog.Fatalf("failed to generate container runtime config: %v", err)
 	}
 
 	runOptions := controllerRunOptions{
@@ -285,12 +264,7 @@ func main() {
 			NoProxy:                      nodeNoProxy,
 			PauseImage:                   nodePauseImage,
 			RegistryCredentialsSecretRef: nodeRegistryCredentialsSecret,
-			ContainerRuntime: containerruntime.Get(
-				nodeContainerRuntime,
-				containerruntime.WithInsecureRegistries(insecureRegistries),
-				containerruntime.WithRegistryMirrors(nodeContainerdRegistryMirrors),
-				containerruntime.WithSandboxImage(nodePauseImage),
-			),
+			ContainerRuntime:             containerRuntimeConfig,
 		},
 		useOSM:        useOSM,
 		podCidr:       podCidr,

pkg/apis/cluster/v1alpha1/zz_generated.deepcopy.go

@@ -0,0 +1,105 @@
+/*
+Copyright 2022 The Machine Controller Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package containerruntime
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/url"
+	"strings"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
+	ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+type Opts struct {
+	ContainerRuntime          string
+	InsecureRegistries        string
+	RegistryMirrors           string
+	RegistryCredentialsSecret string
+	PauseImage                string
+	ContainerdRegistryMirrors RegistryMirrorsFlags
+}
+
+func BuildConfig(opts Opts) (Config, error) {
+	var insecureRegistries []string
+	for _, registry := range strings.Split(opts.InsecureRegistries, ",") {
+		if trimmedRegistry := strings.TrimSpace(registry); trimmedRegistry != "" {
+			insecureRegistries = append(insecureRegistries, trimmedRegistry)
+		}
+	}
+
+	var registryMirrors []string
+	for _, mirror := range strings.Split(opts.RegistryMirrors, ",") {
+		if trimmedMirror := strings.TrimSpace(mirror); trimmedMirror != "" {
+			if !strings.HasPrefix(mirror, "http") {
+				trimmedMirror = "https://" + mirror
+			}
+
+			_, err := url.Parse(trimmedMirror)
+			if err != nil {
+				return Config{}, fmt.Errorf("incorrect mirror provided: %v", err)
+			}
+
+			registryMirrors = append(registryMirrors, trimmedMirror)
+		}
+	}
+
+	if len(registryMirrors) > 0 {
+		if opts.ContainerdRegistryMirrors == nil {
+			opts.ContainerdRegistryMirrors = make(RegistryMirrorsFlags)
+		}
+		opts.ContainerdRegistryMirrors["docker.io"] = registryMirrors
+	}
+
+	// Only validate registry credential here
+	if opts.RegistryCredentialsSecret != "" {
+		if secRef := strings.Split(opts.RegistryCredentialsSecret, "/"); len(secRef) != 2 {
+			return Config{}, fmt.Errorf("-node-registry-credentials-secret is in incorrect format %q, should be in 'namespace/secretname'", opts.RegistryCredentialsSecret)
+		}
+	}
+
+	return get(
+		opts.ContainerRuntime,
+		withInsecureRegistries(insecureRegistries),
+		withRegistryMirrors(opts.ContainerdRegistryMirrors),
+		withSandboxImage(opts.PauseImage),
+	), nil
+}
+
+func GetContainerdAuthConfig(ctx context.Context, client ctrlruntimeclient.Client, registryCredentialsSecret string) (map[string]AuthConfig, error) {
+	registryCredentials := map[string]AuthConfig{}
+
+	if secRef := strings.SplitN(registryCredentialsSecret, "/", 2); len(secRef) == 2 {
+		var credsSecret corev1.Secret
+		err := client.Get(ctx, types.NamespacedName{Namespace: secRef[0], Name: secRef[1]}, &credsSecret)
+		if err != nil {
+			return nil, fmt.Errorf("failed to retrieve registry credentials secret object: %w", err)
+		}
+
+		for registry, data := range credsSecret.Data {
+			var regCred AuthConfig
+			if err := json.Unmarshal(data, &regCred); err != nil {
+				return nil, fmt.Errorf("failed to unmarshal registry credentials: %w", err)
+			}
+			registryCredentials[registry] = regCred
+		}
+	}
+	return registryCredentials, nil
+}

pkg/containerruntime/config.go

@@ -37,31 +37,25 @@ type Engine interface {
 
 type Opt func(*Config)
 
-func WithInsecureRegistries(registries []string) Opt {
+func withInsecureRegistries(registries []string) Opt {
 	return func(cfg *Config) {
 		cfg.InsecureRegistries = registries
 	}
 }
 
-func WithRegistryMirrors(mirrors map[string][]string) Opt {
+func withRegistryMirrors(mirrors map[string][]string) Opt {
 	return func(cfg *Config) {
 		cfg.RegistryMirrors = mirrors
 	}
 }
 
-func WithRegistryCredentials(auth map[string]AuthConfig) Opt {
-	return func(cfg *Config) {
-		cfg.RegistryCredentials = auth
-	}
-}
-
-func WithSandboxImage(image string) Opt {
+func withSandboxImage(image string) Opt {
 	return func(cfg *Config) {
 		cfg.SandboxImage = image
 	}
 }
 
-func Get(containerRuntimeName string, opts ...Opt) Config {
+func get(containerRuntimeName string, opts ...Opt) Config {
 	cfg := Config{}
 
 	switch containerRuntimeName {

pkg/containerruntime/containerruntime.go

@@ -1,5 +1,5 @@
 /*
-Copyright 2019 The Machine Controller Authors.
+Copyright 2022 The Machine Controller Authors.
 
 Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
@@ -14,17 +14,17 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package main
+package containerruntime
 
 import (
 	"fmt"
 	"sort"
 	"strings"
 )
 
-type registryMirrorsFlags map[string][]string
+type RegistryMirrorsFlags map[string][]string
 
-func (fl registryMirrorsFlags) Set(val string) error {
+func (fl RegistryMirrorsFlags) Set(val string) error {
 	split := strings.SplitN(val, "=", 2)
 	if len(split) != 2 {
 		return fmt.Errorf("should have exactly 1 =")
@@ -38,7 +38,7 @@ func (fl registryMirrorsFlags) Set(val string) error {
 	return nil
 }
 
-func (fl registryMirrorsFlags) String() string {
+func (fl RegistryMirrorsFlags) String() string {
 	var (
 		registryNames []string
 		result        []string

cmd/machine-controller/custom_flags.go renamed to pkg/containerruntime/flags.go

@@ -18,7 +18,6 @@ package controller
 
 import (
 	"context"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"net"
@@ -726,22 +725,9 @@ func (r *Reconciler) ensureInstanceExistsForMachine(
 				externalCloudProvider, _ = strconv.ParseBool(val)
 			}
 
-			registryCredentials := map[string]containerruntime.AuthConfig{}
-
-			if secRef := strings.SplitN(r.nodeSettings.RegistryCredentialsSecretRef, "/", 2); len(secRef) == 2 {
-				var credsSecret corev1.Secret
-				err := r.client.Get(ctx, types.NamespacedName{Namespace: secRef[0], Name: secRef[1]}, &credsSecret)
-				if err != nil {
-					return nil, fmt.Errorf("failed to retrieve registry credentials secret object: %w", err)
-				}
-
-				for registry, data := range credsSecret.Data {
-					var regCred containerruntime.AuthConfig
-					if err := json.Unmarshal(data, &regCred); err != nil {
-						return nil, fmt.Errorf("failed to unmarshal registry credentials: %w", err)
-					}
-					registryCredentials[registry] = regCred
-				}
+			registryCredentials, err := containerruntime.GetContainerdAuthConfig(ctx, r.client, r.nodeSettings.RegistryCredentialsSecretRef)
+			if err != nil {
+				return nil, fmt.Errorf("failed to get containerd auth config: %v", err)
 			}
 
 			crRuntime := r.nodeSettings.ContainerRuntime

pkg/controller/machine/machine_controller.go

@@ -87,8 +87,8 @@ type userDataTestCase struct {
 	externalCloudProvider bool
 	httpProxy             string
 	noProxy               string
-	insecureRegistries    []string
-	registryMirrors       map[string][]string
+	insecureRegistries    string
+	registryMirrors       string
 	pauseImage            string
 	containerruntime      string
 }
@@ -158,7 +158,7 @@ func TestUserDataGeneration(t *testing.T) {
 			cloudProviderName:  stringPtr("vsphere"),
 			httpProxy:          "http://192.168.100.100:3128",
 			noProxy:            "192.168.1.0",
-			insecureRegistries: []string{"192.168.100.100:5000", "10.0.0.1:5000"},
+			insecureRegistries: "192.168.100.100:5000, 10.0.0.1:5000",
 			pauseImage:         "192.168.100.100:5000/kubernetes/pause:v3.1",
 		},
 		{
@@ -172,7 +172,7 @@ func TestUserDataGeneration(t *testing.T) {
 			cloudProviderName: stringPtr("vsphere"),
 			httpProxy:         "http://192.168.100.100:3128",
 			noProxy:           "192.168.1.0",
-			registryMirrors:   map[string][]string{"docker.io": {"https://registry.docker-cn.com"}},
+			registryMirrors:   "https://registry.docker-cn.com",
 			pauseImage:        "192.168.100.100:5000/kubernetes/pause:v3.1",
 		},
 		{
@@ -240,6 +240,16 @@ func TestUserDataGeneration(t *testing.T) {
 				t.Fatalf("failed to get cloud config: %v", err)
 			}
 
+			containerRuntimeOpts := containerruntime.Opts{
+				ContainerRuntime:   test.containerruntime,
+				InsecureRegistries: test.insecureRegistries,
+				RegistryMirrors:    test.registryMirrors,
+			}
+			containerRuntimeConfig, err := containerruntime.BuildConfig(containerRuntimeOpts)
+			if err != nil {
+				t.Fatalf("failed to generate container runtime config: %v", err)
+			}
+
 			req := plugin.UserDataRequest{
 				MachineSpec:              test.spec,
 				Kubeconfig:               kubeconfig,
@@ -252,11 +262,7 @@ func TestUserDataGeneration(t *testing.T) {
 				NoProxy:                  test.noProxy,
 				PauseImage:               test.pauseImage,
 				KubeletFeatureGates:      kubeletFeatureGates,
-				ContainerRuntime: containerruntime.Get(
-					test.containerruntime,
-					containerruntime.WithInsecureRegistries(test.insecureRegistries),
-					containerruntime.WithRegistryMirrors(test.registryMirrors),
-				),
+				ContainerRuntime:         containerRuntimeConfig,
 			}
 
 			s, err := provider.UserData(req)