Respect Topology when assigning floating ips or not

pkg/apis/kops/validation/BUILD.bazel

@@ -9,6 +9,7 @@ go_library(
         "helpers.go",
         "instancegroup.go",
         "legacy.go",
+        "openstack.go",
         "validation.go",
     ],
     importpath = "k8s.io/kops/pkg/apis/kops/validation",

pkg/apis/kops/validation/openstack.go

@@ -0,0 +1,35 @@
+/*
+Copyright 2020 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/kops/pkg/apis/kops"
+)
+
+func openstackValidateCluster(c *kops.Cluster) (errList field.ErrorList) {
+	if c.Spec.CloudConfig.Openstack.Router == nil || c.Spec.CloudConfig.Openstack.Router.ExternalNetwork == nil {
+		topology := c.Spec.Topology
+		if topology == nil || topology.Nodes == kops.TopologyPublic {
+			errList = append(errList, field.Forbidden(field.NewPath("spec", "topology", "nodes"), "Public topology requires an external network"))
+		}
+		if topology == nil || topology.Masters == kops.TopologyPublic {
+			errList = append(errList, field.Forbidden(field.NewPath("spec", "topology", "masters"), "Public topology requires an external network"))
+		}
+	}
+	return errList
+}

pkg/apis/kops/validation/openstack_test.go

@@ -0,0 +1,74 @@
+package validation
+
+import (
+	"testing"
+
+	"k8s.io/kops/upup/pkg/fi"
+
+	"k8s.io/kops/pkg/apis/kops"
+)
+
+func Test_ValidateTopology(t *testing.T) {
+	grid := []struct {
+		Input          kops.ClusterSpec
+		ExpectedErrors []string
+	}{
+		{
+			Input: kops.ClusterSpec{
+				CloudConfig: &kops.CloudConfiguration{
+					Openstack: &kops.OpenstackConfiguration{},
+				},
+			},
+			ExpectedErrors: []string{
+				"Forbidden::spec.topology.nodes",
+				"Forbidden::spec.topology.masters",
+			},
+		},
+		{
+			Input: kops.ClusterSpec{
+				CloudConfig: &kops.CloudConfiguration{
+					Openstack: &kops.OpenstackConfiguration{
+						Router: &kops.OpenstackRouter{},
+					},
+				},
+			},
+			ExpectedErrors: []string{
+				"Forbidden::spec.topology.nodes",
+				"Forbidden::spec.topology.masters",
+			},
+		},
+		{
+
+			Input: kops.ClusterSpec{
+				CloudConfig: &kops.CloudConfiguration{
+					Openstack: &kops.OpenstackConfiguration{},
+				},
+				Topology: &kops.TopologySpec{
+					Masters: kops.TopologyPrivate,
+					Nodes:   kops.TopologyPrivate,
+				},
+			},
+			ExpectedErrors: []string{},
+		},
+		{
+			Input: kops.ClusterSpec{
+				CloudConfig: &kops.CloudConfiguration{
+					Openstack: &kops.OpenstackConfiguration{
+						Router: &kops.OpenstackRouter{
+							ExternalNetwork: fi.String("foo"),
+						},
+					},
+				},
+			},
+			ExpectedErrors: []string{},
+		},
+	}
+
+	for _, g := range grid {
+		cluster := &kops.Cluster{
+			Spec: g.Input,
+		}
+		errs := openstackValidateCluster(cluster)
+		testErrors(t, g.Input, errs, g.ExpectedErrors)
+	}
+}

pkg/apis/kops/validation/validation.go

@@ -67,6 +67,8 @@ func newValidateCluster(cluster *kops.Cluster) field.ErrorList {
 		allErrs = append(allErrs, awsValidateCluster(cluster)...)
 	case kops.CloudProviderGCE:
 		allErrs = append(allErrs, gceValidateCluster(cluster)...)
+	case kops.CloudProviderOpenstack:
+		allErrs = append(allErrs, openstackValidateCluster(cluster)...)
 	}
 
 	return allErrs

pkg/model/openstackmodel/servergroup.go

@@ -146,8 +146,8 @@ func (b *ServerGroupModelBuilder) buildInstances(c *fi.ModelBuilderContext, sg *
 		}
 		c.AddTask(instanceTask)
 
-		// Associate a floating IP to the master and bastion always if we have external network in router
-		// associate it to a node if bastion is not used
+		// Associate a floating IP to the instances if we have external network in router
+		// and respective topology is "public"
 		if b.Cluster.Spec.CloudConfig.Openstack != nil && b.Cluster.Spec.CloudConfig.Openstack.Router != nil {
 			if ig.Spec.AssociatePublicIP != nil && !fi.BoolValue(ig.Spec.AssociatePublicIP) {
 				continue
@@ -161,7 +161,8 @@ func (b *ServerGroupModelBuilder) buildInstances(c *fi.ModelBuilderContext, sg *
 				c.AddTask(t)
 				instanceTask.FloatingIP = t
 			case kops.InstanceGroupRoleMaster:
-				if b.Cluster.Spec.CloudConfig.Openstack.Loadbalancer == nil {
+
+				if b.Cluster.Spec.Topology == nil || b.Cluster.Spec.Topology.Masters != kops.TopologyPrivate {
 					t := &openstacktasks.FloatingIP{
 						Name:      fi.String(fmt.Sprintf("%s-%s", "fip", *instanceTask.Name)),
 						Lifecycle: b.Lifecycle,
@@ -171,7 +172,7 @@ func (b *ServerGroupModelBuilder) buildInstances(c *fi.ModelBuilderContext, sg *
 					instanceTask.FloatingIP = t
 				}
 			default:
-				if b.Cluster.Spec.Topology == nil || b.Cluster.Spec.Topology.Nodes == "public" {
+				if b.Cluster.Spec.Topology == nil || b.Cluster.Spec.Topology.Nodes != kops.TopologyPrivate {
 					t := &openstacktasks.FloatingIP{
 						Name:      fi.String(fmt.Sprintf("%s-%s", "fip", *instanceTask.Name)),
 						Lifecycle: b.Lifecycle,

pkg/model/openstackmodel/servergroup_test.go

@@ -864,6 +864,9 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 							},
 						},
 					},
+					Topology: &kops.TopologySpec{
+						Masters: kops.TopologyPrivate,
+					},
 					Subnets: []kops.ClusterSubnetSpec{
 						{
 							Name:   "subnet-a",