From ac13fa610c4ec2b741731ff917d7cc38b01fb2e7 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Sat, 10 Feb 2024 14:17:35 -0600 Subject: [PATCH 1/3] Move DNS topology setup earlier in cluster creation This is needed because setting the bastion public name field depends on the DNS topology. We were incorrectly setting bastion.publicName for dns=none clusters because the dns=none field wasn't yet set on the cluster. --- upup/pkg/fi/cloudup/new_cluster.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/upup/pkg/fi/cloudup/new_cluster.go b/upup/pkg/fi/cloudup/new_cluster.go index 4953ff1057..14e9e3ee9b 100644 --- a/upup/pkg/fi/cloudup/new_cluster.go +++ b/upup/pkg/fi/cloudup/new_cluster.go @@ -1267,6 +1267,12 @@ func setupTopology(opt *NewClusterOptions, cluster *api.Cluster, allZones sets.S } cluster.Spec.Networking.Topology = &api.TopologySpec{} + + err := setupDNSTopology(opt, cluster) + if err != nil { + return nil, err + } + switch opt.Topology { case api.TopologyPublic: @@ -1405,10 +1411,6 @@ func setupTopology(opt *NewClusterOptions, cluster *api.Cluster, allZones sets.S } } - err := setupDNSTopology(opt, cluster) - if err != nil { - return nil, err - } return bastions, nil } From c58b33a627faf5a70a52b1f7932d74d23a1884f1 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Sat, 10 Feb 2024 14:19:05 -0600 Subject: [PATCH 2/3] Add unit test for setupTopology --- upup/pkg/fi/cloudup/new_cluster_test.go | 97 ++++++++++++++++++++++++- 1 file changed, 96 insertions(+), 1 deletion(-) diff --git a/upup/pkg/fi/cloudup/new_cluster_test.go b/upup/pkg/fi/cloudup/new_cluster_test.go index c85f0115a2..20c3351ab0 100644 --- a/upup/pkg/fi/cloudup/new_cluster_test.go +++ b/upup/pkg/fi/cloudup/new_cluster_test.go @@ -87,7 +87,6 @@ func TestCreateEtcdCluster(t *testing.T) { func TestSetupNetworking(t *testing.T) { tests := []struct { options NewClusterOptions - actual api.Cluster expected api.Cluster }{ { @@ -337,6 +336,102 @@ func TestSetupNetworking(t *testing.T) { } } +func TestSetupTopology(t *testing.T) { + tests := []struct { + options NewClusterOptions + skeleton api.Cluster + expected api.Cluster + }{ + { + options: NewClusterOptions{ + Topology: api.TopologyPrivate, + Bastion: true, + }, + skeleton: api.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: api.ClusterSpec{ + KubernetesVersion: "v1.29.0", + Networking: api.NetworkingSpec{ + Topology: &api.TopologySpec{ + DNS: api.DNSTypeNone, + }, + }, + }, + }, + expected: api.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: api.ClusterSpec{ + KubernetesVersion: "v1.29.0", + Networking: api.NetworkingSpec{ + Topology: &api.TopologySpec{ + DNS: api.DNSTypeNone, + }, + }, + }, + }, + }, + { + options: NewClusterOptions{ + Topology: api.TopologyPrivate, + DNSType: string(api.DNSTypePublic), + Bastion: true, + }, + skeleton: api.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: api.ClusterSpec{ + KubernetesVersion: "v1.29.0", + Networking: api.NetworkingSpec{ + Topology: &api.TopologySpec{ + DNS: api.DNSTypePublic, + }, + }, + }, + }, + expected: api.Cluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + }, + Spec: api.ClusterSpec{ + KubernetesVersion: "v1.29.0", + Networking: api.NetworkingSpec{ + Topology: &api.TopologySpec{ + DNS: api.DNSTypePublic, + Bastion: &api.BastionSpec{ + PublicName: "bastion.test", + }, + }, + }, + }, + }, + }, + } + + for _, test := range tests { + _, err := setupTopology(&test.options, &test.skeleton, nil) + if err != nil { + t.Errorf("error during topology setup: %v", err) + } + expectedYaml, err := yaml.Marshal(test.expected) + if err != nil { + t.Errorf("error converting expected cluster spec to yaml: %v", err) + } + actualYaml, err := yaml.Marshal(&test.skeleton) + if err != nil { + t.Errorf("error converting actual cluster spec to yaml: %v", err) + } + if string(expectedYaml) != string(actualYaml) { + diffString := diff.FormatDiff(string(expectedYaml), string(actualYaml)) + t.Errorf("unexpected cluster topology setup:\n%s", diffString) + } + } +} + func TestDefaultImage(t *testing.T) { tests := []struct { cluster *api.Cluster From e6652feb3038f8ef0655bf353ba4ab557fefca55 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Sat, 10 Feb 2024 14:19:38 -0600 Subject: [PATCH 3/3] ./hack/update-expected.sh --- .../create_cluster/complex-private/expected-v1alpha2.yaml | 2 -- .../create_cluster/ingwspecified/expected-v1alpha2.yaml | 2 -- .../create_cluster/ngwspecified/expected-v1alpha2.yaml | 2 -- tests/integration/create_cluster/private/expected-v1alpha2.yaml | 2 -- .../create_cluster/private_gce/expected-v1alpha2.yaml | 2 -- 5 files changed, 10 deletions(-) diff --git a/tests/integration/create_cluster/complex-private/expected-v1alpha2.yaml b/tests/integration/create_cluster/complex-private/expected-v1alpha2.yaml index 00851163df..54eb6f6403 100644 --- a/tests/integration/create_cluster/complex-private/expected-v1alpha2.yaml +++ b/tests/integration/create_cluster/complex-private/expected-v1alpha2.yaml @@ -122,8 +122,6 @@ spec: type: Utility zone: us-test-1a topology: - bastion: - bastionPublicName: bastion.complex.example.com dns: type: None diff --git a/tests/integration/create_cluster/ingwspecified/expected-v1alpha2.yaml b/tests/integration/create_cluster/ingwspecified/expected-v1alpha2.yaml index f105a69a14..09ff08b84b 100644 --- a/tests/integration/create_cluster/ingwspecified/expected-v1alpha2.yaml +++ b/tests/integration/create_cluster/ingwspecified/expected-v1alpha2.yaml @@ -59,8 +59,6 @@ spec: type: Utility zone: us-test-1a topology: - bastion: - bastionPublicName: bastion.private.example.com dns: type: None diff --git a/tests/integration/create_cluster/ngwspecified/expected-v1alpha2.yaml b/tests/integration/create_cluster/ngwspecified/expected-v1alpha2.yaml index 6b00441e64..e231e703be 100644 --- a/tests/integration/create_cluster/ngwspecified/expected-v1alpha2.yaml +++ b/tests/integration/create_cluster/ngwspecified/expected-v1alpha2.yaml @@ -59,8 +59,6 @@ spec: type: Utility zone: us-test-1a topology: - bastion: - bastionPublicName: bastion.private.example.com dns: type: None diff --git a/tests/integration/create_cluster/private/expected-v1alpha2.yaml b/tests/integration/create_cluster/private/expected-v1alpha2.yaml index dcb5bcc368..67be04d6dd 100644 --- a/tests/integration/create_cluster/private/expected-v1alpha2.yaml +++ b/tests/integration/create_cluster/private/expected-v1alpha2.yaml @@ -62,8 +62,6 @@ spec: type: Utility zone: us-test-1a topology: - bastion: - bastionPublicName: bastion.private.example.com dns: type: None diff --git a/tests/integration/create_cluster/private_gce/expected-v1alpha2.yaml b/tests/integration/create_cluster/private_gce/expected-v1alpha2.yaml index f0fedae6fc..28fa07b222 100644 --- a/tests/integration/create_cluster/private_gce/expected-v1alpha2.yaml +++ b/tests/integration/create_cluster/private_gce/expected-v1alpha2.yaml @@ -57,8 +57,6 @@ spec: region: us-test1 type: Private topology: - bastion: - bastionPublicName: bastion.private.example.com dns: type: None