kubeadm: Fix omitempty in v1beta2

There are a couple of problems with regards to the `omitempty` in v1beta1:

- It is not applied to certain fields. This makes emitting YAML configuration
  files in v1beta1 config format verbose by both kubeadm and third party Go
  lang tools. Certain fields, that were never given an explicit value would
  show up in the marshalled YAML document. This can cause confusion and even
  misconfiguration.

- It can be used in inappropriate places. In this case it's used for fields,
  that need to be always serialized. The only one such field at the moment is
  `NodeRegistrationOptions.Taints`. If the `Taints` field is nil, then it's
  defaulted to a slice containing a single control plane node taint. If it's
  an empty slice, no taints are applied, thus, the cluster behaves differently.
  With that in mind, a Go program, that uses v1beta1 with `omitempty` on the
  `Taints` field has no way to specify an explicit empty slice of taints, as
  this would get lost after marshalling to YAML.

To fix these issues the following is done in this change:

- A whole bunch of additional omitemptys are placed at many fields in v1beta2.
- `omitempty` is removed from `NodeRegistrationOptions.Taints`
- A test, that verifies the ability to specify empty slice value for `Taints`
  is included.

Signed-off-by: Rostislav M. Georgiev <rostislavg@vmware.com>

cmd/kubeadm/app/apis/kubeadm/v1beta2/types.go

@@ -61,13 +61,13 @@ type ClusterConfiguration struct {
 	metav1.TypeMeta `json:",inline"`
 
 	// Etcd holds configuration for etcd.
-	Etcd Etcd `json:"etcd"`
+	Etcd Etcd `json:"etcd,omitempty"`
 
 	// Networking holds configuration for the networking topology of the cluster.
-	Networking Networking `json:"networking"`
+	Networking Networking `json:"networking,omitempty"`
 
 	// KubernetesVersion is the target version of the control plane.
-	KubernetesVersion string `json:"kubernetesVersion"`
+	KubernetesVersion string `json:"kubernetesVersion,omitempty"`
 
 	// ControlPlaneEndpoint sets a stable IP address or DNS name for the control plane; it
 	// can be a valid IP address or a RFC-1123 DNS subdomain, both with optional TCP port.
@@ -80,7 +80,7 @@ type ClusterConfiguration struct {
 	// control plane instances.
 	// e.g.  in environments with enforced node recycling, the ControlPlaneEndpoint
 	// could be used for assigning a stable DNS to the control plane.
-	ControlPlaneEndpoint string `json:"controlPlaneEndpoint"`
+	ControlPlaneEndpoint string `json:"controlPlaneEndpoint,omitempty"`
 
 	// APIServer contains extra settings for the API server control plane component
 	APIServer APIServer `json:"apiServer,omitempty"`
@@ -92,16 +92,16 @@ type ClusterConfiguration struct {
 	Scheduler ControlPlaneComponent `json:"scheduler,omitempty"`
 
 	// DNS defines the options for the DNS add-on installed in the cluster.
-	DNS DNS `json:"dns"`
+	DNS DNS `json:"dns,omitempty"`
 
 	// CertificatesDir specifies where to store or look for all required certificates.
-	CertificatesDir string `json:"certificatesDir"`
+	CertificatesDir string `json:"certificatesDir,omitempty"`
 
 	// ImageRepository sets the container registry to pull images from.
 	// If empty, `k8s.gcr.io` will be used by default; in case of kubernetes version is a CI build (kubernetes version starts with `ci/` or `ci-cross/`)
 	// `gcr.io/kubernetes-ci-images` will be used as a default for control plane components and for kube-proxy, while `k8s.gcr.io`
 	// will be used for all the other images.
-	ImageRepository string `json:"imageRepository"`
+	ImageRepository string `json:"imageRepository,omitempty"`
 
 	// UseHyperKubeImage controls if hyperkube should be used for Kubernetes components instead of their respective separate images
 	UseHyperKubeImage bool `json:"useHyperKubeImage,omitempty"`
@@ -184,11 +184,11 @@ type ClusterStatus struct {
 // APIEndpoint struct contains elements of API server instance deployed on a node.
 type APIEndpoint struct {
 	// AdvertiseAddress sets the IP address for the API server to advertise.
-	AdvertiseAddress string `json:"advertiseAddress"`
+	AdvertiseAddress string `json:"advertiseAddress,omitempty"`
 
 	// BindPort sets the secure port for the API Server to bind to.
 	// Defaults to 6443.
-	BindPort int32 `json:"bindPort"`
+	BindPort int32 `json:"bindPort,omitempty"`
 }
 
 // NodeRegistrationOptions holds fields that relate to registering a new control-plane or node to the cluster, either via "kubeadm init" or "kubeadm join"
@@ -205,7 +205,7 @@ type NodeRegistrationOptions struct {
 	// Taints specifies the taints the Node API object should be registered with. If this field is unset, i.e. nil, in the `kubeadm init` process
 	// it will be defaulted to []v1.Taint{'node-role.kubernetes.io/master=""'}. If you don't want to taint your control-plane node, set this field to an
 	// empty slice, i.e. `taints: {}` in the YAML file. This field is solely used for Node registration.
-	Taints []v1.Taint `json:"taints,omitempty"`
+	Taints []v1.Taint `json:"taints"`
 
 	// KubeletExtraArgs passes through extra arguments to the kubelet. The arguments here are passed to the kubelet command line via the environment file
 	// kubeadm writes at runtime for the kubelet to source. This overrides the generic base-level configuration in the kubelet-config-1.X ConfigMap
@@ -216,11 +216,11 @@ type NodeRegistrationOptions struct {
 // Networking contains elements describing cluster's networking configuration
 type Networking struct {
 	// ServiceSubnet is the subnet used by k8s services. Defaults to "10.96.0.0/12".
-	ServiceSubnet string `json:"serviceSubnet"`
+	ServiceSubnet string `json:"serviceSubnet,omitempty"`
 	// PodSubnet is the subnet used by pods.
-	PodSubnet string `json:"podSubnet"`
+	PodSubnet string `json:"podSubnet,omitempty"`
 	// DNSDomain is the dns domain used by k8s services. Defaults to "cluster.local".
-	DNSDomain string `json:"dnsDomain"`
+	DNSDomain string `json:"dnsDomain,omitempty"`
 }
 
 // BootstrapToken describes one bootstrap token, stored as a Secret in the cluster
@@ -302,12 +302,12 @@ type JoinConfiguration struct {
 	metav1.TypeMeta `json:",inline"`
 
 	// NodeRegistration holds fields that relate to registering the new control-plane node to the cluster
-	NodeRegistration NodeRegistrationOptions `json:"nodeRegistration"`
+	NodeRegistration NodeRegistrationOptions `json:"nodeRegistration,omitempty"`
 
 	// CACertPath is the path to the SSL certificate authority used to
 	// secure comunications between node and control-plane.
 	// Defaults to "/etc/kubernetes/pki/ca.crt".
-	CACertPath string `json:"caCertPath"`
+	CACertPath string `json:"caCertPath,omitempty"`
 
 	// Discovery specifies the options for the kubelet to use during the TLS Bootstrap process
 	Discovery Discovery `json:"discovery"`
@@ -336,7 +336,7 @@ type Discovery struct {
 	// TLSBootstrapToken is a token used for TLS bootstrapping.
 	// If .BootstrapToken is set, this field is defaulted to .BootstrapToken.Token, but can be overridden.
 	// If .File is set, this field **must be set** in case the KubeConfigFile does not contain any other authentication information
-	TLSBootstrapToken string `json:"tlsBootstrapToken"`
+	TLSBootstrapToken string `json:"tlsBootstrapToken,omitempty"`
 
 	// Timeout modifies the discovery timeout
 	Timeout *metav1.Duration `json:"timeout,omitempty"`
@@ -364,7 +364,7 @@ type BootstrapTokenDiscovery struct {
 	// UnsafeSkipCAVerification allows token-based discovery
 	// without CA verification via CACertHashes. This can weaken
 	// the security of kubeadm since other nodes can impersonate the control-plane.
-	UnsafeSkipCAVerification bool `json:"unsafeSkipCAVerification"`
+	UnsafeSkipCAVerification bool `json:"unsafeSkipCAVerification,omitempty"`
 }
 
 // FileDiscovery is used to specify a file or URL to a kubeconfig file from which to load cluster information

cmd/kubeadm/app/cmd/upgrade/common_test.go

@@ -180,21 +180,15 @@ func TestPrintConfiguration(t *testing.T) {
 			expectedBytes: []byte(`[upgrade/config] Configuration used:
 	apiServer: {}
 	apiVersion: kubeadm.k8s.io/v1beta2
-	certificatesDir: ""
-	controlPlaneEndpoint: ""
 	controllerManager: {}
 	dns:
 	  type: CoreDNS
 	etcd:
 	  local:
 	    dataDir: /some/path
-	imageRepository: ""
 	kind: ClusterConfiguration
 	kubernetesVersion: v1.7.1
-	networking:
-	  dnsDomain: ""
-	  podSubnet: ""
-	  serviceSubnet: ""
+	networking: {}
 	scheduler: {}
 `),
 		},
@@ -217,8 +211,6 @@ func TestPrintConfiguration(t *testing.T) {
 			expectedBytes: []byte(`[upgrade/config] Configuration used:
 	apiServer: {}
 	apiVersion: kubeadm.k8s.io/v1beta2
-	certificatesDir: ""
-	controlPlaneEndpoint: ""
 	controllerManager: {}
 	dns:
 	  type: CoreDNS
@@ -229,12 +221,9 @@ func TestPrintConfiguration(t *testing.T) {
 	    endpoints:
 	    - https://one-etcd-instance:2379
 	    keyFile: ""
-	imageRepository: ""
 	kind: ClusterConfiguration
 	kubernetesVersion: v1.7.1
 	networking:
-	  dnsDomain: ""
-	  podSubnet: ""
 	  serviceSubnet: 10.96.0.1/12
 	scheduler: {}
 `),

cmd/kubeadm/app/util/config/BUILD

@@ -68,6 +68,7 @@ go_test(
         "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//vendor/github.com/lithammer/dedent:go_default_library",
         "//vendor/github.com/pmezard/go-difflib/difflib:go_default_library",
+        "//vendor/sigs.k8s.io/yaml:go_default_library",
     ],
 )
 

cmd/kubeadm/app/util/config/initconfiguration_test.go

@@ -26,7 +26,11 @@ import (
 
 	"github.com/pmezard/go-difflib/difflib"
 
+	"k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kubeadmapiv1beta2 "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1beta2"
 	"k8s.io/kubernetes/cmd/kubeadm/app/constants"
+	"sigs.k8s.io/yaml"
 )
 
 func diff(expected, actual []byte) string {
@@ -283,3 +287,80 @@ apiVersion: foo.k8s.io/v1
 		})
 	}
 }
+
+func TestDefaultTaintsMarshaling(t *testing.T) {
+	tests := []struct {
+		desc             string
+		cfg              kubeadmapiv1beta2.InitConfiguration
+		expectedTaintCnt int
+	}{
+		{
+			desc: "Uninitialized nodeRegistration field produces a single taint (the master one)",
+			cfg: kubeadmapiv1beta2.InitConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "kubeadm.k8s.io/v1beta2",
+					Kind:       constants.InitConfigurationKind,
+				},
+			},
+			expectedTaintCnt: 1,
+		},
+		{
+			desc: "Uninitialized taints field produces a single taint (the master one)",
+			cfg: kubeadmapiv1beta2.InitConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "kubeadm.k8s.io/v1beta2",
+					Kind:       constants.InitConfigurationKind,
+				},
+				NodeRegistration: kubeadmapiv1beta2.NodeRegistrationOptions{},
+			},
+			expectedTaintCnt: 1,
+		},
+		{
+			desc: "Forsing taints to an empty slice produces no taints",
+			cfg: kubeadmapiv1beta2.InitConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "kubeadm.k8s.io/v1beta2",
+					Kind:       constants.InitConfigurationKind,
+				},
+				NodeRegistration: kubeadmapiv1beta2.NodeRegistrationOptions{
+					Taints: []v1.Taint{},
+				},
+			},
+			expectedTaintCnt: 0,
+		},
+		{
+			desc: "Custom taints are used",
+			cfg: kubeadmapiv1beta2.InitConfiguration{
+				TypeMeta: metav1.TypeMeta{
+					APIVersion: "kubeadm.k8s.io/v1beta2",
+					Kind:       constants.InitConfigurationKind,
+				},
+				NodeRegistration: kubeadmapiv1beta2.NodeRegistrationOptions{
+					Taints: []v1.Taint{
+						{Key: "taint1"},
+						{Key: "taint2"},
+					},
+				},
+			},
+			expectedTaintCnt: 2,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.desc, func(t *testing.T) {
+			b, err := yaml.Marshal(tc.cfg)
+			if err != nil {
+				t.Fatalf("unexpected error while marshalling to YAML: %v", err)
+			}
+
+			cfg, err := BytesToInitConfiguration(b)
+			if err != nil {
+				t.Fatalf("unexpected error of BytesToInitConfiguration: %v\nconfig: %s", err, string(b))
+			}
+
+			if tc.expectedTaintCnt != len(cfg.NodeRegistration.Taints) {
+				t.Fatalf("unexpected taints count\nexpected: %d\ngot: %d\ntaints: %v", tc.expectedTaintCnt, len(cfg.NodeRegistration.Taints), cfg.NodeRegistration.Taints)
+			}
+		})
+	}
+}

cmd/kubeadm/app/util/config/testdata/defaulting/node/defaulted.yaml

@@ -11,3 +11,4 @@ kind: JoinConfiguration
 nodeRegistration:
   criSocket: /var/run/dockershim.sock
   name: thegopher
+  taints: null