From 8c1bc03f42ebdb4ef7604c129e265ac69a3f9d00 Mon Sep 17 00:00:00 2001 From: Ole Markus With Date: Mon, 15 Aug 2022 20:50:45 +0200 Subject: [PATCH] Don't write the populated IG spec to state store --- cmd/kops/create_cluster.go | 55 ++---- upup/pkg/fi/cloudup/apply_cluster.go | 1 + upup/pkg/fi/cloudup/new_cluster.go | 183 +++++++++++++++++- .../fi/cloudup/populate_instancegroup_spec.go | 171 +--------------- 4 files changed, 198 insertions(+), 212 deletions(-) diff --git a/cmd/kops/create_cluster.go b/cmd/kops/create_cluster.go index e2aced3a36..b1b35b87bb 100644 --- a/cmd/kops/create_cluster.go +++ b/cmd/kops/create_cluster.go @@ -40,6 +40,7 @@ import ( kopsbase "k8s.io/kops" "k8s.io/kops/cmd/kops/util" + "k8s.io/kops/pkg/apis/kops" api "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/apis/kops/registry" kopsutil "k8s.io/kops/pkg/apis/kops/util" @@ -68,9 +69,6 @@ type CreateClusterOptions struct { NodeVolumeSize int32 ContainerRuntime string OutDir string - Image string - NodeImage string - MasterImage string DisableSubnetTags bool NetworkCIDR string DNSZone string @@ -549,22 +547,6 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr } } - if c.Image != "" { - for _, group := range instanceGroups { - group.Spec.Image = c.Image - } - } - if c.MasterImage != "" { - for _, group := range masters { - group.Spec.Image = c.MasterImage - } - } - if c.NodeImage != "" { - for _, group := range nodes { - group.Spec.Image = c.NodeImage - } - } - if c.AssociatePublicIP != nil { for _, group := range instanceGroups { group.Spec.AssociatePublicIP = c.AssociatePublicIP @@ -662,19 +644,6 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr return err } - var fullInstanceGroups []*api.InstanceGroup - for _, group := range instanceGroups { - fullGroup, err := cloudup.PopulateInstanceGroupSpec(fullCluster, group, cloud, clusterResult.Channel) - if err != nil { - return err - } - fullGroup.AddInstanceGroupNodeLabel() - if cluster.Spec.GetCloudProvider() == api.CloudProviderGCE { - fullGroup.Spec.NodeLabels["cloud.google.com/metadata-proxy-ready"] = "true" - } - fullInstanceGroups = append(fullInstanceGroups, fullGroup) - } - kubernetesVersion, err := kopsutil.ParseKubernetesVersion(clusterResult.Cluster.Spec.KubernetesVersion) if err != nil { return fmt.Errorf("cannot parse KubernetesVersion %q in cluster: %w", clusterResult.Cluster.Spec.KubernetesVersion, err) @@ -693,16 +662,28 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr addons = append(addons, addon.Objects...) } - err = validation.DeepValidate(fullCluster, fullInstanceGroups, true, nil) - if err != nil { - return err + { + // Build full IG spec to ensure we end up with a valid IG + fullInstanceGroups := []*kops.InstanceGroup{} + for _, group := range instanceGroups { + fullGroup, err := cloudup.PopulateInstanceGroupSpec(cluster, group, cloud, clusterResult.Channel) + if err != nil { + return err + } + fullInstanceGroups = append(fullInstanceGroups, fullGroup) + } + + err = validation.DeepValidate(fullCluster, fullInstanceGroups, true, nil) + if err != nil { + return fmt.Errorf("validation of the full cluster and instance group specs failed: %w", err) + } } if c.DryRun { var obj []runtime.Object obj = append(obj, cluster) - for _, group := range fullInstanceGroups { + for _, group := range instanceGroups { // Cluster name is not populated, and we need it group.ObjectMeta.Labels = make(map[string]string) group.ObjectMeta.Labels[api.LabelClusterName] = cluster.ObjectMeta.Name @@ -730,7 +711,7 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr } // Note we perform as much validation as we can, before writing a bad config - err = registry.CreateClusterConfig(ctx, clientset, cluster, fullInstanceGroups, addons) + err = registry.CreateClusterConfig(ctx, clientset, cluster, instanceGroups, addons) if err != nil { return fmt.Errorf("error writing updated configuration: %v", err) } diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index be823258bb..3f8ba73d5d 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -819,6 +819,7 @@ func (c *ApplyClusterCmd) upgradeSpecs(assetBuilder *assets.AssetBuilder) error } c.Cluster = fullCluster + klog.Infof("Populating instance group from upgradeSpecs") for i, g := range c.InstanceGroups { fullGroup, err := PopulateInstanceGroupSpec(fullCluster, g, c.Cloud, c.channel) if err != nil { diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index 38d3bb2930..3270103730 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/upup/pkg/fi/cloudup/new_cluster.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/blang/semver/v4" "k8s.io/apimachinery/pkg/api/resource" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" @@ -30,7 +31,9 @@ import ( "k8s.io/kops" api "k8s.io/kops/pkg/apis/kops" + kopsapi "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/apis/kops/model" + "k8s.io/kops/pkg/apis/kops/util" "k8s.io/kops/pkg/client/simple" "k8s.io/kops/pkg/dns" "k8s.io/kops/pkg/featureflag" @@ -40,6 +43,7 @@ import ( "k8s.io/kops/upup/pkg/fi/cloudup/azure" "k8s.io/kops/upup/pkg/fi/cloudup/gce" "k8s.io/kops/upup/pkg/fi/cloudup/openstack" + "k8s.io/kops/util/pkg/architectures" "k8s.io/kops/util/pkg/vfs" ) @@ -150,6 +154,10 @@ type NewClusterOptions struct { // InstanceManager specifies which manager to use for managing instances. InstanceManager string + + Image string + NodeImage string + MasterImage string } func (o *NewClusterOptions) InitDefaults() { @@ -260,9 +268,18 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster return nil, fmt.Errorf("unable to infer cloud provider from zones. pass in the cloud provider explicitly using --cloud") } } + + var cloud fi.Cloud + switch api.CloudProviderID(opt.CloudProvider) { case api.CloudProviderAWS: cluster.Spec.CloudProvider.AWS = &api.AWSSpec{} + cloudTags := map[string]string{} + awsCloud, err := awsup.NewAWSCloud(opt.Zones[0][:len(opt.Zones[0])-1], cloudTags) + if err != nil { + return nil, err + } + cloud = awsCloud case api.CloudProviderAzure: cluster.Spec.CloudProvider.Azure = &api.AzureSpec{ SubscriptionID: opt.AzureSubscriptionID, @@ -310,7 +327,7 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster } } - err = setupVPC(opt, &cluster) + err = setupVPC(opt, &cluster, cloud) if err != nil { return nil, err } @@ -373,6 +390,96 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster instanceGroups = append(instanceGroups, nodes...) instanceGroups = append(instanceGroups, bastions...) + for _, instanceGroup := range instanceGroups { + g := instanceGroup + ig := g + if instanceGroup.Spec.Image == "" { + if opt.Image != "" { + instanceGroup.Spec.Image = opt.Image + } else { + architecture, err := MachineArchitecture(cloud, instanceGroup.Spec.MachineType) + if err != nil { + return nil, err + } + instanceGroup.Spec.Image = defaultImage(&cluster, channel, architecture) + } + } + + // TODO: Clean up + if g.IsMaster() { + if g.Spec.MachineType == "" { + g.Spec.MachineType, err = defaultMachineType(cloud, &cluster, ig) + if err != nil { + return nil, fmt.Errorf("error assigning default machine type for masters: %v", err) + } + + } + } else if g.Spec.Role == kopsapi.InstanceGroupRoleBastion { + if g.Spec.MachineType == "" { + g.Spec.MachineType, err = defaultMachineType(cloud, &cluster, g) + if err != nil { + return nil, fmt.Errorf("error assigning default machine type for bastions: %v", err) + } + } + } else { + if g.IsAPIServerOnly() && !featureflag.APIServerNodes.Enabled() { + return nil, fmt.Errorf("apiserver nodes requires the APIServerNodes feature flag to be enabled") + } + if g.Spec.MachineType == "" { + g.Spec.MachineType, err = defaultMachineType(cloud, &cluster, g) + if err != nil { + return nil, fmt.Errorf("error assigning default machine type for nodes: %v", err) + } + } + + } + + if ig.Spec.Tenancy != "" && ig.Spec.Tenancy != "default" { + switch cluster.Spec.GetCloudProvider() { + case kopsapi.CloudProviderAWS: + if _, ok := awsDedicatedInstanceExceptions[g.Spec.MachineType]; ok { + return nil, fmt.Errorf("invalid dedicated instance type: %s", g.Spec.MachineType) + } + default: + klog.Warning("Trying to set tenancy on non-AWS environment") + } + } + + if ig.IsMaster() { + if len(ig.Spec.Subnets) == 0 { + return nil, fmt.Errorf("master InstanceGroup %s did not specify any Subnets", g.ObjectMeta.Name) + } + } else if ig.IsAPIServerOnly() && cluster.Spec.IsIPv6Only() { + if len(ig.Spec.Subnets) == 0 { + for _, subnet := range cluster.Spec.Subnets { + if subnet.Type != kopsapi.SubnetTypePrivate && subnet.Type != kopsapi.SubnetTypeUtility { + ig.Spec.Subnets = append(g.Spec.Subnets, subnet.Name) + } + } + } + } else { + if len(ig.Spec.Subnets) == 0 { + for _, subnet := range cluster.Spec.Subnets { + if subnet.Type != kopsapi.SubnetTypeDualStack && subnet.Type != kopsapi.SubnetTypeUtility { + g.Spec.Subnets = append(g.Spec.Subnets, subnet.Name) + } + } + } + + if len(g.Spec.Subnets) == 0 { + for _, subnet := range cluster.Spec.Subnets { + if subnet.Type != kopsapi.SubnetTypeUtility { + g.Spec.Subnets = append(g.Spec.Subnets, subnet.Name) + } + } + } + } + + if len(g.Spec.Subnets) == 0 { + return nil, fmt.Errorf("unable to infer any Subnets for InstanceGroup %s ", g.ObjectMeta.Name) + } + } + result := NewClusterResult{ Cluster: &cluster, InstanceGroups: instanceGroups, @@ -381,17 +488,13 @@ func NewCluster(opt *NewClusterOptions, clientset simple.Clientset) (*NewCluster return &result, nil } -func setupVPC(opt *NewClusterOptions, cluster *api.Cluster) error { +func setupVPC(opt *NewClusterOptions, cluster *api.Cluster, cloud fi.Cloud) error { cluster.Spec.NetworkID = opt.NetworkID switch cluster.Spec.GetCloudProvider() { case api.CloudProviderAWS: if cluster.Spec.NetworkID == "" && len(opt.SubnetIDs) > 0 { - cloudTags := map[string]string{} - awsCloud, err := awsup.NewAWSCloud(opt.Zones[0][:len(opt.Zones[0])-1], cloudTags) - if err != nil { - return fmt.Errorf("error loading cloud: %v", err) - } + awsCloud := cloud.(awsup.AWSCloud) res, err := awsCloud.EC2().DescribeSubnets(&ec2.DescribeSubnetsInput{ SubnetIds: []*string{aws.String(opt.SubnetIDs[0])}, }) @@ -755,6 +858,8 @@ func setupMasters(opt *NewClusterOptions, cluster *api.Cluster, zoneToSubnetMap } } + g.Spec.Image = opt.MasterImage + masters = append(masters, g) } } @@ -880,6 +985,8 @@ func setupNodes(opt *NewClusterOptions, cluster *api.Cluster, zoneToSubnetMap ma } } + g.Spec.Image = opt.NodeImage + nodes = append(nodes, g) } @@ -1114,6 +1221,8 @@ func setupTopology(opt *NewClusterOptions, cluster *api.Cluster, allZones sets.S bastionGroup := &api.InstanceGroup{} bastionGroup.Spec.Role = api.InstanceGroupRoleBastion bastionGroup.ObjectMeta.Name = "bastions" + bastionGroup.Spec.MaxSize = fi.Int32(1) + bastionGroup.Spec.MinSize = fi.Int32(1) bastions = append(bastions, bastionGroup) if !dns.IsGossipHostname(cluster.Name) { @@ -1321,3 +1430,63 @@ func addCiliumNetwork(cluster *api.Cluster) { enabled := false cluster.Spec.KubeProxy.Enabled = &enabled } + +// defaultImage returns the default Image, based on the cloudprovider +func defaultImage(cluster *kopsapi.Cluster, channel *kopsapi.Channel, architecture architectures.Architecture) string { + if channel != nil { + var kubernetesVersion *semver.Version + if cluster.Spec.KubernetesVersion != "" { + var err error + kubernetesVersion, err = util.ParseKubernetesVersion(cluster.Spec.KubernetesVersion) + if err != nil { + klog.Warningf("cannot parse KubernetesVersion %q in cluster", cluster.Spec.KubernetesVersion) + } + } + if kubernetesVersion != nil { + image := channel.FindImage(cluster.Spec.GetCloudProvider(), *kubernetesVersion, architecture) + if image != nil { + return image.Name + } + } + } + + switch cluster.Spec.GetCloudProvider() { + case kopsapi.CloudProviderDO: + return defaultDONodeImage + } + klog.Infof("Cannot set default Image for CloudProvider=%q", cluster.Spec.GetCloudProvider()) + return "" +} + +func MachineArchitecture(cloud fi.Cloud, machineType string) (architectures.Architecture, error) { + if machineType == "" { + return architectures.ArchitectureAmd64, nil + } + + switch cloud.ProviderID() { + case kopsapi.CloudProviderAWS: + info, err := cloud.(awsup.AWSCloud).DescribeInstanceType(machineType) + if err != nil { + return "", fmt.Errorf("error finding instance info for instance type %q: %v", machineType, err) + } + if info.ProcessorInfo == nil || len(info.ProcessorInfo.SupportedArchitectures) == 0 { + return "", fmt.Errorf("error finding architecture info for instance type %q", machineType) + } + var unsupported []string + for _, arch := range info.ProcessorInfo.SupportedArchitectures { + // Return the first found supported architecture, in order of popularity + switch fi.StringValue(arch) { + case ec2.ArchitectureTypeX8664: + return architectures.ArchitectureAmd64, nil + case ec2.ArchitectureTypeArm64: + return architectures.ArchitectureArm64, nil + default: + unsupported = append(unsupported, fi.StringValue(arch)) + } + } + return "", fmt.Errorf("unsupported architecture for instance type %q: %v", machineType, unsupported) + default: + // No other clouds are known to support any other architectures at this time + return architectures.ArchitectureAmd64, nil + } +} diff --git a/upup/pkg/fi/cloudup/populate_instancegroup_spec.go b/upup/pkg/fi/cloudup/populate_instancegroup_spec.go index 95f301327d..3c480511c1 100644 --- a/upup/pkg/fi/cloudup/populate_instancegroup_spec.go +++ b/upup/pkg/fi/cloudup/populate_instancegroup_spec.go @@ -20,18 +20,13 @@ import ( "fmt" "strings" - "github.com/aws/aws-sdk-go/service/ec2" - "github.com/blang/semver/v4" "k8s.io/klog/v2" "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/apis/kops/util" "k8s.io/kops/pkg/apis/kops/validation" - "k8s.io/kops/pkg/featureflag" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awsup" "k8s.io/kops/upup/pkg/fi/cloudup/openstack" - "k8s.io/kops/util/pkg/architectures" "k8s.io/kops/util/pkg/reflectutils" ) @@ -67,117 +62,17 @@ var awsDedicatedInstanceExceptions = map[string]bool{ // PopulateInstanceGroupSpec sets default values in the InstanceGroup // The InstanceGroup is simpler than the cluster spec, so we just populate in place (like the rest of k8s) func PopulateInstanceGroupSpec(cluster *kops.Cluster, input *kops.InstanceGroup, cloud fi.Cloud, channel *kops.Channel) (*kops.InstanceGroup, error) { + klog.Infof("Populating instance group spec for %q", input.GetName()) + var err error err = validation.ValidateInstanceGroup(input, nil, false).ToAggregate() if err != nil { - return nil, err + return nil, fmt.Errorf("failed validating input specs: %w", err) } ig := &kops.InstanceGroup{} reflectutils.JSONMergeStruct(ig, input) - // TODO: Clean up - if ig.IsMaster() { - if ig.Spec.MachineType == "" { - ig.Spec.MachineType, err = defaultMachineType(cloud, cluster, ig) - if err != nil { - return nil, fmt.Errorf("error assigning default machine type for masters: %v", err) - } - - } - if ig.Spec.MinSize == nil { - ig.Spec.MinSize = fi.Int32(1) - } - if ig.Spec.MaxSize == nil { - ig.Spec.MaxSize = fi.Int32(1) - } - } else if ig.Spec.Role == kops.InstanceGroupRoleBastion { - if ig.Spec.MachineType == "" { - ig.Spec.MachineType, err = defaultMachineType(cloud, cluster, ig) - if err != nil { - return nil, fmt.Errorf("error assigning default machine type for bastions: %v", err) - } - } - if ig.Spec.MinSize == nil { - ig.Spec.MinSize = fi.Int32(1) - } - if ig.Spec.MaxSize == nil { - ig.Spec.MaxSize = fi.Int32(1) - } - } else { - if ig.IsAPIServerOnly() && !featureflag.APIServerNodes.Enabled() { - return nil, fmt.Errorf("apiserver nodes requires the APIServerNodes feature flag to be enabled") - } - if ig.Spec.MachineType == "" { - ig.Spec.MachineType, err = defaultMachineType(cloud, cluster, ig) - if err != nil { - return nil, fmt.Errorf("error assigning default machine type for nodes: %v", err) - } - } - if ig.Spec.MinSize == nil { - ig.Spec.MinSize = fi.Int32(2) - } - if ig.Spec.MaxSize == nil { - ig.Spec.MaxSize = fi.Int32(2) - } - } - - if ig.Spec.Image == "" { - architecture, err := MachineArchitecture(cloud, ig.Spec.MachineType) - if err != nil { - return nil, fmt.Errorf("unable to determine machine architecture for InstanceGroup %q: %v", ig.ObjectMeta.Name, err) - } - ig.Spec.Image = defaultImage(cluster, channel, architecture) - if ig.Spec.Image == "" { - return nil, fmt.Errorf("unable to determine default image for InstanceGroup %s", ig.ObjectMeta.Name) - } - } - - if ig.Spec.Tenancy != "" && ig.Spec.Tenancy != "default" { - switch cluster.Spec.GetCloudProvider() { - case kops.CloudProviderAWS: - if _, ok := awsDedicatedInstanceExceptions[ig.Spec.MachineType]; ok { - return nil, fmt.Errorf("invalid dedicated instance type: %s", ig.Spec.MachineType) - } - default: - klog.Warning("Trying to set tenancy on non-AWS environment") - } - } - - if ig.IsMaster() { - if len(ig.Spec.Subnets) == 0 { - return nil, fmt.Errorf("master InstanceGroup %s did not specify any Subnets", ig.ObjectMeta.Name) - } - } else if ig.IsAPIServerOnly() && cluster.Spec.IsIPv6Only() { - if len(ig.Spec.Subnets) == 0 { - for _, subnet := range cluster.Spec.Subnets { - if subnet.Type != kops.SubnetTypePrivate && subnet.Type != kops.SubnetTypeUtility { - ig.Spec.Subnets = append(ig.Spec.Subnets, subnet.Name) - } - } - } - } else { - if len(ig.Spec.Subnets) == 0 { - for _, subnet := range cluster.Spec.Subnets { - if subnet.Type != kops.SubnetTypeDualStack && subnet.Type != kops.SubnetTypeUtility { - ig.Spec.Subnets = append(ig.Spec.Subnets, subnet.Name) - } - } - } - - if len(ig.Spec.Subnets) == 0 { - for _, subnet := range cluster.Spec.Subnets { - if subnet.Type != kops.SubnetTypeUtility { - ig.Spec.Subnets = append(ig.Spec.Subnets, subnet.Name) - } - } - } - } - - if len(ig.Spec.Subnets) == 0 { - return nil, fmt.Errorf("unable to infer any Subnets for InstanceGroup %s ", ig.ObjectMeta.Name) - } - hasGPU := false clusterNvidia := false if cluster.Spec.Containerd != nil && cluster.Spec.Containerd.NvidiaGPU != nil && fi.BoolValue(cluster.Spec.Containerd.NvidiaGPU.Enabled) { @@ -296,63 +191,3 @@ func defaultMachineType(cloud fi.Cloud, cluster *kops.Cluster, ig *kops.Instance klog.V(2).Infof("Cannot set default MachineType for CloudProvider=%q, Role=%q", cluster.Spec.GetCloudProvider(), ig.Spec.Role) return "", nil } - -// defaultImage returns the default Image, based on the cloudprovider -func defaultImage(cluster *kops.Cluster, channel *kops.Channel, architecture architectures.Architecture) string { - if channel != nil { - var kubernetesVersion *semver.Version - if cluster.Spec.KubernetesVersion != "" { - var err error - kubernetesVersion, err = util.ParseKubernetesVersion(cluster.Spec.KubernetesVersion) - if err != nil { - klog.Warningf("cannot parse KubernetesVersion %q in cluster", cluster.Spec.KubernetesVersion) - } - } - if kubernetesVersion != nil { - image := channel.FindImage(cluster.Spec.GetCloudProvider(), *kubernetesVersion, architecture) - if image != nil { - return image.Name - } - } - } - - switch cluster.Spec.GetCloudProvider() { - case kops.CloudProviderDO: - return defaultDONodeImage - } - klog.Infof("Cannot set default Image for CloudProvider=%q", cluster.Spec.GetCloudProvider()) - return "" -} - -func MachineArchitecture(cloud fi.Cloud, machineType string) (architectures.Architecture, error) { - if machineType == "" { - return architectures.ArchitectureAmd64, nil - } - - switch cloud.ProviderID() { - case kops.CloudProviderAWS: - info, err := cloud.(awsup.AWSCloud).DescribeInstanceType(machineType) - if err != nil { - return "", fmt.Errorf("error finding instance info for instance type %q: %v", machineType, err) - } - if info.ProcessorInfo == nil || len(info.ProcessorInfo.SupportedArchitectures) == 0 { - return "", fmt.Errorf("error finding architecture info for instance type %q", machineType) - } - var unsupported []string - for _, arch := range info.ProcessorInfo.SupportedArchitectures { - // Return the first found supported architecture, in order of popularity - switch fi.StringValue(arch) { - case ec2.ArchitectureTypeX8664: - return architectures.ArchitectureAmd64, nil - case ec2.ArchitectureTypeArm64: - return architectures.ArchitectureArm64, nil - default: - unsupported = append(unsupported, fi.StringValue(arch)) - } - } - return "", fmt.Errorf("unsupported architecture for instance type %q: %v", machineType, unsupported) - default: - // No other clouds are known to support any other architectures at this time - return architectures.ArchitectureAmd64, nil - } -}