Merge pull request #1925 from justinsb/fix_1793

Additional ShouldCreate method to prevent spurious changes

pkg/model/autoscalinggroup.go

@@ -66,15 +66,26 @@ func (b *AutoscalingGroupModelBuilder) Build(c *fi.ModelBuilderContext) error {
 				SecurityGroups: []*awstasks.SecurityGroup{
 					b.LinkToSecurityGroup(ig.Spec.Role),
 				},
-				AdditionalSecurityGroupIDs: ig.Spec.AdditionalSecurityGroups,
+				IAMInstanceProfile: b.LinkToIAMInstanceProfile(ig),
-				IAMInstanceProfile:         b.LinkToIAMInstanceProfile(ig),
+				ImageID:            s(ig.Spec.Image),
-				ImageID:                    s(ig.Spec.Image),
+				InstanceType:       s(ig.Spec.MachineType),
-				InstanceType:               s(ig.Spec.MachineType),
 
 				RootVolumeSize: i64(int64(volumeSize)),
 				RootVolumeType: s(volumeType),
 			}
 
+			for _, id := range ig.Spec.AdditionalSecurityGroups {
+				sgTask := &awstasks.SecurityGroup{
+					Name:   fi.String(id),
+					ID:     fi.String(id),
+					Shared: fi.Bool(true),
+				}
+				if err := c.EnsureTask(sgTask); err != nil {
+					return err
+				}
+				t.SecurityGroups = append(t.SecurityGroups, sgTask)
+			}
+
 			var err error
 
 			if t.SSHKey, err = b.LinkToSSHKey(); err != nil {

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

@@ -96,6 +96,18 @@ func (s *IAMRolePolicy) CheckChanges(a, e, changes *IAMRolePolicy) error {
 	return nil
 }
 
+func (_ *IAMRolePolicy) ShouldCreate(a, e, changes *IAMRolePolicy) (bool, error) {
+	ePolicy, err := e.PolicyDocument.AsString()
+	if err != nil {
+		return false, fmt.Errorf("error rendering PolicyDocument: %v", err)
+	}
+
+	if a == nil && ePolicy == "" {
+		return false, nil
+	}
+	return true, nil
+}
+
 func (_ *IAMRolePolicy) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *IAMRolePolicy) error {
 	policy, err := e.PolicyDocument.AsString()
 	if err != nil {

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

@@ -29,6 +29,7 @@ import (
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
 	"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
+	"sort"
 	"time"
 )
 
@@ -38,13 +39,12 @@ type LaunchConfiguration struct {
 
 	UserData *fi.ResourceHolder
 
-	ImageID                    *string
+	ImageID            *string
-	InstanceType               *string
+	InstanceType       *string
-	SSHKey                     *SSHKey
+	SSHKey             *SSHKey
-	SecurityGroups             []*SecurityGroup
+	SecurityGroups     []*SecurityGroup
-	AdditionalSecurityGroupIDs []string
+	AssociatePublicIP  *bool
-	AssociatePublicIP          *bool
+	IAMInstanceProfile *IAMInstanceProfile
-	IAMInstanceProfile         *IAMInstanceProfile
 
 	// RootVolumeSize is the size of the EBS root volume to use, in GB
 	RootVolumeSize *int64
@@ -115,6 +115,8 @@ func (e *LaunchConfiguration) Find(c *fi.Context) (*LaunchConfiguration, error)
 	for _, sgID := range lc.SecurityGroups {
 		securityGroups = append(securityGroups, &SecurityGroup{ID: sgID})
 	}
+	sort.Sort(OrderSecurityGroupsById(securityGroups))
+
 	actual.SecurityGroups = securityGroups
 
 	// Find the root volume
@@ -195,9 +197,17 @@ func (e *LaunchConfiguration) buildRootDevice(cloud awsup.AWSCloud) (map[string]
 }
 
 func (e *LaunchConfiguration) Run(c *fi.Context) error {
+	// TODO: Make Normalize a standard method
+	e.Normalize()
+
 	return fi.DefaultDeltaRunMethod(e, c)
 }
 
+func (e *LaunchConfiguration) Normalize() {
+	// We need to sort our arrays consistently, so we don't get spurious changes
+	sort.Stable(OrderSecurityGroupsById(e.SecurityGroups))
+}
+
 func (s *LaunchConfiguration) CheckChanges(a, e, changes *LaunchConfiguration) error {
 	if e.ImageID == nil {
 		return fi.RequiredField("ImageID")
@@ -240,10 +250,6 @@ func (_ *LaunchConfiguration) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *La
 		securityGroupIDs = append(securityGroupIDs, sg.ID)
 	}
 
-	for i := range e.AdditionalSecurityGroupIDs {
-		securityGroupIDs = append(securityGroupIDs, &e.AdditionalSecurityGroupIDs[i])
-	}
-
 	request.SecurityGroups = securityGroupIDs
 	request.AssociatePublicIpAddress = e.AssociatePublicIP
 	if e.SpotPrice != "" {
@@ -369,9 +375,6 @@ func (_ *LaunchConfiguration) RenderTerraform(t *terraform.TerraformTarget, a, e
 		tf.SecurityGroups = append(tf.SecurityGroups, sg.TerraformLink())
 	}
 
-	for _, sg := range e.AdditionalSecurityGroupIDs {
-		tf.SecurityGroups = append(tf.SecurityGroups, terraform.LiteralFromStringValue(sg))
-	}
 	tf.AssociatePublicIpAddress = e.AssociatePublicIP
 
 	{

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

@@ -18,6 +18,8 @@ package awstasks
 
 import (
 	"fmt"
+	"strconv"
+	"strings"
 
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/ec2"
@@ -26,8 +28,6 @@ import (
 	"k8s.io/kops/upup/pkg/fi/cloudup/awsup"
 	"k8s.io/kops/upup/pkg/fi/cloudup/cloudformation"
 	"k8s.io/kops/upup/pkg/fi/cloudup/terraform"
-	"strconv"
-	"strings"
 )
 
 //go:generate fitask -type=SecurityGroup
@@ -39,6 +39,9 @@ type SecurityGroup struct {
 	VPC         *VPC
 
 	RemoveExtraRules []string
+
+	// Shared is set if this is a shared security group (one we don't create or own)
+	Shared *bool
 }
 
 var _ fi.CompareWithID = &SecurityGroup{}
@@ -77,6 +80,12 @@ func (e *SecurityGroup) Find(c *fi.Context) (*SecurityGroup, error) {
 
 	actual.RemoveExtraRules = e.RemoveExtraRules
 
+	// Prevent spurious comparison failures
+	actual.Shared = e.Shared
+	if e.ID == nil {
+		e.ID = actual.ID
+	}
+
 	return actual, nil
 }
 
@@ -123,6 +132,13 @@ func (e *SecurityGroup) Run(c *fi.Context) error {
 	return fi.DefaultDeltaRunMethod(e, c)
 }
 
+func (_ *SecurityGroup) ShouldCreate(a, e, changes *SecurityGroup) (bool, error) {
+	if fi.BoolValue(e.Shared) {
+		return false, nil
+	}
+	return true, nil
+}
+
 func (_ *SecurityGroup) CheckChanges(a, e, changes *SecurityGroup) error {
 	if a != nil {
 		if changes.ID != nil {
@@ -139,6 +155,12 @@ func (_ *SecurityGroup) CheckChanges(a, e, changes *SecurityGroup) error {
 }
 
 func (_ *SecurityGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *SecurityGroup) error {
+	shared := fi.BoolValue(e.Shared)
+	if shared {
+		// Do we want to do any verification of the security group?
+		return nil
+	}
+
 	if a == nil {
 		glog.V(2).Infof("Creating SecurityGroup with Name:%q VPC:%q", *e.Name, *e.VPC.ID)
 
@@ -169,6 +191,12 @@ type terraformSecurityGroup struct {
 func (_ *SecurityGroup) RenderTerraform(t *terraform.TerraformTarget, a, e, changes *SecurityGroup) error {
 	cloud := t.Cloud.(awsup.AWSCloud)
 
+	shared := fi.BoolValue(e.Shared)
+	if shared {
+		// Not terraform owned / managed
+		return nil
+	}
+
 	tf := &terraformSecurityGroup{
 		Name:        e.Name,
 		VPCID:       e.VPC.TerraformLink(),

upup/pkg/fi/default_methods.go

@@ -53,10 +53,17 @@ func DefaultDeltaRunMethod(e Task, c *Context) error {
 			return err
 		}
 
-		err = c.Render(a, e, changes)
+		shouldCreate, err := invokeShouldCreate(a, e, changes)
 		if err != nil {
 			return err
 		}
+
+		if shouldCreate {
+			err = c.Render(a, e, changes)
+			if err != nil {
+				return err
+			}
+		}
 	}
 
 	if producesDeletions, ok := e.(ProducesDeletions); ok && c.Target.ProcessDeletions() {
@@ -109,3 +116,19 @@ func invokeFind(e Task, c *Context) (Task, error) {
 	}
 	return task, err
 }
+
+// invokeShouldCreate calls the ShouldCreate method by reflection, if it exists
+func invokeShouldCreate(a, e, changes Task) (bool, error) {
+	rv, err := utils.InvokeMethod(e, "ShouldCreate", a, e, changes)
+	if err != nil {
+		if utils.IsMethodNotFound(err) {
+			return true, nil
+		}
+		return false, err
+	}
+	shouldCreate := rv[0].Interface().(bool)
+	if !rv[1].IsNil() {
+		err = rv[1].Interface().(error)
+	}
+	return shouldCreate, err
+}

upup/pkg/fi/utils/reflect.go

@@ -26,6 +26,20 @@ import (
 
 var SkipReflection = errors.New("skip this value")
 
+type MethodNotFoundError struct {
+	Name   string
+	Target interface{}
+}
+
+func (e *MethodNotFoundError) Error() string {
+	return fmt.Sprintf("method %s not found on %T", e.Name, e.Target)
+}
+
+func IsMethodNotFound(err error) bool {
+	_, ok := err.(*MethodNotFoundError)
+	return ok
+}
+
 // JsonMergeStruct merges src into dest
 // It uses a JSON marshal & unmarshal, so only fields that are JSON-visible will be copied
 func JsonMergeStruct(dest, src interface{}) {
@@ -46,7 +60,10 @@ func InvokeMethod(target interface{}, name string, args ...interface{}) ([]refle
 
 	method, found := v.Type().MethodByName(name)
 	if !found {
-		return nil, fmt.Errorf("method %q not found on %T", name, target)
+		return nil, &MethodNotFoundError{
+			Name:   name,
+			Target: target,
+		}
 	}
 
 	var argValues []reflect.Value