Merge pull request #14825 from johngmyers/nodeup-update-pkg

Move Cluster into CloudupSubContext

pkg/apis/nodeup/config.go

@@ -19,8 +19,8 @@ package nodeup
 import (
 	"strings"
 
+	"github.com/aws/aws-sdk-go/aws"
 	"k8s.io/kops/pkg/apis/kops"
-	"k8s.io/kops/upup/pkg/fi"
 	"k8s.io/kops/util/pkg/architectures"
 )
 
@@ -85,7 +85,7 @@ type Config struct {
 // BootConfig is the configuration for the nodeup binary that might be too big to fit in userdata.
 type BootConfig struct {
 	// CloudProvider is the cloud provider in use.
-	CloudProvider string
+	CloudProvider kops.CloudProviderID
 	// ConfigBase is the base VFS path for config objects.
 	ConfigBase *string `json:",omitempty"`
 	// ConfigServer holds the configuration for the configuration server.
@@ -155,7 +155,7 @@ func NewConfig(cluster *kops.Cluster, instanceGroup *kops.InstanceGroup) (*Confi
 	}
 
 	bootConfig := BootConfig{
-		CloudProvider:     string(cluster.Spec.GetCloudProvider()),
+		CloudProvider:     cluster.Spec.GetCloudProvider(),
 		InstanceGroupName: instanceGroup.ObjectMeta.Name,
 		InstanceGroupRole: role,
 	}
@@ -176,7 +176,7 @@ func NewConfig(cluster *kops.Cluster, instanceGroup *kops.InstanceGroup) (*Confi
 	}
 
 	if cluster.Spec.Networking.AmazonVPC != nil {
-		config.DefaultMachineType = fi.PtrTo(strings.Split(instanceGroup.Spec.MachineType, ",")[0])
+		config.DefaultMachineType = aws.String(strings.Split(instanceGroup.Spec.MachineType, ",")[0])
 	}
 
 	if UsesInstanceIDForNodeName(cluster) {

pkg/model/bootstrapscript.go

@@ -350,7 +350,7 @@ func (b *BootstrapScript) Run(c *fi.CloudupContext) error {
 
 	{
 		nodeupScript.EnvironmentVariables = func() (string, error) {
-			env, err := b.buildEnvironmentVariables(c.Cluster)
+			env, err := b.buildEnvironmentVariables(c.T.Cluster)
 			if err != nil {
 				return "", err
 			}
@@ -370,11 +370,11 @@ func (b *BootstrapScript) Run(c *fi.CloudupContext) error {
 		}
 
 		nodeupScript.ProxyEnv = func() (string, error) {
-			return b.createProxyEnv(c.Cluster.Spec.Networking.EgressProxy)
+			return b.createProxyEnv(c.T.Cluster.Spec.Networking.EgressProxy)
 		}
 
 		nodeupScript.ClusterSpec = func() (string, error) {
-			cs := c.Cluster.Spec
+			cs := c.T.Cluster.Spec
 
 			spec := make(map[string]interface{})
 			spec["cloudConfig"] = cs.CloudConfig
@@ -433,7 +433,7 @@ func (b *BootstrapScript) Run(c *fi.CloudupContext) error {
 	// See https://github.com/kubernetes/kops/issues/10206 for details.
 	nodeupScript.SetSysctls = setSysctls()
 
-	nodeupScript.CloudProvider = string(c.Cluster.Spec.GetCloudProvider())
+	nodeupScript.CloudProvider = string(c.T.Cluster.Spec.GetCloudProvider())
 
 	nodeupScriptResource, err := nodeupScript.Build()
 	if err != nil {

pkg/model/bootstrapscript_test.go

@@ -190,7 +190,7 @@ func TestBootstrapUserData(t *testing.T) {
 		}
 
 		require.Contains(t, c.Tasks, "BootstrapScript/testIG")
-		err = c.Tasks["BootstrapScript/testIG"].Run(&fi.CloudupContext{Cluster: cluster})
+		err = c.Tasks["BootstrapScript/testIG"].Run(&fi.CloudupContext{T: fi.CloudupSubContext{Cluster: cluster}})
 		require.NoError(t, err, "running task")
 
 		actual, err := fi.ResourceAsString(res)

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

@@ -441,7 +441,7 @@ func (e *NetworkLoadBalancer) FindAddresses(context *fi.CloudupContext) ([]strin
 	var addresses []string
 
 	cloud := context.T.Cloud.(awsup.AWSCloud)
-	cluster := context.Cluster
+	cluster := context.T.Cluster
 
 	{
 		lb, err := cloud.FindELBV2ByNameTag(e.Tags["Name"])

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

@@ -74,7 +74,7 @@ func (e *RouteTable) Find(c *fi.CloudupContext) (*RouteTable, error) {
 		var filters []*ec2.Filter
 		filters = append(filters, &ec2.Filter{
 			Name:   aws.String("tag-key"),
-			Values: aws.StringSlice([]string{"kubernetes.io/cluster/" + c.Cluster.Name}),
+			Values: aws.StringSlice([]string{"kubernetes.io/cluster/" + c.T.Cluster.Name}),
 		})
 		filters = append(filters, &ec2.Filter{
 			Name:   aws.String("tag:" + awsup.TagNameKopsRole),

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

@@ -50,7 +50,7 @@ func (e *Subnet) CompareWithID() *string {
 
 func (e *Subnet) Find(c *fi.CloudupContext) (*Subnet, error) {
 	cloud := c.T.Cloud.(gce.GCECloud)
-	_, project, err := gce.ParseNameAndProjectFromNetworkID(c.Cluster.Spec.Networking.NetworkID)
+	_, project, err := gce.ParseNameAndProjectFromNetworkID(c.T.Cluster.Spec.Networking.NetworkID)
 	if err != nil {
 		return nil, fmt.Errorf("error parsing network name from cluster spec: %w", err)
 	} else if project == "" {

upup/pkg/fi/cloudup/hetznertasks/servergroup.go

@@ -59,7 +59,7 @@ func (v *ServerGroup) Find(c *fi.CloudupContext) (*ServerGroup, error) {
 	client := cloud.ServerClient()
 
 	labelSelector := []string{
-		fmt.Sprintf("%s=%s", hetzner.TagKubernetesClusterName, c.Cluster.Name),
+		fmt.Sprintf("%s=%s", hetzner.TagKubernetesClusterName, c.T.Cluster.Name),
 		fmt.Sprintf("%s=%s", hetzner.TagKubernetesInstanceGroup, fi.ValueOf(v.Name)),
 	}
 	listOptions := hcloud.ListOpts{

upup/pkg/fi/cloudup/openstacktasks/port_test.go

@@ -368,13 +368,13 @@ func Test_Port_Find(t *testing.T) {
 		{
 			desc: "nothing found",
 			context: &fi.CloudupContext{
-				Cluster: &kops.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "clusterName",
-					},
-				},
 				T: fi.CloudupSubContext{
 					Cloud: &portCloud{},
+					Cluster: &kops.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "clusterName",
+						},
+					},
 				},
 			},
 			port: &Port{
@@ -387,11 +387,6 @@ func Test_Port_Find(t *testing.T) {
 		{
 			desc: "port found no tags",
 			context: &fi.CloudupContext{
-				Cluster: &kops.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "clusterName",
-					},
-				},
 				T: fi.CloudupSubContext{
 					Cloud: &portCloud{
 						listPorts: []ports.Port{
@@ -411,6 +406,11 @@ func Test_Port_Find(t *testing.T) {
 							},
 						},
 					},
+					Cluster: &kops.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "clusterName",
+						},
+					},
 				},
 			},
 			port: &Port{
@@ -436,11 +436,6 @@ func Test_Port_Find(t *testing.T) {
 		{
 			desc: "port found with tags",
 			context: &fi.CloudupContext{
-				Cluster: &kops.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "clusterName",
-					},
-				},
 				T: fi.CloudupSubContext{
 					Cloud: &portCloud{
 						listPorts: []ports.Port{
@@ -460,6 +455,11 @@ func Test_Port_Find(t *testing.T) {
 							},
 						},
 					},
+					Cluster: &kops.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "clusterName",
+						},
+					},
 				},
 			},
 			port: &Port{
@@ -487,11 +487,6 @@ func Test_Port_Find(t *testing.T) {
 		{
 			desc: "multiple ports found",
 			context: &fi.CloudupContext{
-				Cluster: &kops.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "clusterName",
-					},
-				},
 				T: fi.CloudupSubContext{
 					Cloud: &portCloud{
 						listPorts: []ports.Port{
@@ -507,6 +502,11 @@ func Test_Port_Find(t *testing.T) {
 							},
 						},
 					},
+					Cluster: &kops.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "clusterName",
+						},
+					},
 				},
 			},
 			port: &Port{
@@ -519,11 +519,6 @@ func Test_Port_Find(t *testing.T) {
 		{
 			desc: "error listing ports",
 			context: &fi.CloudupContext{
-				Cluster: &kops.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "clusterName",
-					},
-				},
 				T: fi.CloudupSubContext{
 					Cloud: &portCloud{
 						listPorts: []ports.Port{
@@ -534,6 +529,11 @@ func Test_Port_Find(t *testing.T) {
 						},
 						listPortsError: fmt.Errorf("list error"),
 					},
+					Cluster: &kops.Cluster{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "clusterName",
+						},
+					},
 				},
 			},
 			port: &Port{

upup/pkg/fi/context.go

@@ -26,6 +26,7 @@ import (
 
 	"k8s.io/klog/v2"
 	"k8s.io/kops/pkg/apis/kops"
+	"k8s.io/kops/pkg/apis/nodeup"
 	"k8s.io/kops/util/pkg/vfs"
 )
 
@@ -33,7 +34,6 @@ type Context[T SubContext] struct {
 	ctx context.Context
 
 	Target      Target[T]
-	Cluster     *kops.Cluster
 	Keystore    Keystore
 	SecretStore SecretStore
 
@@ -54,12 +54,16 @@ type SubContext interface {
 }
 
 type CloudupSubContext struct {
-	Cloud Cloud
+	Cloud   Cloud
+	Cluster *kops.Cluster
 	// TODO: Few places use this. They could instead get it from the cluster spec.
 	ClusterConfigBase vfs.Path
 }
 type InstallSubContext struct{}
-type NodeupSubContext struct{}
+type NodeupSubContext struct {
+	BootConfig   *nodeup.BootConfig
+	NodeupConfig *nodeup.Config
+}
 
 func (c *Context[T]) Context() context.Context {
 	return c.ctx
@@ -71,10 +75,9 @@ type Warning[T SubContext] struct {
 	Message string
 }
 
-func newContext[T SubContext](ctx context.Context, target Target[T], cluster *kops.Cluster, keystore Keystore, secretStore SecretStore, checkExisting bool, sub T, tasks map[string]Task[T]) (*Context[T], error) {
+func newContext[T SubContext](ctx context.Context, target Target[T], keystore Keystore, secretStore SecretStore, checkExisting bool, sub T, tasks map[string]Task[T]) (*Context[T], error) {
 	c := &Context[T]{
 		ctx:           ctx,
-		Cluster:       cluster,
 		Target:        target,
 		Keystore:      keystore,
 		SecretStore:   secretStore,
@@ -88,19 +91,23 @@ func newContext[T SubContext](ctx context.Context, target Target[T], cluster *ko
 
 func NewInstallContext(ctx context.Context, target InstallTarget, tasks map[string]InstallTask) (*InstallContext, error) {
 	sub := InstallSubContext{}
-	return newContext[InstallSubContext](ctx, target, nil, nil, nil, true, sub, tasks)
+	return newContext[InstallSubContext](ctx, target, nil, nil, true, sub, tasks)
 }
-func NewNodeupContext(ctx context.Context, target NodeupTarget, cluster *kops.Cluster, keystore Keystore, secretStore SecretStore, checkExisting bool, tasks map[string]NodeupTask) (*NodeupContext, error) {
+func NewNodeupContext(ctx context.Context, target NodeupTarget, keystore Keystore, secretStore SecretStore, checkExisting bool, bootConfig *nodeup.BootConfig, nodeupConfig *nodeup.Config, tasks map[string]NodeupTask) (*NodeupContext, error) {
-	sub := NodeupSubContext{}
+	sub := NodeupSubContext{
-	return newContext[NodeupSubContext](ctx, target, cluster, keystore, secretStore, checkExisting, sub, tasks)
+		BootConfig:   bootConfig,
+		NodeupConfig: nodeupConfig,
+	}
+	return newContext[NodeupSubContext](ctx, target, keystore, secretStore, checkExisting, sub, tasks)
 }
 
 func NewCloudupContext(ctx context.Context, target CloudupTarget, cluster *kops.Cluster, cloud Cloud, keystore Keystore, secretStore SecretStore, clusterConfigBase vfs.Path, checkExisting bool, tasks map[string]CloudupTask) (*CloudupContext, error) {
 	sub := CloudupSubContext{
 		Cloud:             cloud,
+		Cluster:           cluster,
 		ClusterConfigBase: clusterConfigBase,
 	}
-	return newContext[CloudupSubContext](ctx, target, cluster, keystore, secretStore, checkExisting, sub, tasks)
+	return newContext[CloudupSubContext](ctx, target, keystore, secretStore, checkExisting, sub, tasks)
 }
 
 func (c *Context[T]) AllTasks() map[string]Task[T] {

upup/pkg/fi/fitasks/managedfile.go

@@ -143,7 +143,7 @@ func (e *ManagedFile) getACL(c *fi.CloudupContext, p vfs.Path) (vfs.ACL, error)
 		return acl, nil
 	}
 
-	return acls.GetACL(p, c.Cluster)
+	return acls.GetACL(p, c.T.Cluster)
 }
 
 func (_ *ManagedFile) Render(c *fi.CloudupContext, a, e, changes *ManagedFile) error {

upup/pkg/fi/nodeup/command.go

@@ -181,7 +181,7 @@ func (c *NodeUpCommand) Run(out io.Writer) error {
 		return fmt.Errorf("nodeup config hash mismatch (was %q, expected %q)", got, want)
 	}
 
-	cloudProvider := api.CloudProviderID(bootConfig.CloudProvider)
+	cloudProvider := bootConfig.CloudProvider
 	if cloudProvider == "" {
 		cloudProvider = c.cluster.Spec.GetCloudProvider()
 	}
@@ -369,10 +369,8 @@ func (c *NodeUpCommand) Run(out io.Writer) error {
 	switch c.Target {
 	case "direct":
 		target = &local.LocalTarget{
-			CacheDir:   c.CacheDir,
+			CacheDir: c.CacheDir,
-			Cloud:      cloud,
+			Cloud:    cloud,
-			InstanceID: modelContext.InstanceID,
-			Cluster:    c.cluster,
 		}
 	case "dryrun":
 		assetBuilder := assets.NewAssetBuilder(c.cluster, false)
@@ -381,7 +379,7 @@ func (c *NodeUpCommand) Run(out io.Writer) error {
 		return fmt.Errorf("unsupported target type %q", c.Target)
 	}
 
-	context, err := fi.NewNodeupContext(ctx, target, c.cluster, keyStore, secretStore, checkExisting, taskMap)
+	context, err := fi.NewNodeupContext(ctx, target, keyStore, secretStore, checkExisting, &bootConfig, &nodeupConfig, taskMap)
 	if err != nil {
 		klog.Exitf("error building context: %v", err)
 	}
@@ -684,7 +682,7 @@ func loadKernelModules(context *model.NodeupModelContext) error {
 
 // getRegion queries the cloud provider for the region.
 func getRegion(ctx context.Context, bootConfig *nodeup.BootConfig) (string, error) {
-	switch api.CloudProviderID(bootConfig.CloudProvider) {
+	switch bootConfig.CloudProvider {
 	case api.CloudProviderAWS:
 		region, err := awsup.RegionFromMetadata(ctx)
 		if err != nil {
@@ -699,7 +697,7 @@ func getRegion(ctx context.Context, bootConfig *nodeup.BootConfig) (string, erro
 
 // seedRNG adds entropy to the random number generator.
 func seedRNG(ctx context.Context, bootConfig *nodeup.BootConfig, region string) error {
-	switch api.CloudProviderID(bootConfig.CloudProvider) {
+	switch bootConfig.CloudProvider {
 	case api.CloudProviderAWS:
 		config := aws.NewConfig().WithCredentialsChainVerboseErrors(true).WithRegion(region)
 		sess, err := session.NewSession(config)
@@ -735,7 +733,7 @@ func getNodeConfigFromServer(ctx context.Context, bootConfig *nodeup.BootConfig,
 	var authenticator bootstrap.Authenticator
 	var resolver resolver.Resolver
 
-	switch api.CloudProviderID(bootConfig.CloudProvider) {
+	switch bootConfig.CloudProvider {
 	case api.CloudProviderAWS:
 		a, err := awsup.NewAWSAuthenticator(region)
 		if err != nil {

upup/pkg/fi/nodeup/local/local_target.go

@@ -19,15 +19,12 @@ package local
 import (
 	"os/exec"
 
-	"k8s.io/kops/pkg/apis/kops"
 	"k8s.io/kops/upup/pkg/fi"
 )
 
 type LocalTarget struct {
-	CacheDir   string
+	CacheDir string
-	Cloud      fi.Cloud
+	Cloud    fi.Cloud
-	InstanceID string
-	Cluster    *kops.Cluster
 }
 
 var _ fi.NodeupTarget = &LocalTarget{}

upup/pkg/fi/nodeup/nodetasks/package.go

@@ -197,10 +197,7 @@ func (e *Package) findDpkg(c *fi.NodeupContext) (*Package, error) {
 		}
 	}
 
-	// TODO: Take InstanceGroup-level overriding of the Cluster-level update policy into account
+	if c.T.NodeupConfig.UpdatePolicy != kops.UpdatePolicyExternal || !installed {
-	// here. Doing so requires that we make the current InstanceGroup available within Package's
-	// methods.
-	if fi.ValueOf(c.Cluster.Spec.UpdatePolicy) != kops.UpdatePolicyExternal || !installed {
 		return nil, nil
 	}
 
@@ -248,10 +245,7 @@ func (e *Package) findYum(c *fi.NodeupContext) (*Package, error) {
 		healthy = fi.PtrTo(true)
 	}
 
-	// TODO: Take InstanceGroup-level overriding of the Cluster-level update policy into account
+	if c.T.NodeupConfig.UpdatePolicy != kops.UpdatePolicyExternal || !installed {
-	// here. Doing so requires that we make the current InstanceGroup available within Package's
-	// methods.
-	if fi.ValueOf(c.Cluster.Spec.UpdatePolicy) != kops.UpdatePolicyExternal || !installed {
 		return nil, nil
 	}
 

upup/pkg/fi/nodeup/nodetasks/prefix.go

@@ -50,8 +50,8 @@ func (p *Prefix) String() string {
 }
 
 func (e *Prefix) Find(c *fi.NodeupContext) (*Prefix, error) {
-	if c.Cluster.Spec.GetCloudProvider() != kops.CloudProviderAWS {
+	if c.T.BootConfig.CloudProvider != kops.CloudProviderAWS {
-		return nil, fmt.Errorf("unsupported cloud provider: %s", c.Cluster.Spec.GetCloudProvider())
+		return nil, fmt.Errorf("unsupported cloud provider: %s", c.T.BootConfig.CloudProvider)
 	}
 
 	mac, err := getInstanceMetadataFirstValue("mac")