Address review comments

docs/cli/kops_create_cluster.md

@@ -76,7 +76,6 @@ kops create cluster [CLUSTER] [flags]
       --channel string                          Channel for default versions and configuration to use (default "stable")
       --cloud string                            Cloud provider to use - aws, digitalocean, gce, hetzner, openstack
       --cloud-labels string                     A list of key/value pairs used to tag all instance groups (for example "Owner=John Doe,Team=Some Team").
-      --container-runtime string                Container runtime to use: containerd, docker
       --control-plane-count int32               Number of control-plane nodes. Defaults to one control-plane node per control-plane-zone
       --control-plane-image string              Machine image for control-plane nodes. Takes precedence over --image
       --control-plane-security-groups strings   Additional pre-created security groups to add to control-plane nodes.

k8s/crds/kops.k8s.io_clusters.yaml

@@ -944,7 +944,7 @@ spec:
                   the zone.
                 type: string
               docker:
-                description: DockerConfig is the configuration for docker
+                description: Docker was removed.
                 properties:
                   authorizationPlugins:
                     description: AuthorizationPlugins is a list of authorization plugins

nodeup/pkg/model/containerd.go

@@ -190,9 +190,7 @@ func (b *ContainerdBuilder) buildSystemdService(sv semver.Version) *nodetasks.Se
 	manifest.Set("Service", "ExecStart", "/usr/bin/containerd -c "+containerdConfigFilePath+" \"$CONTAINERD_OPTS\"")
 
 	// notify the daemon's readiness to systemd
-	if sv.GTE(semver.MustParse("1.3.4")) {
-		manifest.Set("Service", "Type", "notify")
-	}
+	manifest.Set("Service", "Type", "notify")
 
 	// set delegate yes so that systemd does not reset the cgroups of containerd containers
 	manifest.Set("Service", "Delegate", "yes")

pkg/apis/kops/cluster.go

@@ -62,7 +62,6 @@ type ClusterSpec struct {
 	// GossipConfig for the cluster assuming the use of gossip DNS
 	GossipConfig *GossipConfig `json:"gossipConfig,omitempty"`
 	// ContainerRuntime was removed.
-	// +k8s:conversion-gen=false
 	ContainerRuntime string `json:"-"`
 	// The version of kubernetes to install (optional, and can be a "spec" like stable)
 	KubernetesVersion string `json:"kubernetesVersion,omitempty"`
@@ -96,11 +95,10 @@ type ClusterSpec struct {
 	FileAssets []FileAssetSpec `json:"fileAssets,omitempty"`
 	// EtcdClusters stores the configuration for each cluster
 	EtcdClusters []EtcdClusterSpec `json:"etcdClusters,omitempty"`
-	// Component configurations
-	Containerd *ContainerdConfig `json:"containerd,omitempty"`
 	// Docker was removed.
-	// +k8s:conversion-gen=false
-	Docker                         *DockerConfig                 `json:"-"`
+	Docker *DockerConfig `json:"-"`
+	// Component configurations
+	Containerd                     *ContainerdConfig             `json:"containerd,omitempty"`
 	KubeDNS                        *KubeDNSConfig                `json:"kubeDNS,omitempty"`
 	KubeAPIServer                  *KubeAPIServerConfig          `json:"kubeAPIServer,omitempty"`
 	KubeControllerManager          *KubeControllerManagerConfig  `json:"kubeControllerManager,omitempty"`

pkg/apis/kops/componentconfig.go

@@ -182,7 +182,6 @@ type KubeletConfigSpec struct {
 	// StreamingConnectionIdleTimeout is the maximum time a streaming connection can be idle before the connection is automatically closed
 	StreamingConnectionIdleTimeout *metav1.Duration `json:"streamingConnectionIdleTimeout,omitempty" flag:"streaming-connection-idle-timeout"`
 	// DockerDisableSharedPID was removed.
-	// +k8s:conversion-gen=false
 	DockerDisableSharedPID *bool `json:"-"`
 	// RootDir is the directory path for managing kubelet files (volume mounts,etc)
 	RootDir string `json:"rootDir,omitempty" flag:"root-dir"`

pkg/apis/kops/v1alpha2/cluster.go

@@ -167,9 +167,10 @@ type ClusterSpec struct {
 	FileAssets []FileAssetSpec `json:"fileAssets,omitempty"`
 	// EtcdClusters stores the configuration for each cluster
 	EtcdClusters []EtcdClusterSpec `json:"etcdClusters,omitempty"`
+	// Docker was removed.
+	Docker *DockerConfig `json:"docker,omitempty"`
 	// Component configurations
 	Containerd                     *ContainerdConfig             `json:"containerd,omitempty"`
-	Docker                         *DockerConfig                 `json:"docker,omitempty"`
 	KubeDNS                        *KubeDNSConfig                `json:"kubeDNS,omitempty"`
 	KubeAPIServer                  *KubeAPIServerConfig          `json:"kubeAPIServer,omitempty"`
 	KubeControllerManager          *KubeControllerManagerConfig  `json:"kubeControllerManager,omitempty"`

pkg/apis/kops/v1alpha2/zz_generated.conversion.go

@@ -2416,15 +2416,6 @@ func autoConvert_v1alpha2_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.EtcdClusters = nil
 	}
-	if in.Containerd != nil {
-		in, out := &in.Containerd, &out.Containerd
-		*out = new(kops.ContainerdConfig)
-		if err := Convert_v1alpha2_ContainerdConfig_To_kops_ContainerdConfig(*in, *out, s); err != nil {
-			return err
-		}
-	} else {
-		out.Containerd = nil
-	}
 	if in.Docker != nil {
 		in, out := &in.Docker, &out.Docker
 		*out = new(kops.DockerConfig)
@@ -2434,6 +2425,15 @@ func autoConvert_v1alpha2_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.Docker = nil
 	}
+	if in.Containerd != nil {
+		in, out := &in.Containerd, &out.Containerd
+		*out = new(kops.ContainerdConfig)
+		if err := Convert_v1alpha2_ContainerdConfig_To_kops_ContainerdConfig(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Containerd = nil
+	}
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(kops.KubeDNSConfig)
@@ -2714,7 +2714,7 @@ func autoConvert_kops_ClusterSpec_To_v1alpha2_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.GossipConfig = nil
 	}
-	// INFO: in.ContainerRuntime opted out of conversion generation
+	out.ContainerRuntime = in.ContainerRuntime
 	out.KubernetesVersion = in.KubernetesVersion
 	out.DNSZone = in.DNSZone
 	if in.DNSControllerGossipConfig != nil {
@@ -2755,6 +2755,15 @@ func autoConvert_kops_ClusterSpec_To_v1alpha2_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.EtcdClusters = nil
 	}
+	if in.Docker != nil {
+		in, out := &in.Docker, &out.Docker
+		*out = new(DockerConfig)
+		if err := Convert_kops_DockerConfig_To_v1alpha2_DockerConfig(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Docker = nil
+	}
 	if in.Containerd != nil {
 		in, out := &in.Containerd, &out.Containerd
 		*out = new(ContainerdConfig)
@@ -2764,7 +2773,6 @@ func autoConvert_kops_ClusterSpec_To_v1alpha2_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.Containerd = nil
 	}
-	// INFO: in.Docker opted out of conversion generation
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(KubeDNSConfig)
@@ -5470,7 +5478,7 @@ func autoConvert_kops_KubeletConfigSpec_To_v1alpha2_KubeletConfigSpec(in *kops.K
 	out.ExperimentalAllowedUnsafeSysctls = in.ExperimentalAllowedUnsafeSysctls
 	out.AllowedUnsafeSysctls = in.AllowedUnsafeSysctls
 	out.StreamingConnectionIdleTimeout = in.StreamingConnectionIdleTimeout
-	// INFO: in.DockerDisableSharedPID opted out of conversion generation
+	out.DockerDisableSharedPID = in.DockerDisableSharedPID
 	out.RootDir = in.RootDir
 	out.AuthenticationTokenWebhook = in.AuthenticationTokenWebhook
 	out.AuthenticationTokenWebhookCacheTTL = in.AuthenticationTokenWebhookCacheTTL

pkg/apis/kops/v1alpha2/zz_generated.deepcopy.go

@@ -1138,16 +1138,16 @@ func (in *ClusterSpec) DeepCopyInto(out *ClusterSpec) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
-	if in.Containerd != nil {
-		in, out := &in.Containerd, &out.Containerd
-		*out = new(ContainerdConfig)
-		(*in).DeepCopyInto(*out)
-	}
 	if in.Docker != nil {
 		in, out := &in.Docker, &out.Docker
 		*out = new(DockerConfig)
 		(*in).DeepCopyInto(*out)
 	}
+	if in.Containerd != nil {
+		in, out := &in.Containerd, &out.Containerd
+		*out = new(ContainerdConfig)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(KubeDNSConfig)

pkg/apis/kops/v1alpha3/cluster.go

@@ -58,7 +58,6 @@ type ClusterSpec struct {
 	// GossipConfig for the cluster assuming the use of gossip DNS
 	GossipConfig *GossipConfig `json:"gossipConfig,omitempty"`
 	// ContainerRuntime was removed.
-	// +k8s:conversion-gen=false
 	ContainerRuntime string `json:"-"`
 	// The version of kubernetes to install (optional, and can be a "spec" like stable)
 	KubernetesVersion string `json:"kubernetesVersion,omitempty"`
@@ -93,11 +92,10 @@ type ClusterSpec struct {
 	FileAssets []FileAssetSpec `json:"fileAssets,omitempty"`
 	// EtcdClusters stores the configuration for each cluster
 	EtcdClusters []EtcdClusterSpec `json:"etcdClusters,omitempty"`
-	// Component configurations
-	Containerd *ContainerdConfig `json:"containerd,omitempty"`
 	// Docker was removed.
-	// +k8s:conversion-gen=false
-	Docker                         *DockerConfig                 `json:"-"`
+	Docker *DockerConfig `json:"-"`
+	// Component configurations
+	Containerd                     *ContainerdConfig             `json:"containerd,omitempty"`
 	KubeDNS                        *KubeDNSConfig                `json:"kubeDNS,omitempty"`
 	KubeAPIServer                  *KubeAPIServerConfig          `json:"kubeAPIServer,omitempty"`
 	KubeControllerManager          *KubeControllerManagerConfig  `json:"kubeControllerManager,omitempty"`

pkg/apis/kops/v1alpha3/componentconfig.go

@@ -180,7 +180,6 @@ type KubeletConfigSpec struct {
 	// StreamingConnectionIdleTimeout is the maximum time a streaming connection can be idle before the connection is automatically closed
 	StreamingConnectionIdleTimeout *metav1.Duration `json:"streamingConnectionIdleTimeout,omitempty" flag:"streaming-connection-idle-timeout"`
 	// DockerDisableSharedPID was removed.
-	// +k8s:conversion-gen=false
 	DockerDisableSharedPID *bool `json:"-"`
 	// RootDir is the directory path for managing kubelet files (volume mounts,etc)
 	RootDir string `json:"rootDir,omitempty" flag:"root-dir"`

pkg/apis/kops/v1alpha3/zz_generated.conversion.go

@@ -2605,7 +2605,7 @@ func autoConvert_v1alpha3_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.GossipConfig = nil
 	}
-	// INFO: in.ContainerRuntime opted out of conversion generation
+	out.ContainerRuntime = in.ContainerRuntime
 	out.KubernetesVersion = in.KubernetesVersion
 	out.DNSZone = in.DNSZone
 	if in.DNSControllerGossipConfig != nil {
@@ -2646,6 +2646,15 @@ func autoConvert_v1alpha3_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.EtcdClusters = nil
 	}
+	if in.Docker != nil {
+		in, out := &in.Docker, &out.Docker
+		*out = new(kops.DockerConfig)
+		if err := Convert_v1alpha3_DockerConfig_To_kops_DockerConfig(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Docker = nil
+	}
 	if in.Containerd != nil {
 		in, out := &in.Containerd, &out.Containerd
 		*out = new(kops.ContainerdConfig)
@@ -2655,7 +2664,6 @@ func autoConvert_v1alpha3_ClusterSpec_To_kops_ClusterSpec(in *ClusterSpec, out *
 	} else {
 		out.Containerd = nil
 	}
-	// INFO: in.Docker opted out of conversion generation
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(kops.KubeDNSConfig)
@@ -2930,7 +2938,7 @@ func autoConvert_kops_ClusterSpec_To_v1alpha3_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.GossipConfig = nil
 	}
-	// INFO: in.ContainerRuntime opted out of conversion generation
+	out.ContainerRuntime = in.ContainerRuntime
 	out.KubernetesVersion = in.KubernetesVersion
 	out.DNSZone = in.DNSZone
 	if in.DNSControllerGossipConfig != nil {
@@ -2971,6 +2979,15 @@ func autoConvert_kops_ClusterSpec_To_v1alpha3_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.EtcdClusters = nil
 	}
+	if in.Docker != nil {
+		in, out := &in.Docker, &out.Docker
+		*out = new(DockerConfig)
+		if err := Convert_kops_DockerConfig_To_v1alpha3_DockerConfig(*in, *out, s); err != nil {
+			return err
+		}
+	} else {
+		out.Docker = nil
+	}
 	if in.Containerd != nil {
 		in, out := &in.Containerd, &out.Containerd
 		*out = new(ContainerdConfig)
@@ -2980,7 +2997,6 @@ func autoConvert_kops_ClusterSpec_To_v1alpha3_ClusterSpec(in *kops.ClusterSpec,
 	} else {
 		out.Containerd = nil
 	}
-	// INFO: in.Docker opted out of conversion generation
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(KubeDNSConfig)
@@ -5752,7 +5768,7 @@ func autoConvert_v1alpha3_KubeletConfigSpec_To_kops_KubeletConfigSpec(in *Kubele
 	out.ExperimentalAllowedUnsafeSysctls = in.ExperimentalAllowedUnsafeSysctls
 	out.AllowedUnsafeSysctls = in.AllowedUnsafeSysctls
 	out.StreamingConnectionIdleTimeout = in.StreamingConnectionIdleTimeout
-	// INFO: in.DockerDisableSharedPID opted out of conversion generation
+	out.DockerDisableSharedPID = in.DockerDisableSharedPID
 	out.RootDir = in.RootDir
 	out.AuthenticationTokenWebhook = in.AuthenticationTokenWebhook
 	out.AuthenticationTokenWebhookCacheTTL = in.AuthenticationTokenWebhookCacheTTL
@@ -5853,7 +5869,7 @@ func autoConvert_kops_KubeletConfigSpec_To_v1alpha3_KubeletConfigSpec(in *kops.K
 	out.ExperimentalAllowedUnsafeSysctls = in.ExperimentalAllowedUnsafeSysctls
 	out.AllowedUnsafeSysctls = in.AllowedUnsafeSysctls
 	out.StreamingConnectionIdleTimeout = in.StreamingConnectionIdleTimeout
-	// INFO: in.DockerDisableSharedPID opted out of conversion generation
+	out.DockerDisableSharedPID = in.DockerDisableSharedPID
 	out.RootDir = in.RootDir
 	out.AuthenticationTokenWebhook = in.AuthenticationTokenWebhook
 	out.AuthenticationTokenWebhookCacheTTL = in.AuthenticationTokenWebhookCacheTTL

pkg/apis/kops/v1alpha3/zz_generated.deepcopy.go

@@ -1063,16 +1063,16 @@ func (in *ClusterSpec) DeepCopyInto(out *ClusterSpec) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
-	if in.Containerd != nil {
-		in, out := &in.Containerd, &out.Containerd
-		*out = new(ContainerdConfig)
-		(*in).DeepCopyInto(*out)
-	}
 	if in.Docker != nil {
 		in, out := &in.Docker, &out.Docker
 		*out = new(DockerConfig)
 		(*in).DeepCopyInto(*out)
 	}
+	if in.Containerd != nil {
+		in, out := &in.Containerd, &out.Containerd
+		*out = new(ContainerdConfig)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(KubeDNSConfig)

pkg/apis/kops/validation/validation.go

@@ -188,10 +188,18 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie
 		}
 	}
 
+	if spec.ContainerRuntime != "" {
+		allErrs = append(allErrs, validateContainerRuntime(c, spec.ContainerRuntime, fieldPath.Child("containerRuntime"))...)
+	}
+
 	if spec.Containerd != nil {
 		allErrs = append(allErrs, validateContainerdConfig(spec, spec.Containerd, fieldPath.Child("containerd"), true)...)
 	}
 
+	if spec.Docker != nil {
+		allErrs = append(allErrs, field.Forbidden(fieldPath.Child("docker"), "Docker CRI support was removed in Kubernetes 1.24: https://kubernetes.io/blog/2020/12/02/dockershim-faq"))
+	}
+
 	if spec.Assets != nil {
 		if spec.Assets.ContainerProxy != nil && spec.Assets.ContainerRegistry != nil {
 			allErrs = append(allErrs, field.Forbidden(fieldPath.Child("assets", "containerProxy"), "containerProxy cannot be used in conjunction with containerRegistry"))
@@ -1644,6 +1652,19 @@ func validateCalicoEncapsulationMode(mode string, fldPath *field.Path) field.Err
 	return allErrs
 }
 
+func validateContainerRuntime(c *kops.Cluster, runtime string, fldPath *field.Path) field.ErrorList {
+	valid := []string{"containerd", "docker"}
+
+	allErrs := field.ErrorList{}
+	allErrs = append(allErrs, IsValidValue(fldPath, &runtime, valid)...)
+
+	if runtime == "docker" {
+		allErrs = append(allErrs, field.Forbidden(fldPath, "Docker CRI support was removed in Kubernetes 1.24: https://kubernetes.io/blog/2020/12/02/dockershim-faq"))
+	}
+
+	return allErrs
+}
+
 func validateContainerdConfig(spec *kops.ClusterSpec, config *kops.ContainerdConfig, fldPath *field.Path, inClusterConfig bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 

pkg/apis/kops/zz_generated.deepcopy.go

@@ -1160,16 +1160,16 @@ func (in *ClusterSpec) DeepCopyInto(out *ClusterSpec) {
 			(*in)[i].DeepCopyInto(&(*out)[i])
 		}
 	}
-	if in.Containerd != nil {
-		in, out := &in.Containerd, &out.Containerd
-		*out = new(ContainerdConfig)
-		(*in).DeepCopyInto(*out)
-	}
 	if in.Docker != nil {
 		in, out := &in.Docker, &out.Docker
 		*out = new(DockerConfig)
 		(*in).DeepCopyInto(*out)
 	}
+	if in.Containerd != nil {
+		in, out := &in.Containerd, &out.Containerd
+		*out = new(ContainerdConfig)
+		(*in).DeepCopyInto(*out)
+	}
 	if in.KubeDNS != nil {
 		in, out := &in.KubeDNS, &out.KubeDNS
 		*out = new(KubeDNSConfig)

upup/pkg/fi/cloudup/apply_cluster.go

@@ -1080,24 +1080,22 @@ func (c *ApplyClusterCmd) addFileAssets(assetBuilder *assets.AssetBuilder) error
 			c.Assets[arch] = append(c.Assets[arch], mirrors.BuildMirroredAsset(cniAsset, cniAssetHash))
 		}
 
-		if c.Cluster.Spec.Containerd != nil && c.Cluster.Spec.Containerd.SkipInstall {
-			break
-		}
+		if c.Cluster.Spec.Containerd == nil || !c.Cluster.Spec.Containerd.SkipInstall {
+			containerdAssetUrl, containerdAssetHash, err := findContainerdAsset(c.Cluster, assetBuilder, arch)
+			if err != nil {
+				return err
+			}
+			if containerdAssetUrl != nil && containerdAssetHash != nil {
+				c.Assets[arch] = append(c.Assets[arch], mirrors.BuildMirroredAsset(containerdAssetUrl, containerdAssetHash))
+			}
 
-		containerdAssetUrl, containerdAssetHash, err := findContainerdAsset(c.Cluster, assetBuilder, arch)
-		if err != nil {
-			return err
-		}
-		if containerdAssetUrl != nil && containerdAssetHash != nil {
-			c.Assets[arch] = append(c.Assets[arch], mirrors.BuildMirroredAsset(containerdAssetUrl, containerdAssetHash))
-		}
-
-		runcAssetUrl, runcAssetHash, err := findRuncAsset(c.Cluster, assetBuilder, arch)
-		if err != nil {
-			return err
-		}
-		if runcAssetUrl != nil && runcAssetHash != nil {
-			c.Assets[arch] = append(c.Assets[arch], mirrors.BuildMirroredAsset(runcAssetUrl, runcAssetHash))
+			runcAssetUrl, runcAssetHash, err := findRuncAsset(c.Cluster, assetBuilder, arch)
+			if err != nil {
+				return err
+			}
+			if runcAssetUrl != nil && runcAssetHash != nil {
+				c.Assets[arch] = append(c.Assets[arch], mirrors.BuildMirroredAsset(runcAssetUrl, runcAssetHash))
+			}
 		}
 
 		asset, err := NodeUpAsset(assetBuilder, arch)

upup/pkg/fi/nodeup/nodetasks/load_image.go

@@ -88,11 +88,6 @@ func (_ *LoadImageTask) CheckChanges(a, e, changes *LoadImageTask) error {
 }
 
 func (_ *LoadImageTask) RenderLocal(t *local.LocalTarget, a, e, changes *LoadImageTask) error {
-	runtime := e.Runtime
-	if runtime != "docker" && runtime != "containerd" {
-		return fmt.Errorf("no runtime specified")
-	}
-
 	hash, err := hashing.FromString(e.Hash)
 	if err != nil {
 		return err
@@ -148,14 +143,7 @@ func (_ *LoadImageTask) RenderLocal(t *local.LocalTarget, a, e, changes *LoadIma
 
 	// Load the container image
 	var args []string
-	switch runtime {
-	case "docker":
-		args = []string{"docker", "load", "-i", tarFile}
-	case "containerd":
-		args = []string{"ctr", "--namespace", "k8s.io", "images", "import", tarFile}
-	default:
-		return fmt.Errorf("unknown container runtime: %s", runtime)
-	}
+	args = []string{"ctr", "--namespace", "k8s.io", "images", "import", tarFile}
 	human := strings.Join(args, " ")
 
 	klog.Infof("running command %s", human)