From 6eed8ff09527841d233b43afdbfa3a95edaf232c Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Sun, 30 Oct 2022 14:09:06 -0700 Subject: [PATCH] Refactor all normalization code into new Normalize() method --- .../fi/cloudup/awstasks/autoscalinggroup.go | 10 ++---- .../cloudup/awstasks/classic_load_balancer.go | 10 +++--- upup/pkg/fi/cloudup/awstasks/ebsvolume.go | 36 +++++-------------- .../pkg/fi/cloudup/awstasks/launchtemplate.go | 15 ++++---- .../cloudup/awstasks/network_load_balancer.go | 10 +++--- upup/pkg/fi/cloudup/awstasks/sshkey.go | 7 +++- upup/pkg/fi/cloudup/azuretasks/disk.go | 7 +++- upup/pkg/fi/cloudup/azuretasks/disk_test.go | 6 +++- .../pkg/fi/cloudup/azuretasks/loadbalancer.go | 7 +++- .../cloudup/azuretasks/loadbalancer_test.go | 6 +++- .../fi/cloudup/azuretasks/publicipaddress.go | 7 +++- .../azuretasks/publicipaddress_test.go | 6 +++- .../fi/cloudup/azuretasks/resourcegroup.go | 7 +++- .../cloudup/azuretasks/resourcegroup_test.go | 6 +++- upup/pkg/fi/cloudup/azuretasks/routetable.go | 7 +++- .../fi/cloudup/azuretasks/virtualnetwork.go | 7 +++- .../cloudup/azuretasks/virtualnetwork_test.go | 6 +++- upup/pkg/fi/cloudup/azuretasks/vmscaleset.go | 8 ++++- .../fi/cloudup/azuretasks/vmscaleset_test.go | 6 +++- upup/pkg/fi/cloudup/gcetasks/firewallrule.go | 8 ++--- upup/pkg/fi/cloudup/openstacktasks/sshkey.go | 7 +++- upup/pkg/fi/cloudup/openstacktasks/volume.go | 6 +++- upup/pkg/fi/executor.go | 8 +++++ upup/pkg/fi/fitasks/keypair.go | 7 ++-- upup/pkg/fi/task.go | 13 ++++++- 25 files changed, 142 insertions(+), 81 deletions(-) diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go index 84d383cd64..bccf3db0e1 100644 --- a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go +++ b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go @@ -93,6 +93,7 @@ type AutoscalingGroup struct { } var _ fi.CompareWithID = &AutoscalingGroup{} +var _ fi.TaskNormalize = &AutoscalingGroup{} // CompareWithID returns the ID of the ASG func (e *AutoscalingGroup) CompareWithID() *string { @@ -311,20 +312,15 @@ func findAutoscalingGroup(cloud awsup.AWSCloud, name string) (*autoscaling.Group return nil, fmt.Errorf("found multiple AutoscalingGroups with name: %q", name) } -func (e *AutoscalingGroup) normalize(c *fi.Context) error { +func (e *AutoscalingGroup) Normalize(c *fi.Context) error { sort.Strings(e.Metrics) + c.Cloud.(awsup.AWSCloud).AddTags(e.Name, e.Tags) return nil } // Run is responsible for running the task func (e *AutoscalingGroup) Run(c *fi.Context) error { - err := e.normalize(c) - if err != nil { - return err - } - c.Cloud.(awsup.AWSCloud).AddTags(e.Name, e.Tags) - return fi.DefaultDeltaRunMethod(e, c) } diff --git a/upup/pkg/fi/cloudup/awstasks/classic_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/classic_load_balancer.go index 0c47f91543..433c5ab180 100644 --- a/upup/pkg/fi/cloudup/awstasks/classic_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/classic_load_balancer.go @@ -76,6 +76,7 @@ type ClassicLoadBalancer struct { } var _ fi.CompareWithID = &ClassicLoadBalancer{} +var _ fi.TaskNormalize = &ClassicLoadBalancer{} func (e *ClassicLoadBalancer) CompareWithID() *string { return e.Name @@ -332,8 +333,7 @@ func (e *ClassicLoadBalancer) Find(c *fi.Context) (*ClassicLoadBalancer, error) e.LoadBalancerName = actual.LoadBalancerName } - // TODO: Make Normalize a standard method - actual.Normalize() + _ = actual.Normalize(c) klog.V(4).Infof("Found ELB %+v", actual) @@ -365,9 +365,6 @@ func (e *ClassicLoadBalancer) FindAddresses(context *fi.Context) ([]string, erro } func (e *ClassicLoadBalancer) Run(c *fi.Context) error { - // TODO: Make Normalize a standard method - e.Normalize() - return fi.DefaultDeltaRunMethod(e, c) } @@ -378,10 +375,11 @@ func (_ *ClassicLoadBalancer) ShouldCreate(a, e, changes *ClassicLoadBalancer) ( return true, nil } -func (e *ClassicLoadBalancer) Normalize() { +func (e *ClassicLoadBalancer) Normalize(c *fi.Context) error { // We need to sort our arrays consistently, so we don't get spurious changes sort.Stable(OrderSubnetsById(e.Subnets)) sort.Stable(OrderSecurityGroupsById(e.SecurityGroups)) + return nil } func (s *ClassicLoadBalancer) CheckChanges(a, e, changes *ClassicLoadBalancer) error { diff --git a/upup/pkg/fi/cloudup/awstasks/ebsvolume.go b/upup/pkg/fi/cloudup/awstasks/ebsvolume.go index abd3b5ef0f..1eed81f985 100644 --- a/upup/pkg/fi/cloudup/awstasks/ebsvolume.go +++ b/upup/pkg/fi/cloudup/awstasks/ebsvolume.go @@ -47,39 +47,15 @@ type EBSVolume struct { } var _ fi.CompareWithID = &EBSVolume{} +var _ fi.TaskNormalize = &EBSVolume{} func (e *EBSVolume) CompareWithID() *string { return e.ID } -type TaggableResource interface { - FindResourceID(c fi.Cloud) (*string, error) -} - -var _ TaggableResource = &EBSVolume{} - -func (e *EBSVolume) FindResourceID(c fi.Cloud) (*string, error) { - actual, err := e.find(c.(awsup.AWSCloud)) - if err != nil { - return nil, fmt.Errorf("error querying for EBSVolume: %v", err) - } - if actual == nil { - return nil, nil - } - - return actual.ID, nil -} - func (e *EBSVolume) Find(context *fi.Context) (*EBSVolume, error) { - actual, err := e.find(context.Cloud.(awsup.AWSCloud)) - if actual != nil && err == nil { - e.ID = actual.ID - } + cloud := context.Cloud.(awsup.AWSCloud) - return actual, err -} - -func (e *EBSVolume) find(cloud awsup.AWSCloud) (*EBSVolume, error) { filters := cloud.BuildFilters(e.Name) request := &ec2.DescribeVolumesInput{ Filters: filters, @@ -116,11 +92,17 @@ func (e *EBSVolume) find(cloud awsup.AWSCloud) (*EBSVolume, error) { // Avoid spurious changes actual.Lifecycle = e.Lifecycle + e.ID = actual.ID + return actual, nil } -func (e *EBSVolume) Run(c *fi.Context) error { +func (e *EBSVolume) Normalize(c *fi.Context) error { c.Cloud.(awsup.AWSCloud).AddTags(e.Name, e.Tags) + return nil +} + +func (e *EBSVolume) Run(c *fi.Context) error { return fi.DefaultDeltaRunMethod(e, c) } diff --git a/upup/pkg/fi/cloudup/awstasks/launchtemplate.go b/upup/pkg/fi/cloudup/awstasks/launchtemplate.go index ddb071ce6d..ba33be6cbf 100644 --- a/upup/pkg/fi/cloudup/awstasks/launchtemplate.go +++ b/upup/pkg/fi/cloudup/awstasks/launchtemplate.go @@ -93,6 +93,7 @@ type LaunchTemplate struct { var ( _ fi.CompareWithID = &LaunchTemplate{} _ fi.ProducesDeletions = &LaunchTemplate{} + _ fi.TaskNormalize = &LaunchTemplate{} _ fi.Deletion = &deleteLaunchTemplate{} ) @@ -135,16 +136,14 @@ func (t *LaunchTemplate) buildRootDevice(cloud awsup.AWSCloud) (map[string]*Bloc return bm, nil } -// Run is responsible for -func (t *LaunchTemplate) Run(c *fi.Context) error { - t.Normalize() - - return fi.DefaultDeltaRunMethod(t, c) +func (t *LaunchTemplate) Normalize(c *fi.Context) error { + sort.Stable(OrderSecurityGroupsById(t.SecurityGroups)) + return nil } -// Normalize is responsible for normalizing any data within the resource -func (t *LaunchTemplate) Normalize() { - sort.Stable(OrderSecurityGroupsById(t.SecurityGroups)) +// Run is responsible for +func (t *LaunchTemplate) Run(c *fi.Context) error { + return fi.DefaultDeltaRunMethod(t, c) } // CheckChanges is responsible for ensuring certains fields diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 4caa022809..1f21fefca5 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -73,6 +73,7 @@ type NetworkLoadBalancer struct { } var _ fi.CompareWithID = &NetworkLoadBalancer{} +var _ fi.TaskNormalize = &NetworkLoadBalancer{} func (e *NetworkLoadBalancer) CompareWithID() *string { return e.Name @@ -418,8 +419,7 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) e.LoadBalancerName = actual.LoadBalancerName } - // TODO: Make Normalize a standard method - actual.Normalize() + _ = actual.Normalize(c) actual.ForAPIServer = e.ForAPIServer actual.Lifecycle = e.Lifecycle @@ -453,17 +453,15 @@ func (e *NetworkLoadBalancer) FindAddresses(context *fi.Context) ([]string, erro } func (e *NetworkLoadBalancer) Run(c *fi.Context) error { - // TODO: Make Normalize a standard method - e.Normalize() - return fi.DefaultDeltaRunMethod(e, c) } -func (e *NetworkLoadBalancer) Normalize() { +func (e *NetworkLoadBalancer) Normalize(c *fi.Context) error { // We need to sort our arrays consistently, so we don't get spurious changes sort.Stable(OrderSubnetMappingsByName(e.SubnetMappings)) sort.Stable(OrderListenersByPort(e.Listeners)) sort.Stable(OrderTargetGroupsByName(e.TargetGroups)) + return nil } func (*NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) error { diff --git a/upup/pkg/fi/cloudup/awstasks/sshkey.go b/upup/pkg/fi/cloudup/awstasks/sshkey.go index ddb98848cd..76f72b9271 100644 --- a/upup/pkg/fi/cloudup/awstasks/sshkey.go +++ b/upup/pkg/fi/cloudup/awstasks/sshkey.go @@ -47,6 +47,7 @@ type SSHKey struct { } var _ fi.CompareWithID = &SSHKey{} +var _ fi.TaskNormalize = &SSHKey{} func (e *SSHKey) CompareWithID() *string { return e.Name @@ -120,7 +121,7 @@ func (e *SSHKey) find(cloud awsup.AWSCloud) (*SSHKey, error) { return actual, nil } -func (e *SSHKey) Run(c *fi.Context) error { +func (e *SSHKey) Normalize(c *fi.Context) error { if e.KeyFingerprint == nil && e.PublicKey != nil { publicKey, err := fi.ResourceAsString(e.PublicKey) if err != nil { @@ -135,6 +136,10 @@ func (e *SSHKey) Run(c *fi.Context) error { e.KeyFingerprint = &keyFingerprint } + return nil +} + +func (e *SSHKey) Run(c *fi.Context) error { return fi.DefaultDeltaRunMethod(e, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/disk.go b/upup/pkg/fi/cloudup/azuretasks/disk.go index c6148520cf..af670615f6 100644 --- a/upup/pkg/fi/cloudup/azuretasks/disk.go +++ b/upup/pkg/fi/cloudup/azuretasks/disk.go @@ -40,6 +40,7 @@ type Disk struct { var ( _ fi.Task = &Disk{} _ fi.CompareWithID = &Disk{} + _ fi.TaskNormalize = &Disk{} ) // CompareWithID returns the Name of the Disk. @@ -76,9 +77,13 @@ func (d *Disk) Find(c *fi.Context) (*Disk, error) { }, nil } +func (d *Disk) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(d.Tags) + return nil +} + // Run implements fi.Task.Run. func (d *Disk) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(d.Tags) return fi.DefaultDeltaRunMethod(d, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/disk_test.go b/upup/pkg/fi/cloudup/azuretasks/disk_test.go index 2119e27004..ab860aba34 100644 --- a/upup/pkg/fi/cloudup/azuretasks/disk_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/disk_test.go @@ -138,7 +138,11 @@ func TestDiskRun(t *testing.T) { } vmss := newTestDisk() - err := vmss.Run(ctx) + err := vmss.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = vmss.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/azuretasks/loadbalancer.go b/upup/pkg/fi/cloudup/azuretasks/loadbalancer.go index 904c4641f4..cdfbe996a1 100644 --- a/upup/pkg/fi/cloudup/azuretasks/loadbalancer.go +++ b/upup/pkg/fi/cloudup/azuretasks/loadbalancer.go @@ -45,6 +45,7 @@ type LoadBalancer struct { var ( _ fi.Task = &LoadBalancer{} _ fi.CompareWithID = &LoadBalancer{} + _ fi.TaskNormalize = &LoadBalancer{} ) // CompareWithID returns the Name of the LoadBalancer @@ -98,9 +99,13 @@ func (lb *LoadBalancer) Find(c *fi.Context) (*LoadBalancer, error) { }, nil } +func (lb *LoadBalancer) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(lb.Tags) + return nil +} + // Run implements fi.Task.Run. func (lb *LoadBalancer) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(lb.Tags) return fi.DefaultDeltaRunMethod(lb, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/loadbalancer_test.go b/upup/pkg/fi/cloudup/azuretasks/loadbalancer_test.go index 95ae9da635..0d230d0a96 100644 --- a/upup/pkg/fi/cloudup/azuretasks/loadbalancer_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/loadbalancer_test.go @@ -145,7 +145,11 @@ func TestLoadBalancerRun(t *testing.T) { } lb := newTestLoadBalancer() - err := lb.Run(ctx) + err := lb.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = lb.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/azuretasks/publicipaddress.go b/upup/pkg/fi/cloudup/azuretasks/publicipaddress.go index bbd5cf62c3..5e3ce17c81 100644 --- a/upup/pkg/fi/cloudup/azuretasks/publicipaddress.go +++ b/upup/pkg/fi/cloudup/azuretasks/publicipaddress.go @@ -39,6 +39,7 @@ type PublicIPAddress struct { var ( _ fi.Task = &PublicIPAddress{} _ fi.CompareWithID = &PublicIPAddress{} + _ fi.TaskNormalize = &PublicIPAddress{} ) // CompareWithID returns the Name of the Public IP Address @@ -75,9 +76,13 @@ func (p *PublicIPAddress) Find(c *fi.Context) (*PublicIPAddress, error) { }, nil } +func (p *PublicIPAddress) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(p.Tags) + return nil +} + // Run implements fi.Task.Run. func (p *PublicIPAddress) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(p.Tags) return fi.DefaultDeltaRunMethod(p, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/publicipaddress_test.go b/upup/pkg/fi/cloudup/azuretasks/publicipaddress_test.go index 4d03f3a84a..4fccbe897d 100644 --- a/upup/pkg/fi/cloudup/azuretasks/publicipaddress_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/publicipaddress_test.go @@ -117,7 +117,11 @@ func TestPublicIPAddressRun(t *testing.T) { } lb := newTestPublicIPAddress() - err := lb.Run(ctx) + err := lb.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = lb.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/azuretasks/resourcegroup.go b/upup/pkg/fi/cloudup/azuretasks/resourcegroup.go index 5a697823ed..81a967d949 100644 --- a/upup/pkg/fi/cloudup/azuretasks/resourcegroup.go +++ b/upup/pkg/fi/cloudup/azuretasks/resourcegroup.go @@ -41,6 +41,7 @@ type ResourceGroup struct { var ( _ fi.Task = &ResourceGroup{} _ fi.CompareWithID = &ResourceGroup{} + _ fi.TaskNormalize = &ResourceGroup{} ) // CompareWithID returns the Name of the VM Scale Set. @@ -74,9 +75,13 @@ func (r *ResourceGroup) Find(c *fi.Context) (*ResourceGroup, error) { }, nil } +func (r *ResourceGroup) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(r.Tags) + return nil +} + // Run implements fi.Task.Run. func (r *ResourceGroup) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(r.Tags) return fi.DefaultDeltaRunMethod(r, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/resourcegroup_test.go b/upup/pkg/fi/cloudup/azuretasks/resourcegroup_test.go index 4272c1b95b..0b0750e25d 100644 --- a/upup/pkg/fi/cloudup/azuretasks/resourcegroup_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/resourcegroup_test.go @@ -156,7 +156,11 @@ func TestResourceGroupRun(t *testing.T) { key: to.StringPtr(val), }, } - err := rg.Run(ctx) + err := rg.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = rg.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/azuretasks/routetable.go b/upup/pkg/fi/cloudup/azuretasks/routetable.go index 579b745c2f..08f7afec26 100644 --- a/upup/pkg/fi/cloudup/azuretasks/routetable.go +++ b/upup/pkg/fi/cloudup/azuretasks/routetable.go @@ -39,6 +39,7 @@ type RouteTable struct { var ( _ fi.Task = &RouteTable{} _ fi.CompareWithID = &RouteTable{} + _ fi.TaskNormalize = &RouteTable{} ) // CompareWithID returns the Name of the VM Scale Set. @@ -73,9 +74,13 @@ func (r *RouteTable) Find(c *fi.Context) (*RouteTable, error) { }, nil } +func (r *RouteTable) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(r.Tags) + return nil +} + // Run implements fi.Task.Run. func (r *RouteTable) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(r.Tags) return fi.DefaultDeltaRunMethod(r, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/virtualnetwork.go b/upup/pkg/fi/cloudup/azuretasks/virtualnetwork.go index 9fa34ec365..7f5bb92f9f 100644 --- a/upup/pkg/fi/cloudup/azuretasks/virtualnetwork.go +++ b/upup/pkg/fi/cloudup/azuretasks/virtualnetwork.go @@ -43,6 +43,7 @@ type VirtualNetwork struct { var ( _ fi.Task = &VirtualNetwork{} _ fi.CompareWithID = &VirtualNetwork{} + _ fi.TaskNormalize = &VirtualNetwork{} ) // CompareWithID returns the Name of the VM Scale Set. @@ -84,9 +85,13 @@ func (n *VirtualNetwork) Find(c *fi.Context) (*VirtualNetwork, error) { }, nil } +func (n *VirtualNetwork) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(n.Tags) + return nil +} + // Run implements fi.Task.Run. func (n *VirtualNetwork) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(n.Tags) return fi.DefaultDeltaRunMethod(n, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/virtualnetwork_test.go b/upup/pkg/fi/cloudup/azuretasks/virtualnetwork_test.go index 9c9264b36d..b91491a86a 100644 --- a/upup/pkg/fi/cloudup/azuretasks/virtualnetwork_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/virtualnetwork_test.go @@ -141,7 +141,11 @@ func TestVirtualNetworkRun(t *testing.T) { key: to.StringPtr(val), }, } - err := vnet.Run(ctx) + err := vnet.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = vnet.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/azuretasks/vmscaleset.go b/upup/pkg/fi/cloudup/azuretasks/vmscaleset.go index 63267cdf1f..a505fb8dd9 100644 --- a/upup/pkg/fi/cloudup/azuretasks/vmscaleset.go +++ b/upup/pkg/fi/cloudup/azuretasks/vmscaleset.go @@ -120,6 +120,8 @@ type VMScaleSet struct { PrincipalID *string } +var _ fi.TaskNormalize = &VMScaleSet{} + // VMScaleSetStorageProfile wraps *compute.VirtualMachineScaleSetStorageProfile // and implements fi.HasDependencies. // @@ -240,9 +242,13 @@ func (s *VMScaleSet) Find(c *fi.Context) (*VMScaleSet, error) { return vmss, nil } +func (s *VMScaleSet) Normalize(c *fi.Context) error { + c.Cloud.(azure.AzureCloud).AddClusterTags(s.Tags) + return nil +} + // Run implements fi.Task.Run. func (s *VMScaleSet) Run(c *fi.Context) error { - c.Cloud.(azure.AzureCloud).AddClusterTags(s.Tags) return fi.DefaultDeltaRunMethod(s, c) } diff --git a/upup/pkg/fi/cloudup/azuretasks/vmscaleset_test.go b/upup/pkg/fi/cloudup/azuretasks/vmscaleset_test.go index 2df118fdec..f8349754ae 100644 --- a/upup/pkg/fi/cloudup/azuretasks/vmscaleset_test.go +++ b/upup/pkg/fi/cloudup/azuretasks/vmscaleset_test.go @@ -307,7 +307,11 @@ func TestVMScaleSetRun(t *testing.T) { } vmss := newTestVMScaleSet() - err := vmss.Run(ctx) + err := vmss.Normalize(ctx) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + err = vmss.Run(ctx) if err != nil { t.Fatalf("unexpected error: %s", err) } diff --git a/upup/pkg/fi/cloudup/gcetasks/firewallrule.go b/upup/pkg/fi/cloudup/gcetasks/firewallrule.go index 3d525bb167..7d80a57101 100644 --- a/upup/pkg/fi/cloudup/gcetasks/firewallrule.go +++ b/upup/pkg/fi/cloudup/gcetasks/firewallrule.go @@ -48,6 +48,7 @@ type FirewallRule struct { } var _ fi.CompareWithID = &FirewallRule{} +var _ fi.TaskNormalize = &FirewallRule{} func (e *FirewallRule) CompareWithID() *string { return e.Name @@ -82,15 +83,12 @@ func (e *FirewallRule) Find(c *fi.Context) (*FirewallRule, error) { } func (e *FirewallRule) Run(c *fi.Context) error { - if err := e.sanityCheck(); err != nil { - return err - } return fi.DefaultDeltaRunMethod(e, c) } -// sanityCheck applies some validation that isn't technically required, +// Normalize applies some validation that isn't technically required, // but avoids some problems with surprising behaviours. -func (e *FirewallRule) sanityCheck() error { +func (e *FirewallRule) Normalize(c *fi.Context) error { if !e.Disabled { // Treat it as an error if SourceRanges _and_ SourceTags empty with Disabled=false // this is interpreted as SourceRanges="0.0.0.0/0", which is likely not what was intended. diff --git a/upup/pkg/fi/cloudup/openstacktasks/sshkey.go b/upup/pkg/fi/cloudup/openstacktasks/sshkey.go index 02a23554d1..89f582f462 100644 --- a/upup/pkg/fi/cloudup/openstacktasks/sshkey.go +++ b/upup/pkg/fi/cloudup/openstacktasks/sshkey.go @@ -39,6 +39,7 @@ type SSHKey struct { } var _ fi.CompareWithID = &SSHKey{} +var _ fi.TaskNormalize = &SSHKey{} func (e *SSHKey) CompareWithID() *string { return e.Name @@ -69,7 +70,7 @@ func (e *SSHKey) Find(c *fi.Context) (*SSHKey, error) { return actual, nil } -func (e *SSHKey) Run(c *fi.Context) error { +func (e *SSHKey) Normalize(c *fi.Context) error { if e.KeyFingerprint == nil && e.PublicKey != nil { publicKey, err := fi.ResourceAsString(e.PublicKey) if err != nil { @@ -83,6 +84,10 @@ func (e *SSHKey) Run(c *fi.Context) error { klog.V(2).Infof("Computed SSH key fingerprint as %q", keyFingerprint) e.KeyFingerprint = &keyFingerprint } + return nil +} + +func (e *SSHKey) Run(c *fi.Context) error { return fi.DefaultDeltaRunMethod(e, c) } diff --git a/upup/pkg/fi/cloudup/openstacktasks/volume.go b/upup/pkg/fi/cloudup/openstacktasks/volume.go index 1a91827fb3..e0a9253c79 100644 --- a/upup/pkg/fi/cloudup/openstacktasks/volume.go +++ b/upup/pkg/fi/cloudup/openstacktasks/volume.go @@ -37,6 +37,7 @@ type Volume struct { } var _ fi.CompareWithID = &Volume{} +var _ fi.TaskNormalize = &Volume{} func (c *Volume) CompareWithID() *string { return c.ID @@ -77,12 +78,15 @@ func (c *Volume) Find(context *fi.Context) (*Volume, error) { return actual, nil } -func (c *Volume) Run(context *fi.Context) error { +func (c *Volume) Normalize(context *fi.Context) error { cloud := context.Cloud.(openstack.OpenstackCloud) for k, v := range cloud.GetCloudTags() { c.Tags[k] = v } + return nil +} +func (c *Volume) Run(context *fi.Context) error { return fi.DefaultDeltaRunMethod(c, context) } diff --git a/upup/pkg/fi/executor.go b/upup/pkg/fi/executor.go index 16c2d5a286..401314dc8f 100644 --- a/upup/pkg/fi/executor.go +++ b/upup/pkg/fi/executor.go @@ -184,6 +184,14 @@ func (e *executor) forkJoin(tasks []*taskState) []error { results[index] = fmt.Errorf("function panic") defer wg.Done() klog.V(2).Infof("Executing task %q: %v\n", ts.key, ts.task) + + if taskNormalize, ok := ts.task.(TaskNormalize); ok { + if err := taskNormalize.Normalize(e.context); err != nil { + results[index] = err + return + } + } + results[index] = ts.task.Run(e.context) }(tasks[i], i) } diff --git a/upup/pkg/fi/fitasks/keypair.go b/upup/pkg/fi/fitasks/keypair.go index 52ac4fc283..8072cd0899 100644 --- a/upup/pkg/fi/fitasks/keypair.go +++ b/upup/pkg/fi/fitasks/keypair.go @@ -54,6 +54,7 @@ type Keypair struct { var ( _ fi.HasCheckExisting = &Keypair{} _ fi.HasName = &Keypair{} + _ fi.TaskNormalize = &Keypair{} ) // It's important always to check for the existing key, so we don't regenerate keys e.g. on terraform @@ -115,14 +116,10 @@ func (e *Keypair) Find(c *fi.Context) (*Keypair, error) { } func (e *Keypair) Run(c *fi.Context) error { - err := e.normalize() - if err != nil { - return err - } return fi.DefaultDeltaRunMethod(e, c) } -func (e *Keypair) normalize() error { +func (e *Keypair) Normalize(c *fi.Context) error { var alternateNames []string for _, s := range e.AlternateNames { diff --git a/upup/pkg/fi/task.go b/upup/pkg/fi/task.go index b7f6480b8e..f8325f695d 100644 --- a/upup/pkg/fi/task.go +++ b/upup/pkg/fi/task.go @@ -30,10 +30,20 @@ type Task interface { // TaskPreRun is implemented by tasks that perform some initial validation. type TaskPreRun interface { - // PreRun will be run for all TaskPreRuns, before any Run functions are invoked. + Task + // PreRun will be run for all TaskPreRuns, before the Run function of any Task is invoked. + // Invocation order does not pay attention to Task dependencies. PreRun(*Context) error } +// TaskNormalize is implemented by tasks that perform some initial normalization. +type TaskNormalize interface { + Task + // Normalize will be run for all TaskNormalizes, before the Run function of + // the TaskNormalize and after the Run function of any Task it is dependent on. + Normalize(*Context) error +} + // TaskAsString renders the task for debug output // TODO: Use reflection to make this cleaner: don't recurse into tasks - print their names instead // also print resources in a cleaner way (use the resource source information?) @@ -42,6 +52,7 @@ func TaskAsString(t Task) string { } type HasCheckExisting interface { + Task CheckExisting(c *Context) bool }