Handle upgrade with invalid CIDR stored in default ClusterNetwork

We now fix invalid CIDRs before storing them into the ClusterNetwork
object, but we need to make sure that's not considered an illegal
change from a previous invalid-but-working state stored by an older
version that didn't fix them.

Author: danwinship

pkg/network/master/master.go

@@ -110,7 +110,7 @@ func Start(networkConfig osconfigapi.MasterNetworkConfig, networkClient networkc
 				glog.Errorf("Cluster contains objects incompatible with new ClusterNetwork: %v", err)
 			}
 		} else {
-			configChanged, err := clusterNetworkChanged(configCN, existingCN)
+			configChanged, err := clusterNetworkChanged(configCN, existingCN, networkConfig)
 			if err != nil {
 				return false, err
 			}
@@ -184,9 +184,13 @@ func (master *OsdnMaster) checkClusterNetworkAgainstClusterObjects() error {
 	return master.networkInfo.CheckClusterObjects(subnets, pods, services)
 }
 
-func clusterNetworkChanged(obj *networkapi.ClusterNetwork, old *networkapi.ClusterNetwork) (bool, error) {
+func clusterNetworkChanged(obj *networkapi.ClusterNetwork, old *networkapi.ClusterNetwork, networkConfig osconfigapi.MasterNetworkConfig) (bool, error) {
 
 	if old.ServiceNetwork != obj.ServiceNetwork {
+		if old.ServiceNetwork == networkConfig.ServiceNetworkCIDR {
+			// We corrected it from invalid to valid
+			return true, nil
+		}
 		return true, fmt.Errorf("cannot change the serviceNetworkCIDR of an already-deployed cluster")
 	} else if old.PluginName != obj.PluginName {
 		return true, nil
@@ -208,6 +212,5 @@ func clusterNetworkChanged(obj *networkapi.ClusterNetwork, old *networkapi.Clust
 			}
 		}
 		return changed, nil
-
 	}
 }

pkg/network/master/master_test.go

@@ -3,6 +3,7 @@ package master
 import (
 	"testing"
 
+	osconfigapi "github.com/openshift/origin/pkg/cmd/server/api"
 	networkapi "github.com/openshift/origin/pkg/network/apis/network"
 	"github.com/openshift/origin/pkg/network/common"
 )
@@ -85,7 +86,10 @@ func Test_clusterNetworkChanged(t *testing.T) {
 			expectChanged = true
 		}
 
-		changed, err := clusterNetworkChanged(&newCN, &origCN)
+		networkConfig := osconfigapi.MasterNetworkConfig{
+			ServiceNetworkCIDR: newCN.ServiceNetwork,
+		}
+		changed, err := clusterNetworkChanged(&newCN, &origCN, networkConfig)
 		if changed != expectChanged {
 			t.Fatalf("unexpected result (%t instead of %t) on %q: %s -> %s", changed, expectChanged, test.name, common.ClusterNetworkToString(&origCN), common.ClusterNetworkToString(&newCN))
 		}
@@ -94,3 +98,39 @@ func Test_clusterNetworkChanged(t *testing.T) {
 		}
 	}
 }
+
+func Test_clusterNetworkChanged_BadCIDR(t *testing.T) {
+	badCN := networkapi.ClusterNetwork{
+		ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.128.1.0/14", HostSubnetLength: 10}},
+		ServiceNetwork:  "172.30.1.0/16",
+		PluginName:      "redhat/openshift-ovs-subnet",
+	}
+	correctedCN := networkapi.ClusterNetwork{
+		ClusterNetworks: []networkapi.ClusterNetworkEntry{{CIDR: "10.128.0.0/14", HostSubnetLength: 10}},
+		ServiceNetwork:  "172.30.0.0/16",
+		PluginName:      "redhat/openshift-ovs-subnet",
+	}
+	badNetworkConfig := osconfigapi.MasterNetworkConfig{
+		ServiceNetworkCIDR: "172.30.1.0/16",
+	}
+
+	// Older releases would store bad CIDR values into default ClusterNetwork.
+	// Current master will always construct a corrected ClusterNetwork; this should
+	// be reported as changed but not an error (so the etcd value will get updated).
+	changed, err := clusterNetworkChanged(&correctedCN, &badCN, badNetworkConfig)
+	if !changed {
+		t.Fatalf("ClusterNetwork upgrade from bad CIDR to corrected was not considered a change")
+	}
+	if err != nil {
+		t.Fatalf("ClusterNetwork upgrade from bad CIDR to corrected was considered an error: %v", err)
+	}
+
+	// Should work after the etcd value is corrected even if master-config isn't
+	changed, err = clusterNetworkChanged(&correctedCN, &correctedCN, badNetworkConfig)
+	if changed {
+		t.Fatalf("ClusterNetwork unexpectedly considered changed")
+	}
+	if err != nil {
+		t.Fatalf("ClusterNetwork unexpectedly considered an error: %v", err)
+	}
+}