Merge pull request openshift#214 from squeed/sdn-unidling-flag

Add EnableUnidling flag to openshift-sdn

README.md

@@ -114,6 +114,7 @@ OpenShiftSDN supports the following configuration options, all of which are opti
 * `vxlanPort`: The port to use for the VXLAN overlay. The default is 4789
 * `MTU`: The MTU to use for the VXLAN overlay. The default is the MTU of the node that the cluster-network-operator is first run on, minus 50 bytes for overhead. If the nodes in your cluster don't all have the same MTU then you will need to set this explicitly.
 * `useExternalOpenvswitch`: boolean. If the nodes are already running openvswitch, and OpenShiftSDN should not install its own, set this to true. This only needed for certain advanced installations with DPDK or OpenStack.
+* `enableUnidling`: boolean. Whether the service proxy should allow idling and unidling of services.
 
 These configuration flags are only in the Operator configuration object.
 
@@ -126,6 +127,7 @@ spec:
       mode: NetworkPolicy
       vxlanPort: 4789
       mtu: 1450
+      enableUnidling: true
       useExternalOpenvswitch: false
 ```
 
@@ -201,6 +203,7 @@ Most network changes are unsafe to roll out to a production cluster. Therefore,
 It is safe to edit the following fields in the Operator configuration:
 * deployKubeProxy
 * all of kubeProxyConfig
+* OpenshiftSDN enableUnidling, useExternalOpenvswitch.
 
 ### Force-applying an unsafe change
 Administrators may wish to forcefully apply a disruptive change to a cluster that is not serving production traffic. To do this, first they should make the desired configuration change to the CRD. Then, delete the network operator's understanding of the state of the system:

manifests/0000_70_cluster-network-operator_01_crd.yaml

@@ -57,6 +57,8 @@ spec:
                       minimum: 0
                     useExternalOpenvswitch:
                       type: boolean
+                    enableUnidling:
+                      type: boolean
                 ovnKubernetesConfig:
                   type: object
                   properties:

pkg/controller/operconfig/operconfig_controller.go

@@ -160,6 +160,10 @@ func (r *ReconcileOperConfig) Reconcile(request reconcile.Request) (reconcile.Re
 		// FIXME: operator status?
 		return reconcile.Result{}, err
 	}
+	// up-convert Prev by filling defaults
+	if prev != nil {
+		network.FillDefaults(prev, prev)
+	}
 
 	// Fill all defaults explicitly
 	network.FillDefaults(&operConfig.Spec, prev)

pkg/network/openshift_sdn.go

@@ -48,10 +48,21 @@ func renderOpenShiftSDN(conf *operv1.NetworkSpec, manifestDir string) ([]*uns.Un
 		"metrics-bind-address":    {"0.0.0.0"},
 		"metrics-port":            {"9101"},
 		"healthz-port":            {"10256"},
-		"proxy-mode":              {"iptables"},
+		"proxy-mode":              {"unidling+iptables"},
 		"iptables-masquerade-bit": {"0"},
 	}
-	kpc, err := kubeProxyConfiguration(conf, kpcDefaults)
+	if !*c.EnableUnidling {
+		kpcDefaults["proxy-mode"][0] = "iptables"
+	}
+
+	// Always set the proxy-mode, even if the user specified it
+	// We already validated that proxy-mode was either unset or iptables.
+	kpcOverrides := map[string]operv1.ProxyArgumentList{}
+	if *c.EnableUnidling {
+		kpcOverrides["proxy-mode"] = operv1.ProxyArgumentList{"unidling+iptables"}
+	}
+
+	kpc, err := kubeProxyConfiguration(kpcDefaults, conf, kpcOverrides)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to build kube-proxy config")
 	}
@@ -92,6 +103,15 @@ func validateOpenShiftSDN(conf *operv1.NetworkSpec) []error {
 		if sc.MTU != nil && (*sc.MTU < 576 || *sc.MTU > 65536) {
 			out = append(out, errors.Errorf("invalid MTU %d", *sc.MTU))
 		}
+
+		// the proxy mode must be unset or iptables for unidling to work
+		if (sc.EnableUnidling == nil || *sc.EnableUnidling) &&
+			conf.KubeProxyConfig != nil && conf.KubeProxyConfig.ProxyArguments != nil &&
+			len(conf.KubeProxyConfig.ProxyArguments["proxy-mode"]) > 0 &&
+			conf.KubeProxyConfig.ProxyArguments["proxy-mode"][0] != "iptables" {
+
+			out = append(out, errors.Errorf("invalid proxy-mode - when unidling is enabled, proxy-mode must be \"iptables\""))
+		}
 	}
 
 	proxyErrs := validateKubeProxy(conf)
@@ -100,20 +120,39 @@ func validateOpenShiftSDN(conf *operv1.NetworkSpec) []error {
 	return out
 }
 
-// isOpenShiftSDNChangeSafe currently returns an error if any changes are made.
+// isOpenShiftSDNChangeSafe ensures no unsafe changes are applied to the running
+// network
+// It allows changing only useExternalOpenvswitch and enableUnidling.
 // In the future, we may support rolling out MTU or external openvswitch alterations.
+// as with all is*ChangeSafe functions, defaults have already been applied.
 func isOpenShiftSDNChangeSafe(prev, next *operv1.NetworkSpec) []error {
 	pn := prev.DefaultNetwork.OpenShiftSDNConfig
 	nn := next.DefaultNetwork.OpenShiftSDNConfig
+	errs := []error{}
 
 	if reflect.DeepEqual(pn, nn) {
-		return []error{}
+		return errs
+	}
+
+	if pn.Mode != nn.Mode {
+		errs = append(errs, errors.Errorf("cannot change openshift-sdn mode"))
+	}
+
+	// deepequal is nil-safe
+	if !reflect.DeepEqual(pn.VXLANPort, nn.VXLANPort) {
+		errs = append(errs, errors.Errorf("cannot change openshift-sdn vxlanPort"))
+	}
+
+	if !reflect.DeepEqual(pn.MTU, nn.MTU) {
+		errs = append(errs, errors.Errorf("cannot change openshift-sdn mtu"))
 	}
-	return []error{errors.Errorf("cannot change openshift-sdn configuration")}
+
+	// It is allowed to change useExternalOpenvswitch and enableUnidling
+	return errs
 }
 
 func fillOpenShiftSDNDefaults(conf, previous *operv1.NetworkSpec, hostMTU int) {
-	// NOTE: If you change any defaults, and it's not a safe chang to roll out
+	// NOTE: If you change any defaults, and it's not a safe change to roll out
 	// to existing clusters, you MUST use the value from previous instead.
 	if conf.DeployKubeProxy == nil {
 		prox := false
@@ -127,20 +166,25 @@ func fillOpenShiftSDNDefaults(conf, previous *operv1.NetworkSpec, hostMTU int) {
 		conf.KubeProxyConfig.BindAddress = "0.0.0.0"
 	}
 
-	if conf.DefaultNetwork.OpenShiftSDNConfig == nil {
-		conf.DefaultNetwork.OpenShiftSDNConfig = &operv1.OpenShiftSDNConfig{}
-	}
-
 	if conf.KubeProxyConfig.ProxyArguments == nil {
 		conf.KubeProxyConfig.ProxyArguments = map[string]operv1.ProxyArgumentList{}
 	}
 
+	if conf.DefaultNetwork.OpenShiftSDNConfig == nil {
+		conf.DefaultNetwork.OpenShiftSDNConfig = &operv1.OpenShiftSDNConfig{}
+	}
 	sc := conf.DefaultNetwork.OpenShiftSDNConfig
+
 	if sc.VXLANPort == nil {
 		var port uint32 = 4789
 		sc.VXLANPort = &port
 	}
 
+	if sc.EnableUnidling == nil {
+		truth := true
+		sc.EnableUnidling = &truth
+	}
+
 	// MTU is currently the only field we pull from previous.
 	// If it's not supplied, we infer it from  the node on which we're running.
 	// However, this can never change, so we always prefer previous.

pkg/network/openshift_sdn_test.go

@@ -116,6 +116,7 @@ func TestFillOpenShiftSDNDefaults(t *testing.T) {
 	f := false
 	p := uint32(4789)
 	m := uint32(8950)
+	truth := true
 
 	expected := operv1.NetworkSpec{
 		ServiceNetwork: []string{"172.30.0.0/16"},
@@ -132,9 +133,10 @@ func TestFillOpenShiftSDNDefaults(t *testing.T) {
 		DefaultNetwork: operv1.DefaultNetworkDefinition{
 			Type: operv1.NetworkTypeOpenShiftSDN,
 			OpenShiftSDNConfig: &operv1.OpenShiftSDNConfig{
-				Mode:      operv1.SDNModeNetworkPolicy,
-				VXLANPort: &p,
-				MTU:       &m,
+				Mode:           operv1.SDNModeNetworkPolicy,
+				VXLANPort:      &p,
+				MTU:            &m,
+				EnableUnidling: &truth,
 			},
 		},
 		DeployKubeProxy: &f,
@@ -182,6 +184,13 @@ func TestValidateOpenShiftSDN(t *testing.T) {
 
 	config.ClusterNetwork = nil
 	errExpect("ClusterNetwork cannot be empty")
+
+	config.KubeProxyConfig = &operv1.ProxyConfig{
+		ProxyArguments: map[string]operv1.ProxyArgumentList{
+			"proxy-mode": {"userspace"},
+		},
+	}
+	errExpect("invalid proxy-mode - when unidling is enabled")
 }
 
 func TestProxyArgs(t *testing.T) {
@@ -250,6 +259,27 @@ func TestProxyArgs(t *testing.T) {
 	arg, _, _ = uns.NestedString(cfg.Object, "configSyncPeriod")
 	g.Expect(arg).To(Equal("2s"))
 
+	// Setting the proxy mode explicitly still gets unidling
+	config.KubeProxyConfig.ProxyArguments = map[string]operv1.ProxyArgumentList{
+		"proxy-mode": {"iptables"},
+	}
+	objs, err = renderOpenShiftSDN(config, manifestDir)
+	g.Expect(err).NotTo(HaveOccurred())
+	cfg = getProxyConfigFile(objs)
+
+	arg, _, _ = uns.NestedString(cfg.Object, "mode")
+	g.Expect(arg).To(Equal("unidling+iptables"))
+
+	// Disabling unidling doesn't add the fixup
+	f := false
+	config.DefaultNetwork.OpenShiftSDNConfig.EnableUnidling = &f
+	objs, err = renderOpenShiftSDN(config, manifestDir)
+	g.Expect(err).NotTo(HaveOccurred())
+	cfg = getProxyConfigFile(objs)
+
+	arg, _, _ = uns.NestedString(cfg.Object, "mode")
+	g.Expect(arg).To(Equal("iptables"))
+
 }
 
 func TestOpenShiftSDNIsSafe(t *testing.T) {
@@ -266,10 +296,17 @@ func TestOpenShiftSDNIsSafe(t *testing.T) {
 	// change the vxlan port
 	p := uint32(99)
 	next.DefaultNetwork.OpenShiftSDNConfig.VXLANPort = &p
+	next.DefaultNetwork.OpenShiftSDNConfig.Mode = operv1.SDNModeMultitenant
+	mtu := uint32(4000)
+	next.DefaultNetwork.OpenShiftSDNConfig.MTU = &mtu
+	f := false
+	next.DefaultNetwork.OpenShiftSDNConfig.EnableUnidling = &f
 
 	errs = isOpenShiftSDNChangeSafe(prev, next)
-	g.Expect(errs).To(HaveLen(1))
-	g.Expect(errs[0]).To(MatchError("cannot change openshift-sdn configuration"))
+	g.Expect(errs).To(HaveLen(3))
+	g.Expect(errs[0]).To(MatchError("cannot change openshift-sdn mode"))
+	g.Expect(errs[1]).To(MatchError("cannot change openshift-sdn vxlanPort"))
+	g.Expect(errs[2]).To(MatchError("cannot change openshift-sdn mtu"))
 }
 
 func TestOpenShiftSDNMultitenant(t *testing.T) {
@@ -390,6 +427,50 @@ ipvs:
   syncPeriod: 0s
 kind: KubeProxyConfiguration
 metricsBindAddress: 0.0.0.0:9101
+mode: unidling+iptables
+nodePortAddresses: null
+oomScoreAdj: null
+portRange: ""
+resourceContainer: ""
+udpIdleTimeout: 0s`))
+
+	// Disable unidling
+	f := false
+	config.DefaultNetwork.OpenShiftSDNConfig.EnableUnidling = &f
+	objs, err = renderOpenShiftSDN(config, manifestDir)
+	g.Expect(err).NotTo(HaveOccurred())
+	g.Expect(getProxyConfig(objs)).To(MatchYAML(`
+apiVersion: kubeproxy.config.k8s.io/v1alpha1
+bindAddress: 0.0.0.0
+clientConnection:
+  acceptContentTypes: ""
+  burst: 0
+  contentType: ""
+  kubeconfig: ""
+  qps: 0
+clusterCIDR: ""
+configSyncPeriod: 0s
+conntrack:
+  max: null
+  maxPerCore: null
+  min: null
+  tcpCloseWaitTimeout: null
+  tcpEstablishedTimeout: null
+enableProfiling: false
+healthzBindAddress: 0.0.0.0:10256
+hostnameOverride: ""
+iptables:
+  masqueradeAll: false
+  masqueradeBit: 0
+  minSyncPeriod: 0s
+  syncPeriod: 0s
+ipvs:
+  excludeCIDRs: null
+  minSyncPeriod: 0s
+  scheduler: ""
+  syncPeriod: 0s
+kind: KubeProxyConfiguration
+metricsBindAddress: 0.0.0.0:9101
 mode: iptables
 nodePortAddresses: null
 oomScoreAdj: null

pkg/network/previous_test.go

@@ -0,0 +1,68 @@
+package network
+
+import (
+	"strings"
+	"testing"
+
+	k8syaml "k8s.io/apimachinery/pkg/util/yaml"
+
+	operv1 "github.com/openshift/api/operator/v1"
+
+	. "github.com/onsi/gomega"
+)
+
+// TestPreviousConversion ensures that types and defaults are compatable with
+// previous deployed versions of the operator.
+// One important principle is that the generated state with defaults applied
+// *must* always be safe, even as the API evolves
+func TestPreviousVersionsSafe(t *testing.T) {
+	testcases := []struct {
+		name string
+
+		// The configuration expected to be provided by the user.
+		inputConfig string
+
+		// The configuration after running through the fillDefaults **FOR THAT VERSION OF THE OPERATOR**
+		appliedConfig string
+	}{
+
+		// The default configuration for a 4.1.0 cluster
+		{
+			name: "4.1.0 openshift-sdn",
+
+			inputConfig: `{"clusterNetwork":[{"cidr":"10.128.0.0/14","hostPrefix":23}],"defaultNetwork":{"type":"OpenShiftSDN"},"serviceNetwork":["172.30.0.0/16"]}`,
+
+			appliedConfig: `{"clusterNetwork":[{"cidr":"10.128.0.0/14","hostPrefix":23}],"serviceNetwork":["172.30.0.0/16"],"defaultNetwork":{"type":"OpenShiftSDN","openshiftSDNConfig":{"mode":"NetworkPolicy","vxlanPort":4789,"mtu":8951}},"disableMultiNetwork":false,"deployKubeProxy":false,"kubeProxyConfig":{"bindAddress":"0.0.0.0","proxyArguments":{"metrics-bind-address":["0.0.0.0"],"metrics-port":["9101"]}}}'`,
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			input, err := parseNetworkSpec(tc.inputConfig)
+			g.Expect(err).NotTo(HaveOccurred())
+
+			applied, err := parseNetworkSpec(tc.appliedConfig)
+			g.Expect(err).NotTo(HaveOccurred())
+			FillDefaults(applied, applied)
+
+			// This is the exact config transformation flow in the operator
+			Canonicalize(input)
+			g.Expect(Validate(input)).NotTo(HaveOccurred())
+			FillDefaults(input, applied)
+			g.Expect(IsChangeSafe(applied, input)).NotTo(HaveOccurred())
+		})
+	}
+}
+
+func parseNetworkSpec(in string) (*operv1.NetworkSpec, error) {
+	f := strings.NewReader(in)
+	decoder := k8syaml.NewYAMLOrJSONDecoder(f, 4096)
+	spec := operv1.NetworkSpec{}
+	err := decoder.Decode(&spec)
+
+	if err != nil {
+		return nil, err
+	}
+	return &spec, nil
+}