Merge pull request #4660 from justinsb/misc_task_cleanups

Misc task code cleanups

upup/pkg/fi/cloudup/awstasks/elastic_ip.go

@@ -17,13 +17,12 @@ limitations under the License.
 package awstasks
 
 import (
-	//"fmt"
-	//
 	"fmt"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
+
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
@@ -32,8 +31,7 @@ import (
 
 //go:generate fitask -type=ElasticIP
 
-// Elastic IP
-// Representation the EIP AWS task
+// ElasticIP manages an AWS Address (ElasticIP)
 type ElasticIP struct {
 	Name      *string
 	Lifecycle *fi.Lifecycle
@@ -71,7 +69,7 @@ func (e *ElasticIP) FindIPAddress(context *fi.Context) (*string, error) {
 	return actual.PublicIP, nil
 }
 
-// Find is a public wrapper for find()
+// Find returns the actual ElasticIP state, or nil if not found
 func (e *ElasticIP) Find(context *fi.Context) (*ElasticIP, error) {
 	return e.find(context.Cloud.(awsup.AWSCloud))
 }
@@ -175,7 +173,7 @@ func (e *ElasticIP) find(cloud awsup.AWSCloud) (*ElasticIP, error) {
 	return nil, nil
 }
 
-// The Run() function is called to execute this task.
+// Run is called to execute this task.
 // This is the main entry point of the task, and will actually
 // connect our internal resource representation to an actual
 // resource in AWS
@@ -233,7 +231,7 @@ func (_ *ElasticIP) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *ElasticIP) e
 		eipId = a.ID
 		err := t.AddAWSTags(*a.ID, changes.Tags)
 		if err != nil {
-			return fmt.Errorf("Unable to tag eip %v", err)
+			return fmt.Errorf("unable to tag ElasticIP: %v", err)
 		}
 	}
 

upup/pkg/fi/cloudup/awstasks/natgateway.go

@@ -22,6 +22,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
@@ -78,14 +79,13 @@ func (e *NatGateway) Find(c *fi.Context) (*NatGateway, error) {
 		}
 
 		if len(response.NatGateways) != 1 {
-			return nil, fmt.Errorf("found %d Nat Gateways, expected 1", len(response.NatGateways))
+			return nil, fmt.Errorf("found %d Nat Gateways with ID %q, expected 1", len(response.NatGateways), fi.StringValue(e.ID))
 		}
 		ngw = response.NatGateways[0]
 
 		if len(ngw.NatGatewayAddresses) != 1 {
 			return nil, fmt.Errorf("found %d EIP Addresses for 1 NATGateway, expected 1", len(ngw.NatGatewayAddresses))
 		}
-		actual.ElasticIP = &ElasticIP{ID: ngw.NatGatewayAddresses[0].AllocationId}
 	} else {
 		// This is the normal/default path
 		var err error
@@ -113,10 +113,10 @@ func (e *NatGateway) Find(c *fi.Context) (*NatGateway, error) {
 	// NATGateways now have names and tags so lets pull from there instead.
 	actual.Name = findNameTag(ngw.Tags)
 	actual.Tags = intersectTags(ngw.Tags, e.Tags)
+
+	// Avoid spurious changes
 	actual.Lifecycle = e.Lifecycle
-
 	actual.Shared = e.Shared
-
 	actual.AssociatedRouteTable = e.AssociatedRouteTable
 
 	e.ID = actual.ID
@@ -231,7 +231,7 @@ func (s *NatGateway) CheckChanges(a, e, changes *NatGateway) error {
 	if a == nil {
 		if !fi.BoolValue(e.Shared) {
 			if e.ElasticIP == nil {
-				return fi.RequiredField("ElasticIp")
+				return fi.RequiredField("ElasticIP")
 			}
 			if e.Subnet == nil {
 				return fi.RequiredField("Subnet")
@@ -245,7 +245,15 @@ func (s *NatGateway) CheckChanges(a, e, changes *NatGateway) error {
 	// Delta
 	if a != nil {
 		if changes.ElasticIP != nil {
-			return fi.CannotChangeField("ElasticIp")
+			eID := ""
+			if e.ElasticIP != nil {
+				eID = fi.StringValue(e.ElasticIP.ID)
+			}
+			aID := ""
+			if a.ElasticIP != nil {
+				aID = fi.StringValue(a.ElasticIP.ID)
+			}
+			return fi.FieldIsImmutable(eID, aID, field.NewPath("ElasticIP"))
 		}
 		if changes.Subnet != nil {
 			return fi.CannotChangeField("Subnet")

upup/pkg/fi/cloudup/awstasks/subnet.go

@@ -136,7 +136,15 @@ func (s *Subnet) CheckChanges(a, e, changes *Subnet) error {
 	if a != nil {
 		// TODO: Do we want to destroy & recreate the subnet when theses immutable fields change?
 		if changes.VPC != nil {
-			errors = append(errors, fi.FieldIsImmutable(a.VPC, e.VPC, fieldPath.Child("VPC")))
+			var aID *string
+			if a.VPC != nil {
+				aID = a.VPC.ID
+			}
+			var eID *string
+			if e.VPC != nil {
+				eID = e.VPC.ID
+			}
+			errors = append(errors, fi.FieldIsImmutable(eID, aID, fieldPath.Child("VPC")))
 		}
 		if changes.AvailabilityZone != nil {
 			errors = append(errors, fi.FieldIsImmutable(a.AvailabilityZone, e.AvailabilityZone, fieldPath.Child("AvailabilityZone")))

upup/pkg/fi/cloudup/awstasks/vpc.go

@@ -22,6 +22,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/golang/glog"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kops/pkg/featureflag"
 	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
@@ -124,7 +125,7 @@ func (s *VPC) CheckChanges(a, e, changes *VPC) error {
 	if a != nil {
 		if changes.CIDR != nil {
 			// TODO: Do we want to destroy & recreate the VPC?
-			return fi.CannotChangeField("CIDR")
+			return fi.FieldIsImmutable(e.CIDR, a.CIDR, field.NewPath("CIDR"))
 		}
 	}
 	return nil