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
This commit is contained in:
Joe Searcy 2019-10-21 00:44:17 -04:00
parent d0d4572c82
commit 10879d3bd4
3 changed files with 100 additions and 19 deletions

View File

@ -26,6 +26,7 @@ go_library(
"//pkg/proxy/apis:go_default_library", "//pkg/proxy/apis:go_default_library",
"//pkg/proxy/apis/config:go_default_library", "//pkg/proxy/apis/config:go_default_library",
"//pkg/proxy/apis/config/scheme: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/apis/config/validation:go_default_library",
"//pkg/proxy/config:go_default_library", "//pkg/proxy/config:go_default_library",
"//pkg/proxy/healthcheck: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/apis/meta/v1:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/labels: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: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/selection:go_default_library",
"//staging/src/k8s.io/apimachinery/pkg/util/runtime: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", "//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/component-base/version/verflag:go_default_library",
"//staging/src/k8s.io/kube-proxy/config/v1alpha1: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/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/cobra:go_default_library",
"//vendor/github.com/spf13/pflag:go_default_library", "//vendor/github.com/spf13/pflag:go_default_library",
"//vendor/k8s.io/klog:go_default_library", "//vendor/k8s.io/klog:go_default_library",
@ -167,7 +170,6 @@ go_test(
"//pkg/proxy/apis/config:go_default_library", "//pkg/proxy/apis/config:go_default_library",
"//pkg/util/configz: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/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/apimachinery/pkg/util/diff:go_default_library",
"//staging/src/k8s.io/component-base/config:go_default_library", "//staging/src/k8s.io/component-base/config:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library",

View File

@ -32,10 +32,12 @@ import (
"github.com/spf13/cobra" "github.com/spf13/cobra"
"github.com/spf13/pflag" "github.com/spf13/pflag"
gerrors "github.com/pkg/errors"
v1 "k8s.io/api/core/v1" v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/serializer"
"k8s.io/apimachinery/pkg/selection" "k8s.io/apimachinery/pkg/selection"
utilruntime "k8s.io/apimachinery/pkg/util/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime"
"k8s.io/apimachinery/pkg/util/wait" "k8s.io/apimachinery/pkg/util/wait"
@ -65,6 +67,7 @@ import (
"k8s.io/kubernetes/pkg/proxy/apis" "k8s.io/kubernetes/pkg/proxy/apis"
kubeproxyconfig "k8s.io/kubernetes/pkg/proxy/apis/config" kubeproxyconfig "k8s.io/kubernetes/pkg/proxy/apis/config"
proxyconfigscheme "k8s.io/kubernetes/pkg/proxy/apis/config/scheme" 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/apis/config/validation"
"k8s.io/kubernetes/pkg/proxy/config" "k8s.io/kubernetes/pkg/proxy/config"
"k8s.io/kubernetes/pkg/proxy/healthcheck" "k8s.io/kubernetes/pkg/proxy/healthcheck"
@ -369,6 +372,20 @@ func addressFromDeprecatedFlags(addr string, port int32) string {
return proxyutil.AppendPortIfNeeded(addr, port) 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 // loadConfigFromFile loads the contents of file and decodes it as a
// KubeProxyConfiguration object. // KubeProxyConfiguration object.
func (o *Options) loadConfigFromFile(file string) (*kubeproxyconfig.KubeProxyConfiguration, error) { 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) 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) { func (o *Options) loadConfig(data []byte) (*kubeproxyconfig.KubeProxyConfiguration, error) {
configObj, gvk, err := proxyconfigscheme.Codecs.UniversalDecoder().Decode(data, nil, nil) configObj, gvk, err := proxyconfigscheme.Codecs.UniversalDecoder().Decode(data, nil, nil)
if err != 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) proxyConfig, ok := configObj.(*kubeproxyconfig.KubeProxyConfiguration)
if !ok { if !ok {
return nil, fmt.Errorf("got unexpected config type: %v", gvk) return nil, fmt.Errorf("got unexpected config type: %v", gvk)

View File

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