Merge pull request #14927 from justinsb/ensuretask_should_panic

EnsureTask should panic

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)
+		ctx.EnsureTask(kubeConfig)
-		if err != nil {
-			return nil, err
-		}
 
 		return kubeConfig.GetConfig(), nil
 	} else {

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)
+			c.EnsureTask(t)
-			if err != nil {
-				return err
-			}
 		}
 	}
 	{

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 {
+			c.EnsureTask(t)
-				return err
-			}
 			clb.SecurityGroups = append(clb.SecurityGroups, t)
 		}
 	}

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 {
+			c.EnsureTask(sgTask)
-				return nil, err
-			}
 			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 {
+		c.EnsureTask(sgTask)
-			return nil, err
-		}
 		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 {
+			c.EnsureTask(lb)
-				return nil, err
-			}
 		}
 
 		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 {
+			c.EnsureTask(tg)
-				return nil, err
-			}
 		}
 	}
 	sort.Stable(awstasks.OrderLoadBalancersByName(t.LoadBalancers))

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
-				}
 			}
 		}
 	}

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)
+			c.EnsureTask(inst)
-			if err != nil {
-				return err
-			}
 			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)
+			c.EnsureTask(nat)
-			if err != nil {
-				return err
-			}
 			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)
+			c.EnsureTask(internetGW)
-			if err != nil {
-				return err
-			}
 			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)
+			c.EnsureTask(eigw)
-			if err != nil {
-				return err
-			}
 			t.EgressOnlyInternetGateway = eigw
 			c.AddTask(t)
 		}

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 {
+		c.EnsureTask(sg)
-			return nil, err
-		}
 		securityGroups = append(securityGroups, sg)
 	}
 

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{

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

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.
+// if they are different we panic; otherwise it's too easy to forget to check the error code,
-func (c *ModelBuilderContext[T]) EnsureTask(task Task[T]) error {
+// 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.