Merge pull request kubernetes#84143 from phenixblue/master

Add lenient decoding path for v1alpha1 kube-proxy config

Author: k8s-ci-robot

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"