Add lenient decoding path for v1alpha1 kube-proxy

Removed unneeded comments

Matched style from other PR's

Only print error when lenient decoding is successful

Update Bazel for BUILD

Comment out existing strict decoder tests

Added tests for leniant path

Added comments to explain test additions

Cleanup TODO's and tests

Add explicit newline for appended config

Author: phenixblue

cmd/kube-proxy/app/BUILD

@@ -26,6 +26,7 @@ go_library(
         "//pkg/proxy/apis:go_default_library",
         "//pkg/proxy/apis/config:go_default_library",
         "//pkg/proxy/apis/config/scheme:go_default_library",
+        "//pkg/proxy/apis/config/v1alpha1:go_default_library",
         "//pkg/proxy/apis/config/validation:go_default_library",
         "//pkg/proxy/config:go_default_library",
         "//pkg/proxy/healthcheck:go_default_library",
@@ -46,6 +47,7 @@ go_library(
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/runtime/serializer:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/selection:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/wait:go_default_library",
@@ -67,6 +69,7 @@ go_library(
         "//staging/src/k8s.io/component-base/version/verflag:go_default_library",
         "//staging/src/k8s.io/kube-proxy/config/v1alpha1:go_default_library",
         "//vendor/github.com/fsnotify/fsnotify:go_default_library",
+        "//vendor/github.com/pkg/errors:go_default_library",
         "//vendor/github.com/spf13/cobra:go_default_library",
         "//vendor/github.com/spf13/pflag:go_default_library",
         "//vendor/k8s.io/klog:go_default_library",
@@ -167,7 +170,6 @@ go_test(
         "//pkg/proxy/apis/config:go_default_library",
         "//pkg/util/configz:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
-        "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
         "//staging/src/k8s.io/component-base/config:go_default_library",
         "//vendor/github.com/stretchr/testify/assert:go_default_library",

cmd/kube-proxy/app/server.go

@@ -32,10 +32,12 @@ import (
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 
+	gerrors "github.com/pkg/errors"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/apimachinery/pkg/selection"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/apimachinery/pkg/util/wait"
@@ -65,6 +67,7 @@ import (
 	"k8s.io/kubernetes/pkg/proxy/apis"
 	kubeproxyconfig "k8s.io/kubernetes/pkg/proxy/apis/config"
 	proxyconfigscheme "k8s.io/kubernetes/pkg/proxy/apis/config/scheme"
+	kubeproxyconfigv1alpha1 "k8s.io/kubernetes/pkg/proxy/apis/config/v1alpha1"
 	"k8s.io/kubernetes/pkg/proxy/apis/config/validation"
 	"k8s.io/kubernetes/pkg/proxy/config"
 	"k8s.io/kubernetes/pkg/proxy/healthcheck"
@@ -369,6 +372,20 @@ func addressFromDeprecatedFlags(addr string, port int32) string {
 	return proxyutil.AppendPortIfNeeded(addr, port)
 }
 
+// newLenientSchemeAndCodecs returns a scheme that has only v1alpha1 registered into
+// it and a CodecFactory with strict decoding disabled.
+func newLenientSchemeAndCodecs() (*runtime.Scheme, *serializer.CodecFactory, error) {
+	lenientScheme := runtime.NewScheme()
+	if err := kubeproxyconfig.AddToScheme(lenientScheme); err != nil {
+		return nil, nil, fmt.Errorf("failed to add kube-proxy config API to lenient scheme: %v", err)
+	}
+	if err := kubeproxyconfigv1alpha1.AddToScheme(lenientScheme); err != nil {
+		return nil, nil, fmt.Errorf("failed to add kube-proxy config v1alpha1 API to lenient scheme: %v", err)
+	}
+	lenientCodecs := serializer.NewCodecFactory(lenientScheme, serializer.DisableStrict)
+	return lenientScheme, &lenientCodecs, nil
+}
+
 // loadConfigFromFile loads the contents of file and decodes it as a
 // KubeProxyConfiguration object.
 func (o *Options) loadConfigFromFile(file string) (*kubeproxyconfig.KubeProxyConfiguration, error) {
@@ -380,12 +397,34 @@ func (o *Options) loadConfigFromFile(file string) (*kubeproxyconfig.KubeProxyCon
 	return o.loadConfig(data)
 }
 
-// loadConfig decodes data as a KubeProxyConfiguration object.
+// loadConfig decodes a serialized KubeProxyConfiguration to the internal type.
 func (o *Options) loadConfig(data []byte) (*kubeproxyconfig.KubeProxyConfiguration, error) {
+
 	configObj, gvk, err := proxyconfigscheme.Codecs.UniversalDecoder().Decode(data, nil, nil)
 	if err != nil {
-		return nil, err
+		// Try strict decoding first. If that fails decode with a lenient
+		// decoder, which has only v1alpha1 registered, and log a warning.
+		// The lenient path is to be dropped when support for v1alpha1 is dropped.
+		if !runtime.IsStrictDecodingError(err) {
+			return nil, gerrors.Wrap(err, "failed to decode")
+		}
+
+		_, lenientCodecs, lenientErr := newLenientSchemeAndCodecs()
+		if lenientErr != nil {
+			return nil, lenientErr
+		}
+
+		configObj, gvk, lenientErr = lenientCodecs.UniversalDecoder().Decode(data, nil, nil)
+		if lenientErr != nil {
+			// Lenient decoding failed with the current version, return the
+			// original strict error.
+			return nil, fmt.Errorf("failed lenient decoding: %v", err)
+		}
+
+		// Continue with the v1alpha1 object that was decoded leniently, but emit a warning.
+		klog.Warningf("using lenient decoding as strict decoding failed: %v", err)
 	}
+
 	proxyConfig, ok := configObj.(*kubeproxyconfig.KubeProxyConfiguration)
 	if !ok {
 		return nil, fmt.Errorf("got unexpected config type: %v", gvk)

cmd/kube-proxy/app/server_test.go

@@ -33,7 +33,6 @@ import (
 	utilpointer "k8s.io/utils/pointer"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	kuberuntime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/diff"
 	componentbaseconfig "k8s.io/component-base/config"
 	kubeproxyconfig "k8s.io/kubernetes/pkg/proxy/apis/config"
@@ -161,6 +160,7 @@ nodePortAddresses:
 		clusterCIDR        string
 		healthzBindAddress string
 		metricsBindAddress string
+		extraConfig        string
 	}{
 		{
 			name:               "iptables mode, IPv4 all-zeros bind address",
@@ -219,6 +219,30 @@ nodePortAddresses:
 			healthzBindAddress: "[fd00:1::5]:12345",
 			metricsBindAddress: "[fd00:2::5]:23456",
 		},
+		{
+			// Test for unknown field within config.
+			// For v1alpha1 a lenient path is implemented and will throw a
+			// strict decoding warning instead of failing to load
+			name:               "unknown field",
+			mode:               "iptables",
+			bindAddress:        "9.8.7.6",
+			clusterCIDR:        "1.2.3.0/24",
+			healthzBindAddress: "1.2.3.4:12345",
+			metricsBindAddress: "2.3.4.5:23456",
+			extraConfig:        "foo: bar",
+		},
+		{
+			// Test for duplicate field within config.
+			// For v1alpha1 a lenient path is implemented and will throw a
+			// strict decoding warning instead of failing to load
+			name:               "duplicate field",
+			mode:               "iptables",
+			bindAddress:        "9.8.7.6",
+			clusterCIDR:        "1.2.3.0/24",
+			healthzBindAddress: "1.2.3.4:12345",
+			metricsBindAddress: "2.3.4.5:23456",
+			extraConfig:        "bindAddress: 9.8.7.6",
+		},
 	}
 
 	for _, tc := range testCases {
@@ -268,11 +292,17 @@ nodePortAddresses:
 
 		options := NewOptions()
 
-		yaml := fmt.Sprintf(
+		baseYAML := fmt.Sprintf(
 			yamlTemplate, tc.bindAddress, tc.clusterCIDR,
 			tc.healthzBindAddress, tc.metricsBindAddress, tc.mode)
+
+		// Append additional configuration to the base yaml template
+		yaml := fmt.Sprintf("%s\n%s", baseYAML, tc.extraConfig)
+
 		config, err := options.loadConfig([]byte(yaml))
+
 		assert.NoError(t, err, "unexpected error for %s: %v", tc.name, err)
+
 		if !reflect.DeepEqual(expected, config) {
 			t.Fatalf("unexpected config for %s, diff = %s", tc.name, diff.ObjectDiff(config, expected))
 		}
@@ -281,10 +311,15 @@ nodePortAddresses:
 
 // TestLoadConfigFailures tests failure modes for loadConfig()
 func TestLoadConfigFailures(t *testing.T) {
-	yamlTemplate := `bindAddress: 0.0.0.0
-clusterCIDR: "1.2.3.0/24"
-configSyncPeriod: 15s
-kind: KubeProxyConfiguration`
+	// TODO(phenixblue): Uncomment below template when v1alpha2+ of kube-proxy config is
+	// released with strict decoding. These associated tests will fail with
+	// the lenient codec and only one config API version.
+	/*
+			yamlTemplate := `bindAddress: 0.0.0.0
+		clusterCIDR: "1.2.3.0/24"
+		configSyncPeriod: 15s
+		kind: KubeProxyConfiguration`
+	*/
 
 	testCases := []struct {
 		name    string
@@ -307,16 +342,21 @@ kind: KubeProxyConfiguration`
 			config: "bindAddress: ::",
 			expErr: "mapping values are not allowed in this context",
 		},
-		{
-			name:    "Duplicate fields",
-			config:  fmt.Sprintf("%s\nbindAddess: 1.2.3.4", yamlTemplate),
-			checkFn: kuberuntime.IsStrictDecodingError,
-		},
-		{
-			name:    "Unknown field",
-			config:  fmt.Sprintf("%s\nfoo: bar", yamlTemplate),
-			checkFn: kuberuntime.IsStrictDecodingError,
-		},
+		// TODO(phenixblue): Uncomment below tests when v1alpha2+ of kube-proxy config is
+		// released with strict decoding. These tests will fail with the
+		// lenient codec and only one config API version.
+		/*
+			{
+				name:    "Duplicate fields",
+				config:  fmt.Sprintf("%s\nbindAddress: 1.2.3.4", yamlTemplate),
+				checkFn: kuberuntime.IsStrictDecodingError,
+			},
+			{
+				name:    "Unknown field",
+				config:  fmt.Sprintf("%s\nfoo: bar", yamlTemplate),
+				checkFn: kuberuntime.IsStrictDecodingError,
+			},
+		*/
 	}
 
 	version := "apiVersion: kubeproxy.config.k8s.io/v1alpha1"