From b7d9319fff2b75346ca0e2dd5cd20b2b0d69fc6e Mon Sep 17 00:00:00 2001 From: justinsb Date: Sun, 1 Jan 2023 13:17:16 -0500 Subject: [PATCH] EnsureTask should panic on error This means that we automatically check the error code. A linter could detect errors here (maybe), but in practice we can't recover from errors here anyway. --- nodeup/pkg/model/context.go | 5 +---- nodeup/pkg/model/kubelet.go | 5 +---- pkg/model/awsmodel/api_loadbalancer.go | 4 +--- pkg/model/awsmodel/autoscalinggroup.go | 16 ++++------------ pkg/model/awsmodel/dns.go | 13 ++++--------- pkg/model/awsmodel/network.go | 20 ++++---------------- pkg/model/awsmodel/spotinst.go | 4 +--- pkg/model/components/etcdmanager/model.go | 6 ++---- upup/pkg/fi/nodeup/nodetasks/issue_cert.go | 10 ++-------- upup/pkg/fi/task.go | 12 +++++++----- 10 files changed, 27 insertions(+), 68 deletions(-) diff --git a/nodeup/pkg/model/context.go b/nodeup/pkg/model/context.go index 3d30fe2922..ac6135dfe2 100644 --- a/nodeup/pkg/model/context.go +++ b/nodeup/pkg/model/context.go @@ -294,10 +294,7 @@ func (c *NodeupModelContext) BuildBootstrapKubeconfig(name string, ctx *fi.Nodeu kubeConfig.ServerURL = "https://" + c.APIInternalName() } - err = ctx.EnsureTask(kubeConfig) - if err != nil { - return nil, err - } + ctx.EnsureTask(kubeConfig) return kubeConfig.GetConfig(), nil } else { diff --git a/nodeup/pkg/model/kubelet.go b/nodeup/pkg/model/kubelet.go index 8016f2a237..a0b79814e2 100644 --- a/nodeup/pkg/model/kubelet.go +++ b/nodeup/pkg/model/kubelet.go @@ -115,10 +115,7 @@ func (b *KubeletBuilder) Build(c *fi.NodeupModelBuilderContext) error { if err != nil { return err } - err = c.EnsureTask(t) - if err != nil { - return err - } + c.EnsureTask(t) } } { diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index d02c93e871..e9c6cd5644 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -520,9 +520,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.CloudupModelBuilderContext) error { ID: fi.PtrTo(id), Shared: fi.PtrTo(true), } - if err := c.EnsureTask(t); err != nil { - return err - } + c.EnsureTask(t) clb.SecurityGroups = append(clb.SecurityGroups, t) } } diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index 8d94b0aabf..f1bf854a9b 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -354,9 +354,7 @@ func (b *AutoscalingGroupModelBuilder) buildSecurityGroups(c *fi.CloudupModelBui Name: fi.PtrTo("nlb-" + id), Shared: fi.PtrTo(true), } - if err := c.EnsureTask(sgTask); err != nil { - return nil, err - } + c.EnsureTask(sgTask) securityGroups = append(securityGroups, sgTask) } } @@ -369,9 +367,7 @@ func (b *AutoscalingGroupModelBuilder) buildSecurityGroups(c *fi.CloudupModelBui Name: fi.PtrTo(id), Shared: fi.PtrTo(true), } - if err := c.EnsureTask(sgTask); err != nil { - return nil, err - } + c.EnsureTask(sgTask) securityGroups = append(securityGroups, sgTask) } @@ -480,9 +476,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.CloudupMo Shared: fi.PtrTo(true), } t.LoadBalancers = append(t.LoadBalancers, lb) - if err := c.EnsureTask(lb); err != nil { - return nil, err - } + c.EnsureTask(lb) } if extLB.TargetGroupARN != nil { @@ -497,9 +491,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.CloudupMo Shared: fi.PtrTo(true), } t.TargetGroups = append(t.TargetGroups, tg) - if err := c.EnsureTask(tg); err != nil { - return nil, err - } + c.EnsureTask(tg) } } sort.Stable(awstasks.OrderLoadBalancersByName(t.LoadBalancers)) diff --git a/pkg/model/awsmodel/dns.go b/pkg/model/awsmodel/dns.go index c46e7a02d0..1be293fcc0 100644 --- a/pkg/model/awsmodel/dns.go +++ b/pkg/model/awsmodel/dns.go @@ -67,7 +67,8 @@ func (b *DNSModelBuilder) ensureDNSZone(c *fi.CloudupModelBuilderContext) error dnsZone.DNSName = fi.PtrTo(b.Cluster.Spec.DNSZone) } - return c.EnsureTask(dnsZone) + c.EnsureTask(dnsZone) + return nil } func (b *DNSModelBuilder) Build(c *fi.CloudupModelBuilderContext) error { @@ -131,7 +132,7 @@ func (b *DNSModelBuilder) Build(c *fi.CloudupModelBuilderContext) error { // Using EnsureTask as APIInternalName() and APIPublicName could be the same { - err := c.EnsureTask(&awstasks.DNSName{ + c.EnsureTask(&awstasks.DNSName{ Name: fi.PtrTo(b.Cluster.APIInternalName()), ResourceName: fi.PtrTo(b.Cluster.APIInternalName()), Lifecycle: b.Lifecycle, @@ -139,12 +140,9 @@ func (b *DNSModelBuilder) Build(c *fi.CloudupModelBuilderContext) error { ResourceType: fi.PtrTo("A"), TargetLoadBalancer: targetLoadBalancer, }) - if err != nil { - return err - } } if b.UseIPv6ForAPI() { - err := c.EnsureTask(&awstasks.DNSName{ + c.EnsureTask(&awstasks.DNSName{ Name: fi.PtrTo(b.Cluster.APIInternalName() + "-AAAA"), ResourceName: fi.PtrTo(b.Cluster.APIInternalName()), Lifecycle: b.Lifecycle, @@ -152,9 +150,6 @@ func (b *DNSModelBuilder) Build(c *fi.CloudupModelBuilderContext) error { ResourceType: fi.PtrTo("AAAA"), TargetLoadBalancer: targetLoadBalancer, }) - if err != nil { - return err - } } } } diff --git a/pkg/model/awsmodel/network.go b/pkg/model/awsmodel/network.go index ff1091fa77..175625998a 100644 --- a/pkg/model/awsmodel/network.go +++ b/pkg/model/awsmodel/network.go @@ -666,10 +666,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro ID: fi.PtrTo(r.Target), Shared: fi.PtrTo(true), } - err := c.EnsureTask(inst) - if err != nil { - return err - } + c.EnsureTask(inst) t.Instance = inst c.AddTask(t) } else if strings.HasPrefix(r.Target, "nat-") { @@ -679,10 +676,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro ID: fi.PtrTo(r.Target), Shared: fi.PtrTo(true), } - err := c.EnsureTask(nat) - if err != nil { - return err - } + c.EnsureTask(nat) t.NatGateway = nat c.AddTask(t) } else if strings.HasPrefix(r.Target, "tgw-") { @@ -695,10 +689,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro ID: fi.PtrTo(r.Target), Shared: fi.PtrTo(true), } - err := c.EnsureTask(internetGW) - if err != nil { - return err - } + c.EnsureTask(internetGW) t.InternetGateway = internetGW c.AddTask(t) } else if strings.HasPrefix(r.Target, "eigw-") { @@ -708,10 +699,7 @@ func addAdditionalRoutes(routes []kops.RouteSpec, sbName string, rt *awstasks.Ro ID: fi.PtrTo(r.Target), Shared: fi.PtrTo(true), } - err := c.EnsureTask(eigw) - if err != nil { - return err - } + c.EnsureTask(eigw) t.EgressOnlyInternetGateway = eigw c.AddTask(t) } diff --git a/pkg/model/awsmodel/spotinst.go b/pkg/model/awsmodel/spotinst.go index a186afc19e..28f2b13b71 100644 --- a/pkg/model/awsmodel/spotinst.go +++ b/pkg/model/awsmodel/spotinst.go @@ -640,9 +640,7 @@ func (b *SpotInstanceGroupModelBuilder) buildSecurityGroups(c *fi.CloudupModelBu Name: fi.PtrTo(id), Shared: fi.PtrTo(true), } - if err := c.EnsureTask(sg); err != nil { - return nil, err - } + c.EnsureTask(sg) securityGroups = append(securityGroups, sg) } diff --git a/pkg/model/components/etcdmanager/model.go b/pkg/model/components/etcdmanager/model.go index ffbd78164d..b2849e1da3 100644 --- a/pkg/model/components/etcdmanager/model.go +++ b/pkg/model/components/etcdmanager/model.go @@ -129,14 +129,12 @@ func (b *EtcdManagerBuilder) Build(c *fi.CloudupModelBuilderContext) error { }) // Because API server can only have a single client-cert, we need to share a client CA - if err := c.EnsureTask(&fitasks.Keypair{ + c.EnsureTask(&fitasks.Keypair{ Name: fi.PtrTo("etcd-clients-ca"), Lifecycle: b.Lifecycle, Subject: "cn=etcd-clients-ca", Type: "ca", - }); err != nil { - return err - } + }) if etcdCluster.Name == "cilium" { clientsCaCilium := &fitasks.Keypair{ diff --git a/upup/pkg/fi/nodeup/nodetasks/issue_cert.go b/upup/pkg/fi/nodeup/nodetasks/issue_cert.go index f9236fb983..eff790cc2c 100644 --- a/upup/pkg/fi/nodeup/nodetasks/issue_cert.go +++ b/upup/pkg/fi/nodeup/nodetasks/issue_cert.go @@ -89,14 +89,11 @@ func (i *IssueCert) GetResources() (certResource, keyResource, caResource *fi.No func (i *IssueCert) AddFileTasks(c *fi.NodeupModelBuilderContext, dir string, name string, caName string, owner *string) error { certResource, keyResource, caResource := i.GetResources() - err := c.EnsureTask(&File{ + c.EnsureTask(&File{ Path: dir, Type: FileType_Directory, Mode: fi.PtrTo("0755"), }) - if err != nil { - return err - } c.AddTask(&File{ Path: filepath.Join(dir, name+".crt"), @@ -115,16 +112,13 @@ func (i *IssueCert) AddFileTasks(c *fi.NodeupModelBuilderContext, dir string, na }) if caName != "" { - err = c.EnsureTask(&File{ + c.EnsureTask(&File{ Path: filepath.Join(dir, caName+".crt"), Contents: caResource, Type: FileType_File, Mode: fi.PtrTo("0644"), Owner: owner, }) - if err != nil { - return nil - } } return nil diff --git a/upup/pkg/fi/task.go b/upup/pkg/fi/task.go index b1dfc81b9d..6838273ad9 100644 --- a/upup/pkg/fi/task.go +++ b/upup/pkg/fi/task.go @@ -138,8 +138,9 @@ func (c *ModelBuilderContext[T]) AddTask(task Task[T]) { // EnsureTask ensures that the specified task is configured. // It adds the task if it does not already exist. // If it does exist, it verifies that the existing task reflect.DeepEqual the new task, -// if they are different an error is returned. -func (c *ModelBuilderContext[T]) EnsureTask(task Task[T]) error { +// if they are different we panic; otherwise it's too easy to forget to check the error code, +// and realistically we have yet to find a scenario where we can recover from an error here. +func (c *ModelBuilderContext[T]) EnsureTask(task Task[T]) { task = c.setLifecycleOverride(task) key := buildTaskKey(task) @@ -147,16 +148,17 @@ func (c *ModelBuilderContext[T]) EnsureTask(task Task[T]) error { if found { if reflect.DeepEqual(task, existing) { klog.V(8).Infof("EnsureTask ignoring identical ") - return nil + return } klog.Warningf("EnsureTask found task mismatch for %q", key) klog.Warningf("\tExisting: %v", existing) klog.Warningf("\tNew: %v", task) - return fmt.Errorf("cannot add different task with same key %q", key) + // c.Errorf("cannot add different task with same key %q", key) + klog.Fatalf("cannot add different task with same key %q", key) + return } c.Tasks[key] = task - return nil } // setLifecycleOverride determines if a Lifecycle is in the LifecycleOverrides map for the current task.