From 2420991954dd5cda81c89de30d1809bb76b36ea3 Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Fri, 13 Jan 2023 11:02:18 -0800 Subject: [PATCH 1/2] Determine default API access method by IG subnet type --- cmd/kops/create_cluster.go | 2 +- cmd/kops/edit_cluster.go | 2 +- cmd/kops/edit_instancegroup.go | 2 +- nodeup/pkg/model/kubelet_test.go | 6 +-- pkg/apis/kops/v1alpha2/defaults.go | 22 -------- pkg/apis/kops/v1alpha3/defaults.go | 25 --------- pkg/apis/kops/validation/aws.go | 6 ++- pkg/apis/kops/validation/aws_test.go | 4 +- pkg/apis/kops/validation/validation.go | 2 +- pkg/commands/helpers_readwrite.go | 4 +- pkg/instancegroups/rollingupdate_os_test.go | 2 +- upup/pkg/fi/cloudup/apply_cluster.go | 2 +- upup/pkg/fi/cloudup/populate_cluster_spec.go | 53 +++++++++++++++++-- .../fi/cloudup/populate_cluster_spec_test.go | 2 +- 14 files changed, 67 insertions(+), 67 deletions(-) diff --git a/cmd/kops/create_cluster.go b/cmd/kops/create_cluster.go index 48f92468f2bb2..b1db8dd13277a 100644 --- a/cmd/kops/create_cluster.go +++ b/cmd/kops/create_cluster.go @@ -657,7 +657,7 @@ func RunCreateCluster(ctx context.Context, f *util.Factory, out io.Writer, c *Cr } assetBuilder := assets.NewAssetBuilder(clientset.VFSContext(), cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) - fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, cloud, assetBuilder) + fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, instanceGroups, cloud, assetBuilder) if err != nil { return err } diff --git a/cmd/kops/edit_cluster.go b/cmd/kops/edit_cluster.go index 28e8332f3fe1b..09f1518ea087b 100644 --- a/cmd/kops/edit_cluster.go +++ b/cmd/kops/edit_cluster.go @@ -261,7 +261,7 @@ func updateCluster(ctx context.Context, clientset simple.Clientset, oldCluster, } assetBuilder := assets.NewAssetBuilder(clientset.VFSContext(), newCluster.Spec.Assets, newCluster.Spec.KubernetesVersion, false) - fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, newCluster, cloud, assetBuilder) + fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, newCluster, instanceGroups, cloud, assetBuilder) if err != nil { return fmt.Sprintf("error populating cluster spec: %s", err), nil } diff --git a/cmd/kops/edit_instancegroup.go b/cmd/kops/edit_instancegroup.go index 521930585f0ac..96661825f84bf 100644 --- a/cmd/kops/edit_instancegroup.go +++ b/cmd/kops/edit_instancegroup.go @@ -296,7 +296,7 @@ func updateInstanceGroup(ctx context.Context, clientset simple.Clientset, channe } assetBuilder := assets.NewAssetBuilder(clientset.VFSContext(), cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) - fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, cloud, assetBuilder) + fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, []*api.InstanceGroup{newGroup}, cloud, assetBuilder) if err != nil { return fmt.Sprintf("error populating cluster spec: %s", err), nil } diff --git a/nodeup/pkg/model/kubelet_test.go b/nodeup/pkg/model/kubelet_test.go index 2c053a96b535e..e60900ef69e36 100644 --- a/nodeup/pkg/model/kubelet_test.go +++ b/nodeup/pkg/model/kubelet_test.go @@ -254,7 +254,7 @@ func BuildNodeupModelContext(model *testutils.Model) (*NodeupModelContext, error return nil, fmt.Errorf("error from PerformAssignments: %v", err) } - nodeupModelContext.Cluster, err = mockedPopulateClusterSpec(ctx, model.Cluster, cloud) + nodeupModelContext.Cluster, err = mockedPopulateClusterSpec(ctx, model.Cluster, model.InstanceGroups, cloud) if err != nil { return nil, fmt.Errorf("unexpected error from mockedPopulateClusterSpec: %v", err) } @@ -289,7 +289,7 @@ func BuildNodeupModelContext(model *testutils.Model) (*NodeupModelContext, error return nodeupModelContext, nil } -func mockedPopulateClusterSpec(ctx context.Context, c *kops.Cluster, cloud fi.Cloud) (*kops.Cluster, error) { +func mockedPopulateClusterSpec(ctx context.Context, c *kops.Cluster, instanceGroups []*kops.InstanceGroup, cloud fi.Cloud) (*kops.Cluster, error) { vfs.Context.ResetMemfsContext(true) assetBuilder := assets.NewAssetBuilder(vfs.Context, c.Spec.Assets, c.Spec.KubernetesVersion, false) @@ -298,7 +298,7 @@ func mockedPopulateClusterSpec(ctx context.Context, c *kops.Cluster, cloud fi.Cl return nil, fmt.Errorf("error building vfspath: %v", err) } clientset := vfsclientset.NewVFSClientset(vfs.Context, basePath) - return cloudup.PopulateClusterSpec(ctx, clientset, c, cloud, assetBuilder) + return cloudup.PopulateClusterSpec(ctx, clientset, c, instanceGroups, cloud, assetBuilder) } // Fixed cert and key, borrowed from the create_kubecfg_test.go test diff --git a/pkg/apis/kops/v1alpha2/defaults.go b/pkg/apis/kops/v1alpha2/defaults.go index b379b9941956b..f8c312084229a 100644 --- a/pkg/apis/kops/v1alpha2/defaults.go +++ b/pkg/apis/kops/v1alpha2/defaults.go @@ -18,7 +18,6 @@ package v1alpha2 import ( "k8s.io/apimachinery/pkg/runtime" - "k8s.io/klog/v2" ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -54,27 +53,6 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) { if obj.LegacyAPI == nil { obj.LegacyAPI = &APISpec{} } - - if obj.LegacyAPI.IsEmpty() { - switch obj.Topology.ControlPlane { - case TopologyPublic: - obj.LegacyAPI.DNS = &DNSAccessSpec{} - - case TopologyPrivate: - obj.LegacyAPI.LoadBalancer = &LoadBalancerAccessSpec{} - - default: - klog.Infof("unknown master topology type: %q", obj.Topology.ControlPlane) - } - } - - if obj.LegacyAPI.LoadBalancer != nil && obj.LegacyAPI.LoadBalancer.Type == "" { - obj.LegacyAPI.LoadBalancer.Type = LoadBalancerTypePublic - } - - if obj.LegacyAPI.LoadBalancer != nil && obj.LegacyAPI.LoadBalancer.Class == "" && obj.LegacyCloudProvider == "aws" { - obj.LegacyAPI.LoadBalancer.Class = LoadBalancerClassClassic - } } if obj.Authorization == nil { diff --git a/pkg/apis/kops/v1alpha3/defaults.go b/pkg/apis/kops/v1alpha3/defaults.go index 0e1a5d292c8fd..36fc0c8b54add 100644 --- a/pkg/apis/kops/v1alpha3/defaults.go +++ b/pkg/apis/kops/v1alpha3/defaults.go @@ -18,7 +18,6 @@ package v1alpha3 import ( "k8s.io/apimachinery/pkg/runtime" - "k8s.io/klog/v2" ) func addDefaultingFuncs(scheme *runtime.Scheme) error { @@ -46,30 +45,6 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) { obj.Networking.Topology.DNS = DNSTypePublic } - if obj.CloudProvider.Openstack == nil { - if obj.API.DNS == nil && obj.API.LoadBalancer == nil { - switch obj.Networking.Topology.ControlPlane { - case TopologyPublic: - obj.API.DNS = &DNSAccessSpec{} - - case TopologyPrivate: - obj.API.LoadBalancer = &LoadBalancerAccessSpec{} - - default: - klog.Infof("unknown controlPlane topology type: %q", obj.Networking.Topology.ControlPlane) - } - } - - if obj.API.LoadBalancer != nil && obj.API.LoadBalancer.Type == "" { - obj.API.LoadBalancer.Type = LoadBalancerTypePublic - } - - } - - if obj.API.LoadBalancer != nil && obj.API.LoadBalancer.Class == "" && obj.CloudProvider.AWS != nil { - obj.API.LoadBalancer.Class = LoadBalancerClassClassic - } - if obj.Authorization == nil { obj.Authorization = &AuthorizationSpec{} } diff --git a/pkg/apis/kops/validation/aws.go b/pkg/apis/kops/validation/aws.go index 738252c38e5fd..7a43f960fe7b5 100644 --- a/pkg/apis/kops/validation/aws.go +++ b/pkg/apis/kops/validation/aws.go @@ -32,13 +32,15 @@ import ( "k8s.io/kops/upup/pkg/fi/cloudup/awsup" ) -func awsValidateCluster(c *kops.Cluster) field.ErrorList { +func awsValidateCluster(c *kops.Cluster, strict bool) field.ErrorList { allErrs := field.ErrorList{} if c.Spec.API.LoadBalancer != nil { lbPath := field.NewPath("spec", "api", "loadBalancer") lbSpec := c.Spec.API.LoadBalancer - allErrs = append(allErrs, IsValidValue(lbPath.Child("class"), &lbSpec.Class, kops.SupportedLoadBalancerClasses)...) + if strict || lbSpec.Class != "" { + allErrs = append(allErrs, IsValidValue(lbPath.Child("class"), &lbSpec.Class, kops.SupportedLoadBalancerClasses)...) + } allErrs = append(allErrs, awsValidateTopologyDNS(lbPath.Child("type"), c)...) allErrs = append(allErrs, awsValidateSecurityGroupOverride(lbPath.Child("securityGroupOverride"), lbSpec)...) allErrs = append(allErrs, awsValidateAdditionalSecurityGroups(lbPath.Child("additionalSecurityGroups"), lbSpec.AdditionalSecurityGroups)...) diff --git a/pkg/apis/kops/validation/aws_test.go b/pkg/apis/kops/validation/aws_test.go index d2e993476fca6..4f5d748b3d7c0 100644 --- a/pkg/apis/kops/validation/aws_test.go +++ b/pkg/apis/kops/validation/aws_test.go @@ -600,7 +600,7 @@ func TestLoadBalancerSubnets(t *testing.T) { }) } cluster.Spec.API.LoadBalancer.Subnets = test.lbSubnets - errs := awsValidateCluster(&cluster) + errs := awsValidateCluster(&cluster, true) testErrors(t, test, errs, test.expected) } } @@ -670,7 +670,7 @@ func TestAWSAuthentication(t *testing.T) { }, }, } - errs := awsValidateCluster(&cluster) + errs := awsValidateCluster(&cluster, true) testErrors(t, test, errs, test.expected) } } diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 44586318f0833..b63b7aae29321 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -68,7 +68,7 @@ func newValidateCluster(cluster *kops.Cluster, strict bool) field.ErrorList { // Additional cloud-specific validation rules switch cluster.Spec.GetCloudProvider() { case kops.CloudProviderAWS: - allErrs = append(allErrs, awsValidateCluster(cluster)...) + allErrs = append(allErrs, awsValidateCluster(cluster, strict)...) case kops.CloudProviderGCE: allErrs = append(allErrs, gceValidateCluster(cluster)...) } diff --git a/pkg/commands/helpers_readwrite.go b/pkg/commands/helpers_readwrite.go index 7411f0ed31a07..d7298cbdae722 100644 --- a/pkg/commands/helpers_readwrite.go +++ b/pkg/commands/helpers_readwrite.go @@ -41,7 +41,7 @@ func UpdateCluster(ctx context.Context, clientset simple.Clientset, cluster *kop } assetBuilder := assets.NewAssetBuilder(clientset.VFSContext(), cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) - fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, cloud, assetBuilder) + fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, instanceGroups, cloud, assetBuilder) if err != nil { return err } @@ -79,7 +79,7 @@ func UpdateInstanceGroup(ctx context.Context, clientset simple.Clientset, cluste } assetBuilder := assets.NewAssetBuilder(clientset.VFSContext(), cluster.Spec.Assets, cluster.Spec.KubernetesVersion, false) - fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, cloud, assetBuilder) + fullCluster, err := cloudup.PopulateClusterSpec(ctx, clientset, cluster, allInstanceGroups, cloud, assetBuilder) if err != nil { return err } diff --git a/pkg/instancegroups/rollingupdate_os_test.go b/pkg/instancegroups/rollingupdate_os_test.go index ea8da3af80f99..b9cd4af870fec 100644 --- a/pkg/instancegroups/rollingupdate_os_test.go +++ b/pkg/instancegroups/rollingupdate_os_test.go @@ -61,7 +61,7 @@ func getTestSetupOS(t *testing.T, ctx context.Context) (*RollingUpdateCluster, * assetBuilder := assets.NewAssetBuilder(vfs.Context, inCluster.Spec.Assets, inCluster.Spec.KubernetesVersion, false) basePath, _ := vfs.Context.BuildVfsPath(inCluster.Spec.ConfigBase) clientset := vfsclientset.NewVFSClientset(vfs.Context, basePath) - cluster, err := cloudup.PopulateClusterSpec(ctx, clientset, inCluster, mockcloud, assetBuilder) + cluster, err := cloudup.PopulateClusterSpec(ctx, clientset, inCluster, nil, mockcloud, assetBuilder) if err != nil { t.Fatalf("Failed to populate cluster spec: %v", err) } diff --git a/upup/pkg/fi/cloudup/apply_cluster.go b/upup/pkg/fi/cloudup/apply_cluster.go index f0d374399408d..9d81ba902bf63 100644 --- a/upup/pkg/fi/cloudup/apply_cluster.go +++ b/upup/pkg/fi/cloudup/apply_cluster.go @@ -815,7 +815,7 @@ func (c *ApplyClusterCmd) Run(ctx context.Context) error { // upgradeSpecs ensures that fields are fully populated / defaulted func (c *ApplyClusterCmd) upgradeSpecs(ctx context.Context, assetBuilder *assets.AssetBuilder) error { - fullCluster, err := PopulateClusterSpec(ctx, c.Clientset, c.Cluster, c.Cloud, assetBuilder) + fullCluster, err := PopulateClusterSpec(ctx, c.Clientset, c.Cluster, c.InstanceGroups, c.Cloud, assetBuilder) if err != nil { return err } diff --git a/upup/pkg/fi/cloudup/populate_cluster_spec.go b/upup/pkg/fi/cloudup/populate_cluster_spec.go index 42216755a6171..c5503f7ba15f8 100644 --- a/upup/pkg/fi/cloudup/populate_cluster_spec.go +++ b/upup/pkg/fi/cloudup/populate_cluster_spec.go @@ -44,6 +44,9 @@ type populateClusterSpec struct { // We build it up into a complete config, but we write the values as input InputCluster *kopsapi.Cluster + // InputInstanceGroups are the instance groups in the cluster + InputInstanceGroups []*kopsapi.InstanceGroup + // fullCluster holds the built completed cluster spec fullCluster *kopsapi.Cluster @@ -53,11 +56,12 @@ type populateClusterSpec struct { // PopulateClusterSpec takes a user-specified cluster spec, and computes the full specification that should be set on the cluster. // We do this so that we don't need any real "brains" on the node side. -func PopulateClusterSpec(ctx context.Context, clientset simple.Clientset, cluster *kopsapi.Cluster, cloud fi.Cloud, assetBuilder *assets.AssetBuilder) (*kopsapi.Cluster, error) { +func PopulateClusterSpec(ctx context.Context, clientset simple.Clientset, cluster *kopsapi.Cluster, instanceGroups []*kopsapi.InstanceGroup, cloud fi.Cloud, assetBuilder *assets.AssetBuilder) (*kopsapi.Cluster, error) { c := &populateClusterSpec{ - cloud: cloud, - InputCluster: cluster, - assetBuilder: assetBuilder, + cloud: cloud, + InputCluster: cluster, + InputInstanceGroups: instanceGroups, + assetBuilder: assetBuilder, } err := c.run(ctx, clientset) if err != nil { @@ -217,6 +221,47 @@ func (c *populateClusterSpec) run(ctx context.Context, clientset simple.Clientse klog.V(2).Infof("Normalizing kubernetes version: %q -> %q", cluster.Spec.KubernetesVersion, versionWithoutV) cluster.Spec.KubernetesVersion = versionWithoutV } + + if cluster.Spec.CloudProvider.Openstack == nil { + if cluster.Spec.API.DNS == nil && cluster.Spec.API.LoadBalancer == nil { + subnetTypesByName := map[string]kopsapi.SubnetType{} + for _, subnet := range cluster.Spec.Networking.Subnets { + subnetTypesByName[subnet.Name] = subnet.Type + } + + haveAPIServerNodes := false + for _, ig := range c.InputInstanceGroups { + if ig.Spec.Role == kopsapi.InstanceGroupRoleAPIServer { + haveAPIServerNodes = true + } + } + for _, ig := range c.InputInstanceGroups { + if ig.Spec.Role == kopsapi.InstanceGroupRoleAPIServer || (!haveAPIServerNodes && ig.Spec.Role == kopsapi.InstanceGroupRoleControlPlane) { + for _, subnet := range ig.Spec.Subnets { + switch subnetTypesByName[subnet] { + case kopsapi.SubnetTypePrivate: + cluster.Spec.API.LoadBalancer = &kopsapi.LoadBalancerAccessSpec{} + case kopsapi.SubnetTypePublic: + cluster.Spec.API.DNS = &kopsapi.DNSAccessSpec{} + } + } + } + } + // If both public and private, go with the load balancer. + if cluster.Spec.API.LoadBalancer != nil { + cluster.Spec.API.DNS = nil + } + } + + if cluster.Spec.API.LoadBalancer != nil && cluster.Spec.API.LoadBalancer.Type == "" { + cluster.Spec.API.LoadBalancer.Type = kopsapi.LoadBalancerTypePublic + } + } + + if cluster.Spec.API.LoadBalancer != nil && cluster.Spec.API.LoadBalancer.Class == "" && cluster.Spec.CloudProvider.AWS != nil { + cluster.Spec.API.LoadBalancer.Class = kopsapi.LoadBalancerClassClassic + } + if cluster.Spec.DNSZone == "" && cluster.PublishesDNSRecords() { dns, err := cloud.DNS() if err != nil { diff --git a/upup/pkg/fi/cloudup/populate_cluster_spec_test.go b/upup/pkg/fi/cloudup/populate_cluster_spec_test.go index 684e3fc00cb71..39d7648f636dc 100644 --- a/upup/pkg/fi/cloudup/populate_cluster_spec_test.go +++ b/upup/pkg/fi/cloudup/populate_cluster_spec_test.go @@ -116,7 +116,7 @@ func mockedPopulateClusterSpec(ctx context.Context, c *kopsapi.Cluster, cloud fi return nil, fmt.Errorf("error building vfspath: %v", err) } clientset := vfsclientset.NewVFSClientset(vfs.Context, basePath) - return PopulateClusterSpec(ctx, clientset, c, cloud, assetBuilder) + return PopulateClusterSpec(ctx, clientset, c, nil, cloud, assetBuilder) } func TestPopulateCluster_Docker_Spec(t *testing.T) { From 1c0b75ae99606b5a189d1cf256f9464630ad75ef Mon Sep 17 00:00:00 2001 From: John Gardiner Myers Date: Fri, 13 Jan 2023 15:50:24 -0800 Subject: [PATCH 2/2] hack/update-expected.sh --- .../tests/kubeschedulerconfig/completed-cluster.yaml | 3 +-- .../kubescheduler/tests/minimal/completed-cluster.yaml | 3 +-- .../kubescheduler/tests/mixing/completed-cluster.yaml | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pkg/model/components/kubescheduler/tests/kubeschedulerconfig/completed-cluster.yaml b/pkg/model/components/kubescheduler/tests/kubeschedulerconfig/completed-cluster.yaml index ce7c4d7ec2c76..6507a7add90f9 100644 --- a/pkg/model/components/kubescheduler/tests/kubeschedulerconfig/completed-cluster.yaml +++ b/pkg/model/components/kubescheduler/tests/kubeschedulerconfig/completed-cluster.yaml @@ -2,8 +2,7 @@ metadata: creationTimestamp: null name: minimal.example.com spec: - api: - dns: {} + api: {} authorization: alwaysAllow: {} cloudProvider: {} diff --git a/pkg/model/components/kubescheduler/tests/minimal/completed-cluster.yaml b/pkg/model/components/kubescheduler/tests/minimal/completed-cluster.yaml index 0e97f83c76e50..d85f4e83e7b95 100644 --- a/pkg/model/components/kubescheduler/tests/minimal/completed-cluster.yaml +++ b/pkg/model/components/kubescheduler/tests/minimal/completed-cluster.yaml @@ -2,8 +2,7 @@ metadata: creationTimestamp: null name: minimal.example.com spec: - api: - dns: {} + api: {} authorization: alwaysAllow: {} cloudProvider: {} diff --git a/pkg/model/components/kubescheduler/tests/mixing/completed-cluster.yaml b/pkg/model/components/kubescheduler/tests/mixing/completed-cluster.yaml index c415caa87136f..44acb6b4ba1cb 100644 --- a/pkg/model/components/kubescheduler/tests/mixing/completed-cluster.yaml +++ b/pkg/model/components/kubescheduler/tests/mixing/completed-cluster.yaml @@ -2,8 +2,7 @@ metadata: creationTimestamp: null name: minimal.example.com spec: - api: - dns: {} + api: {} authorization: alwaysAllow: {} cloudProvider: {}