Merge pull request #14430 from Defensative/updated-fw-tags

Auto commit by PR queue bot

pkg/cloudprovider/providers/gce/gce.go

@@ -423,14 +423,17 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n
 		allowedPorts[ix] = strconv.Itoa(ports[ix].Port)
 	}
 
-	hostTag := gce.computeHostTag(hosts[0])
+	hostTags, err := gce.computeHostTags(hosts)
+	if err != nil {
+		return nil, err
+	}
 
 	firewall := &compute.Firewall{
 		Name:         makeFirewallName(name),
 		Description:  fmt.Sprintf("KubernetesAutoGenerated_OnlyAllowTrafficForDestinationIP_%s", fwd.IPAddress),
 		Network:      gce.networkURL,
 		SourceRanges: []string{"0.0.0.0/0"},
-		TargetTags:   []string{hostTag},
+		TargetTags:   hostTags,
 		Allowed: []*compute.FirewallAllowed{
 			{
 				IPProtocol: "tcp",
@@ -450,16 +453,44 @@ func (gce *GCECloud) EnsureTCPLoadBalancer(name, region string, loadBalancerIP n
 	return status, nil
 }
 
-// This is kind of hacky, but the managed instance group adds 4 random chars and a hyphen
-// to the base name. Older naming schemes put a hyphen and an incrementing index after
-// the base name. Thus we pull off the characters after the final dash to support both.
-func (gce *GCECloud) computeHostTag(host string) string {
-	host = strings.SplitN(host, ".", 2)[0]
-	lastHyphen := strings.LastIndex(host, "-")
-	if lastHyphen == -1 {
-		return host
+// We grab all tags from all instances being added to the pool.
+// * The longest tag that is a prefix of the instance name is used
+// * If any instance has a prefix tag, all instances must
+// * If no instances have a prefix tag, no tags are used
+func (gce *GCECloud) computeHostTags(hosts []string) ([]string, error) {
+	listCall := gce.service.Instances.List(gce.projectID, gce.zone)
+
+	// Add the filter for hosts
+	listCall = listCall.Filter("name eq (" + strings.Join(hosts, "|") + ")")
+
+	// Add the fields we want
+	listCall = listCall.Fields("items(name,tags)")
+
+	res, err := listCall.Do()
+	if err != nil {
+		return nil, err
 	}
-	return host[:lastHyphen]
+
+	tags := sets.NewString()
+	for _, instance := range res.Items {
+		longest_tag := ""
+		for _, tag := range instance.Tags.Items {
+			if strings.HasPrefix(instance.Name, tag) && len(tag) > len(longest_tag) {
+				longest_tag = tag
+			}
+		}
+		if len(longest_tag) > 0 {
+			tags.Insert(longest_tag)
+		} else if len(tags) > 0 {
+			return nil, fmt.Errorf("Some, but not all, instances have prefix tags (%s is missing)", instance.Name)
+		}
+	}
+
+	if len(tags) == 0 {
+		glog.V(2).Info("No instances had tags, creating rule without target tags")
+	}
+
+	return tags.List(), nil
 }
 
 // UpdateTCPLoadBalancer is an implementation of TCPLoadBalancer.UpdateTCPLoadBalancer.

pkg/cloudprovider/providers/gce/gce_test.go

@@ -37,34 +37,6 @@ func TestGetRegion(t *testing.T) {
 	}
 }
 
-func TestGetHostTag(t *testing.T) {
-	tests := []struct {
-		host     string
-		expected string
-	}{
-		{
-			host:     "kubernetes-minion-559o",
-			expected: "kubernetes-minion",
-		},
-		{
-			host:     "gke-test-ea6e8c80-node-8ytk",
-			expected: "gke-test-ea6e8c80-node",
-		},
-		{
-			host:     "kubernetes-minion-559o.c.PROJECT_NAME.internal",
-			expected: "kubernetes-minion",
-		},
-	}
-
-	gce := &GCECloud{}
-	for _, test := range tests {
-		hostTag := gce.computeHostTag(test.host)
-		if hostTag != test.expected {
-			t.Errorf("expected: %s, saw: %s for %s", test.expected, hostTag, test.host)
-		}
-	}
-}
-
 func TestComparingHostURLs(t *testing.T) {
 	tests := []struct {
 		host1       string