diff --git a/pkg/cloudprovider/providers/gce/gce.go b/pkg/cloudprovider/providers/gce/gce.go index 20505d7098..91d7ca0403 100644 --- a/pkg/cloudprovider/providers/gce/gce.go +++ b/pkg/cloudprovider/providers/gce/gce.go @@ -938,10 +938,16 @@ func (gce *GCECloud) firewallObject(name, region, desc string, sourceRanges nets for ix := range ports { allowedPorts[ix] = strconv.Itoa(int(ports[ix].Port)) } - hostTags, err := gce.computeHostTags(hosts) - if err != nil { - return nil, err + // If the node tags to be used for this cluster have been predefined in the + // provider config, just use them. Otherwise, invoke computeHostTags method to get the tags. + hostTags := gce.nodeTags + if len(hostTags) == 0 { + var err error + if hostTags, err = gce.computeHostTags(hosts); err != nil { + return nil, fmt.Errorf("No node tags supplied and also failed to parse the given lists of hosts for tags. Abort creating firewall rule.") + } } + firewall := &compute.Firewall{ Name: makeFirewallName(name), Description: desc, @@ -963,17 +969,13 @@ func (gce *GCECloud) firewallObject(name, region, desc string, sourceRanges nets return firewall, nil } -// If the node tags to be used for this cluster have been predefined in the -// provider config, just use them. Otherwise, grab all tags from all relevant -// instances: +// ComputeHostTags grabs 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 +// * If any instance has no matching prefix tag, return error +// Invoking this method to get host tags is risky since it depends on the format +// of the host names in the cluster. Only use it as a fallback if gce.nodeTags +// is unspecified func (gce *GCECloud) computeHostTags(hosts []*gceInstance) ([]string, error) { - if len(gce.nodeTags) > 0 { - return gce.nodeTags, nil - } - // TODO: We could store the tags in gceInstance, so we could have already fetched it hostNamesByZone := make(map[string][]string) for _, host := range hosts { @@ -1012,8 +1014,8 @@ func (gce *GCECloud) computeHostTags(hosts []*gceInstance) ([]string, error) { } 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) + } else { + return nil, fmt.Errorf("Could not find any tag that is a prefix of instance name for instance %s", instance.Name) } } } @@ -1021,11 +1023,9 @@ func (gce *GCECloud) computeHostTags(hosts []*gceInstance) ([]string, error) { glog.Errorf("computeHostTags exceeded maxPages=%d for Instances.List: truncating.", maxPages) } } - if len(tags) == 0 { - glog.V(2).Info("No instances had tags, creating rule without target tags") + return nil, fmt.Errorf("No instances found") } - return tags.List(), nil } diff --git a/pkg/cloudprovider/providers/gce/gce_test.go b/pkg/cloudprovider/providers/gce/gce_test.go index f9b980c9a4..ad20044cc1 100644 --- a/pkg/cloudprovider/providers/gce/gce_test.go +++ b/pkg/cloudprovider/providers/gce/gce_test.go @@ -152,6 +152,16 @@ func TestScrubDNS(t *testing.T) { } } +func TestCreateFirewallFails(t *testing.T) { + name := "loadbalancer" + region := "us-central1" + desc := "description" + gce := &GCECloud{} + if err := gce.createFirewall(name, region, desc, nil, nil, nil); err == nil { + t.Errorf("error expected when creating firewall without any tags found") + } +} + func TestRestrictTargetPool(t *testing.T) { const maxInstances = 5 tests := []struct {