From 57596785616e97f6cdd122d57ef73ad78a99f272 Mon Sep 17 00:00:00 2001 From: Jeremiah Stuever Date: Thu, 12 Dec 2024 13:57:36 -0800 Subject: [PATCH 1/4] privatelink: ready and failed conditions logged only when changed --- .../privatelink/conditions/conditions.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/controller/privatelink/conditions/conditions.go b/pkg/controller/privatelink/conditions/conditions.go index f8e84e36685..8d6c9a38988 100644 --- a/pkg/controller/privatelink/conditions/conditions.go +++ b/pkg/controller/privatelink/conditions/conditions.go @@ -58,8 +58,13 @@ func SetErrCondition(client client.Client, cd *hivev1.ClusterDeployment, reason if !readyChanged && !failedChanged { return nil } + if failedChanged { + logger.Debug("setting PrivateLinkFailedClusterDeploymentCondition to true") + } + if readyChanged { + logger.Debug("setting PrivateLinkReadyClusterDeploymentCondition to false") + } curr.Status.Conditions = conditions - logger.Debug("setting PrivateLinkFailedClusterDeploymentCondition to true") return client.Status().Update(context.TODO(), curr) } @@ -120,8 +125,13 @@ func SetReadyCondition(client client.Client, cd *hivev1.ClusterDeployment, compl if !readyChanged && !failedChanged { return nil } + if failedChanged { + logger.Debug("setting PrivateLinkFailedClusterDeploymentCondition to false") + } + if readyChanged { + logger.Debugf("setting PrivateLinkReadyClusterDeploymentCondition to %s", completed) + } curr.Status.Conditions = conditions - logger.Debugf("setting PrivateLinkReadyClusterDeploymentCondition to %s", completed) return client.Status().Update(context.TODO(), curr) } From 243e835b8accd80ada8d78f284918e1efa8b2186 Mon Sep 17 00:00:00 2001 From: Jeremiah Stuever Date: Thu, 12 Dec 2024 14:28:36 -0800 Subject: [PATCH 2/4] privatelink: ShouldSync returns false when privatelink is disabled --- .../actuator/awsactuator/awshubactuator.go | 5 ++ .../awsactuator/awshubactuator_test.go | 51 +++++++++--- .../actuator/gcpactuator/gcplinkactuator.go | 7 +- .../gcpactuator/gcplinkactuator_test.go | 79 ++++++++++++++----- 4 files changed, 111 insertions(+), 31 deletions(-) diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go index 4c4b3b39f5f..16e31fc3521 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go @@ -184,6 +184,11 @@ func (a *AWSHubActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hivev // ShouldSync is the actuator interface to determine if there are changes that need to be made. func (a *AWSHubActuator) ShouldSync(cd *hivev1.ClusterDeployment) bool { + if cd.Spec.Platform.AWS == nil || + cd.Spec.Platform.AWS.PrivateLink == nil || + !cd.Spec.Platform.AWS.PrivateLink.Enabled { + return false + } return cd.Status.Platform == nil || cd.Status.Platform.AWS == nil || cd.Status.Platform.AWS.PrivateLink == nil || diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go index fd57c77372d..a396ce5673a 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go @@ -420,35 +420,64 @@ func Test_ShouldSync(t *testing.T) { cd *hivev1.ClusterDeployment expect bool - }{{ // Sync is required when status.platform is nil - name: "status.platform is nil", - cd: cdBuilder.Build(), + }{{ // Sync is not required when spec.platform.aws is nil + name: "spec.plaform is nil", + cd: cdBuilder.Build(), + }, { // Sync is not required when spec.platform.aws.privatelink is nil + name: "spec.plaform.aws.privatelink is nil", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + }, { // Sync is not required when spec.platform.aws.privatelink.enabled is false + name: "privatelink is not enabled", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: false}, + })), + }, { // Sync is required when status.platform is nil + name: "status.platform is nil", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: true}, + })), expect: true, }, { // Sync is required when status.platform.aws is nil - name: "status.platform.aws is nil", - cd: cdBuilder.Options(testcd.WithEmptyPlatformStatus()).Build(), + name: "status.platform.aws is nil", + cd: cdBuilder.Build( + testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: true}, + }), + testcd.WithEmptyPlatformStatus(), + ), expect: true, }, { // Sync is required when status.platform.aws.privatelink is nil - name: "status.platform.aws.privatelink is nil", - cd: cdBuilder.Options(testcd.WithAWSPlatformStatus(&hivev1aws.PlatformStatus{})).Build(), + name: "status.platform.aws.privatelink is nil", + cd: cdBuilder.Build( + testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: true}, + }), + testcd.WithAWSPlatformStatus(&hivev1aws.PlatformStatus{}), + ), expect: true, }, { // Sync is required when hostedZoneID is empty name: "hostedZoneID is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: true}, + }), testcd.WithAWSPlatformStatus(&hivev1aws.PlatformStatus{ PrivateLink: &hivev1aws.PrivateLinkAccessStatus{}, }), - ).Build(), + ), expect: true, }, { // Sync is not required when hostedZoneID is set name: "hostedZoneID is not empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithAWSPlatform(&hivev1aws.Platform{ + PrivateLink: &hivev1aws.PrivateLinkAccess{Enabled: true}, + }), testcd.WithAWSPlatformStatus(&hivev1aws.PlatformStatus{ PrivateLink: &hivev1aws.PrivateLinkAccessStatus{ HostedZoneID: testHostedZone, }, }), - ).Build(), + ), }} for _, test := range cases { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go index 9fda7c35c52..18dd246ceea 100644 --- a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go +++ b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go @@ -263,8 +263,13 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive // ShouldSync is the actuator interface to determine if there are changes that need to be made. func (a *GCPLinkActuator) ShouldSync(cd *hivev1.ClusterDeployment) bool { - shouldCreateSubnet := getServiceAttachmentSubnetExistingName(cd) == "" + if cd.Spec.Platform.GCP == nil || + cd.Spec.Platform.GCP.PrivateServiceConnect == nil || + !cd.Spec.Platform.GCP.PrivateServiceConnect.Enabled { + return false + } + shouldCreateSubnet := getServiceAttachmentSubnetExistingName(cd) == "" return cd.Status.Platform == nil || cd.Status.Platform.GCP == nil || cd.Status.Platform.GCP.PrivateServiceConnect == nil || diff --git a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go index ee5511d13ae..e4f58518220 100644 --- a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go +++ b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go @@ -886,21 +886,47 @@ func Test_ShouldSync(t *testing.T) { cd *hivev1.ClusterDeployment expect bool - }{{ // Sync is required when status.platform is nil - name: "status.platform is nil", - cd: cdBuilder.Build(), + }{{ // Sync is not required when spec.platform.gcp is nil + name: "spec.platform.gcp is nil", + cd: cdBuilder.Build(), + }, { // Sync is not required when spec.platform.gcp.privateServiceConnect is nil + name: "spec.platform.gcp.privateServiceConnect", + cd: cdBuilder.Build(testcd.WithGCPPlatform(&hivev1gcp.Platform{})), + }, { // Sync is not required when spec.platform.gcp.privateServiceConnect.enabled is false + name: "privateServiceConnect is not enabled", + cd: cdBuilder.Build(testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: false}, + })), + }, { // Sync is required when status.platform is nil + name: "status.platform is nil", + cd: cdBuilder.Build(testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + })), expect: true, }, { // Sync is required when status.platform.gcp is nil - name: "status.platform.gcp is nil", - cd: cdBuilder.Options(testcd.WithEmptyPlatformStatus()).Build(), + name: "status.platform.gcp is nil", + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), + testcd.WithEmptyPlatformStatus(), + ), expect: true, }, { // Sync is required when status.platform.gcp.privateServiceConnect is nil - name: "status.platform.aws.privateServiceConnect is nil", - cd: cdBuilder.Options(testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{})).Build(), + name: "status.platform.aws.privateServiceConnect is nil", + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{}), + ), expect: true, }, { // Sync is required when endpoint is empty name: "Endpoint is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ EndpointAddress: mockAddress.SelfLink, @@ -909,11 +935,14 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentSubnet: mockSubnet.SelfLink, }, }), - ).Build(), + ), expect: true, }, { // Sync is required when EndpointAddress is empty name: "EndpointAddress is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ Endpoint: mockForwardingRule.SelfLink, @@ -922,11 +951,14 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentSubnet: mockSubnet.SelfLink, }, }), - ).Build(), + ), expect: true, }, { // Sync is required when ServiceAttachment is empty name: "ServiceAttachment is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ Endpoint: mockForwardingRule.SelfLink, @@ -935,11 +967,14 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentSubnet: mockSubnet.SelfLink, }, }), - ).Build(), + ), expect: true, }, { // Sync is required when ServiceAttachmentFirewall is empty name: "ServiceAttachmentFirewall is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ Endpoint: mockForwardingRule.SelfLink, @@ -948,11 +983,14 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentSubnet: mockSubnet.SelfLink, }, }), - ).Build(), + ), expect: true, }, { // Sync is required when ServiceAttachmentSubnet is empty name: "ServiceAttachmentSubnet is empty", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ Endpoint: mockForwardingRule.SelfLink, @@ -961,11 +999,14 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentFirewall: mockFirewall.SelfLink, }, }), - ).Build(), + ), expect: true, }, { // Sync is not required when hostedZoneID is set name: "no changes required", - cd: cdBuilder.Options( + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{ + PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{Enabled: true}, + }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ Endpoint: mockForwardingRule.SelfLink, @@ -975,7 +1016,7 @@ func Test_ShouldSync(t *testing.T) { ServiceAttachmentSubnet: mockSubnet.SelfLink, }, }), - ).Build(), + ), }} for _, test := range cases { t.Run(test.name, func(t *testing.T) { From 2c6274d0f586ccffc3178ad7ed11f187fde92174 Mon Sep 17 00:00:00 2001 From: Jeremiah Stuever Date: Fri, 13 Dec 2024 12:55:36 -0800 Subject: [PATCH 3/4] privatelink: actuator reconcile functions to requeue on every change --- .../actuator/awsactuator/awshubactuator.go | 105 ++++++ .../awsactuator/awshubactuator_test.go | 339 +++++++++++++++++- .../actuator/gcpactuator/gcplinkactuator.go | 5 + .../gcpactuator/gcplinkactuator_test.go | 269 ++++++++++++-- 4 files changed, 674 insertions(+), 44 deletions(-) diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go index 16e31fc3521..bc5beb99b2b 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go @@ -140,6 +140,7 @@ func (a *AWSHubActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hivev if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } logger.Debug("reconciling Hosted Zone Records") @@ -158,6 +159,7 @@ func (a *AWSHubActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hivev if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } logger.Debug("reconciling Hosted Zone Associations") @@ -177,6 +179,7 @@ func (a *AWSHubActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hivev if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } return reconcile.Result{}, nil @@ -374,11 +377,113 @@ func (a *AWSHubActuator) cleanupHostedZone(cd *hivev1.ClusterDeployment, metadat } func (a *AWSHubActuator) ReconcileHostedZoneRecords(cd *hivev1.ClusterDeployment, hostedZoneID string, dnsRecord *actuator.DnsRecord, apiDomain string, logger log.FieldLogger) (bool, error) { + hzLog := logger.WithField("hostedZoneID", hostedZoneID) + modified := false + rSet, err := a.recordSet(cd, apiDomain, dnsRecord) if err != nil { return false, errors.Wrap(err, "error generating DNS records") } + recordsResp, err := a.awsClientHub.ListResourceRecordSets(&route53.ListResourceRecordSetsInput{ + HostedZoneId: aws.String(hostedZoneID), + }) + if err != nil { + return false, errors.Wrapf(err, "failed to list the hosted zone %s", hostedZoneID) + } + + for _, record := range recordsResp.ResourceRecordSets { + if *record.Name != *rSet.Name { + continue + } + if rSet.ResourceRecords != nil { + if aws.StringValue(record.Type) != aws.StringValue(rSet.Type) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "type": aws.StringValue(rSet.Type), + }).Debug("updating record type") + } + if aws.Int64Value(record.TTL) != aws.Int64Value(rSet.TTL) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "ttl": aws.Int64Value(rSet.TTL), + }).Debug("updating record ttl") + } + + oldRecords := sets.NewString() + for _, record := range record.ResourceRecords { + oldRecords.Insert(aws.StringValue(record.Value)) + } + + desiredRecords := sets.NewString() + for _, record := range rSet.ResourceRecords { + desiredRecords.Insert(aws.StringValue(record.Value)) + } + + added := desiredRecords.Difference(oldRecords).List() + removed := oldRecords.Difference(desiredRecords).List() + + if len(added) > 0 || len(removed) > 0 { + modified = true + hzLog.WithFields(log.Fields{ + "added": added, + "removed": removed, + }).Debug("updating the addresses assigned to the dns record") + } + + if !modified { + return false, nil + } + } else if rSet.AliasTarget != nil { + logger.Debugf("AliasTarget") + if record.AliasTarget == nil { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + }).Debug("updating the record to use alias target") + break + } + + if aws.StringValue(record.Type) != aws.StringValue(rSet.Type) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "type": aws.StringValue(rSet.Type), + }).Debug("updating record type") + } + + if aws.StringValue(record.AliasTarget.DNSName) != aws.StringValue(rSet.AliasTarget.DNSName) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "dnsName": aws.StringValue(rSet.AliasTarget.DNSName), + }).Debug("updating the aliasTarget dnsName") + } + + if aws.StringValue(record.AliasTarget.HostedZoneId) != aws.StringValue(rSet.AliasTarget.HostedZoneId) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "hostedZoneId": aws.StringValue(rSet.AliasTarget.HostedZoneId), + }).Debug("updating the aliasTarget hostedZoneId") + } + + if aws.BoolValue(record.AliasTarget.EvaluateTargetHealth) != aws.BoolValue(rSet.AliasTarget.EvaluateTargetHealth) { + modified = true + hzLog.WithFields(log.Fields{ + "record": aws.StringValue(rSet.Name), + "evaluateTargetHealth": aws.BoolValue(rSet.AliasTarget.EvaluateTargetHealth), + }).Debug("updating the aliasTarget evaluateTargetHealth") + } + + if !modified { + return false, nil + } + } + break + } _, err = a.awsClientHub.ChangeResourceRecordSets(&route53.ChangeResourceRecordSetsInput{ HostedZoneId: aws.String(hostedZoneID), ChangeBatch: &route53.ChangeBatch{ diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go index a396ce5673a..8a6a1adb265 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go @@ -19,6 +19,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/reconcile" hivev1 "github.com/openshift/hive/apis/hive/v1" hivev1aws "github.com/openshift/hive/apis/hive/v1/aws" @@ -248,9 +249,10 @@ func Test_Reconcile(t *testing.T) { AWSClientConfig func(*mock.MockClient) + expect reconcile.Result expectConditions []hivev1.ClusterDeploymentCondition expectError string - expectLogs []string + expectStatus *hivev1aws.PrivateLinkAccessStatus }{{ // There should be an error on failure to initialURL name: "failure on initialURL", cd: cdBuilder.Build( @@ -267,7 +269,7 @@ func Test_Reconcile(t *testing.T) { }}, expectError: "could not get API URL from kubeconfig: failed to load the kubeconfig: kubeconfig secret does not contain necessary data", }, { // There should be an error on failure to ensureHostedZone - name: "failure on ensureHostedZone SetErrConditionWithRetry", + name: "failure on ensureHostedZone", cd: cdBuilder.Build( testcd.WithClusterMetadata(&hivev1.ClusterMetadata{AdminKubeconfigSecretRef: corev1.LocalObjectReference{Name: "kubeconfig"}}), ), @@ -282,6 +284,30 @@ func Test_Reconcile(t *testing.T) { Message: "at least one associated VPC must be configured", }}, expectError: "failed to reconcile the Hosted Zone: at least one associated VPC must be configured", + }, { // Ready condition should be updated when hzModified + name: "hzModified", + cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{AdminKubeconfigSecretRef: corev1.LocalObjectReference{Name: "kubeconfig"}}), + ), + existing: []runtime.Object{ + mockSecret("kubeconfig", mockKubeconfigData), + }, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListHostedZonesByVPC(gomock.Any()).Return(&route53.ListHostedZonesByVPCOutput{ + HostedZoneSummaries: []*route53.HostedZoneSummary{{ + Name: aws.String(testAPIDomain), + HostedZoneId: aws.String(testHostedZone), + }}, + }, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledPrivateHostedZone", + Message: "reconciled the Private Hosted Zone for the VPC Endpoint of the cluster", + }}, + expectStatus: &hivev1aws.PrivateLinkAccessStatus{HostedZoneID: testHostedZone}, }, { // There should be an error on failure to ReconcileHostedZoneRecords name: "failure on ReconcileHostedZoneRecords", cd: cdBuilder.Build( @@ -310,6 +336,7 @@ func Test_Reconcile(t *testing.T) { Message: "error generating DNS records: configured to use ip address, but no address found.", }}, expectError: "failed to reconcile the Hosted Zone Records: error generating DNS records: configured to use ip address, but no address found.", + // }, { // Ready condition should be updated when recordsModified }, { // There should be an error on failure to reconcileHostedZoneAssociations name: "failure on reconcileHostedZoneAssociations", cd: cdBuilder.Build( @@ -331,7 +358,16 @@ func Test_Reconcile(t *testing.T) { HostedZoneId: aws.String(testHostedZone), }}, }, nil) - m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) m.EXPECT().GetHostedZone(gomock.Any()).Return(nil, awserr.New("AccessDenied", "not authorized to GetHostedZone", nil)) }, expectConditions: []hivev1.ClusterDeploymentCondition{{ @@ -341,7 +377,52 @@ func Test_Reconcile(t *testing.T) { Message: "failed to get the Hosted Zone: AccessDenied: not authorized to GetHostedZone", }}, expectError: "failed to reconcile the Hosted Zone Associations: failed to get the Hosted Zone: AccessDenied: not authorized to GetHostedZone", - }, { // There should be no errors on success + }, { // Ready condition should be updated on associationsModified + name: "associationsModified", + cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{AdminKubeconfigSecretRef: corev1.LocalObjectReference{Name: "kubeconfig"}}), + testcd.WithAWSPlatformStatus(&hivev1aws.PlatformStatus{ + PrivateLink: &hivev1aws.PrivateLinkAccessStatus{ + HostedZoneID: testHostedZone, + }, + }), + ), + existing: []runtime.Object{ + mockSecret("kubeconfig", mockKubeconfigData), + }, + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListHostedZonesByVPC(gomock.Any()).Return(&route53.ListHostedZonesByVPCOutput{ + HostedZoneSummaries: []*route53.HostedZoneSummary{{ + Name: aws.String(testAPIDomain), + HostedZoneId: aws.String(testHostedZone), + }}, + }, nil) + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) + m.EXPECT().GetHostedZone(gomock.Any()).Return(&route53.GetHostedZoneOutput{ + HostedZone: &route53.HostedZone{ + Id: aws.String(testHostedZone), + }, + }, nil) + m.EXPECT().AssociateVPCWithHostedZone(gomock.Any()).Return(&route53.AssociateVPCWithHostedZoneOutput{}, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledAssociationsToVPCs", + Message: "reconciled the associations of all the required VPCs to the Private Hosted Zone for the VPC Endpoint", + }}, + }, { // There should be no error on success name: "success", cd: cdBuilder.Build( testcd.WithClusterMetadata(&hivev1.ClusterMetadata{AdminKubeconfigSecretRef: corev1.LocalObjectReference{Name: "kubeconfig"}}), @@ -362,7 +443,16 @@ func Test_Reconcile(t *testing.T) { HostedZoneId: aws.String(testHostedZone), }}, }, nil) - m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) m.EXPECT().GetHostedZone(gomock.Any()).Return(&route53.GetHostedZoneOutput{ HostedZone: &route53.HostedZone{ Id: aws.String(testHostedZone), @@ -380,7 +470,7 @@ func Test_Reconcile(t *testing.T) { if test.config == nil { test.config = &hivev1.AWSPrivateLinkConfig{AssociatedVPCs: mockAssociatedVPCs} } - logger, hook := testlogger.NewLoggerWithHook() + logger, _ := testlogger.NewLoggerWithHook() awsHubActuator, err := newTestAWSHubActuator(t, test.config, test.cd, @@ -398,15 +488,15 @@ func Test_Reconcile(t *testing.T) { assert.EqualError(t, err, test.expectError) } + if test.expectStatus != nil { + assert.Equal(t, test.expectStatus, test.cd.Status.Platform.AWS.PrivateLink) + } + curr := &hivev1.ClusterDeployment{} fakeClient := *awsHubActuator.client errGet := fakeClient.Get(context.TODO(), types.NamespacedName{Namespace: test.cd.Namespace, Name: test.cd.Name}, curr) assert.NoError(t, errGet) testassert.AssertConditions(t, curr, test.expectConditions) - - for _, log := range test.expectLogs { - testlogger.AssertHookContainsMessage(t, hook, log) - } }) } } @@ -978,11 +1068,239 @@ func Test_ReconcileHostedZoneRecords(t *testing.T) { name: "recordSet failed", cd: cdBuilder.Build(), expectError: "error generating DNS records: configured to use ip address, but no address found.", + }, { // There should be an error on ListResourceRecordSets failure + name: "ListResourceRecordSets failed", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(nil, awserr.New("AccessDenied", "not authorized to ListResourceRecordSets", nil)) + }, + expectError: "failed to list the hosted zone testhostedzone: AccessDenied: not authorized to ListResourceRecordSets", + }, { // Should update record Type when using ResourceRecords and Type is different + name: "ResourceRecords type is different", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1", "0.0.0.2"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }, { + Value: aws.String("0.0.0.2"), + }}, + Type: aws.String("different"), + TTL: aws.Int64(10), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update record TTL when using ResourceRecords and TTL is different + name: "ResourceRecords ttl is different", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1", "0.0.0.2"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }, { + Value: aws.String("0.0.0.2"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(11), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update records when using ResourceRecords and an address is missing + name: "ResourceRecords missing address", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1", "0.0.0.2"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update records when using ResourceRecords and there is an extra address + name: "ResourceRecords missing address", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1", "0.0.0.2"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }, { + Value: aws.String("0.0.0.2"), + }, { + Value: aws.String("0.0.0.3"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // No changes when using ResourceRecords and everything matches + name: "ResourceRecords no change", + cd: cdBuilder.Build(), + record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1", "0.0.0.2"}}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + ResourceRecords: []*route53.ResourceRecord{{ + Value: aws.String("0.0.0.1"), + }, { + Value: aws.String("0.0.0.2"), + }}, + Type: aws.String("A"), + TTL: aws.Int64(10), + }}, + }, nil) + }, + }, { // Should update record when using AliasTarget and AliasTarget is nil + name: "AliasTarget is nil", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + Type: aws.String("A"), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update record Type when using AliasTarget and Type is different + name: "AliasTarget type is different", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("testAliasTarget"), + HostedZoneId: aws.String(testHostedZone), + }, + Type: aws.String("different"), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update record DNSName when using AliasTarget and DNSName is different + name: "DNSName type is different", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("different"), + HostedZoneId: aws.String(testHostedZone), + }, + Type: aws.String("A"), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update record HostedZoneId when using AliasTarget and HostedZoneId is different + name: "HostedZoneId type is different", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("testAliasTarget"), + HostedZoneId: aws.String("different"), + }, + Type: aws.String("A"), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // Should update record EvaluateTargetHealth when using AliasTarget and EvaluateTargetHealth is different + name: "EvaluateTargetHealth type is different", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("testAliasTarget"), + HostedZoneId: aws.String(testHostedZone), + EvaluateTargetHealth: aws.Bool(true), + }, + Type: aws.String("A"), + }}, + }, nil) + m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, nil) + }, + expect: true, + }, { // No changes when using ResourceRecords and everything matches + name: "AliasTarget no change", + cd: cdBuilder.Build(testcd.WithAWSPlatform(&hivev1aws.Platform{})), + record: &actuator.DnsRecord{AliasTarget: actuator.AliasTarget{ + Name: "testAliasTarget", + HostedZoneID: testHostedZone, + }}, + AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{ + ResourceRecordSets: []*route53.ResourceRecordSet{{ + Name: aws.String(testAPIDomain), + AliasTarget: &route53.AliasTarget{ + DNSName: aws.String("testAliasTarget"), + HostedZoneId: aws.String(testHostedZone), + }, + Type: aws.String("A"), + }}, + }, nil) + }, }, { // There should be an error on failure to change the record set in the aws api name: "ChangeResourceRecordSets failed", cd: cdBuilder.Build(), record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1"}}, AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{}, nil) m.EXPECT().ChangeResourceRecordSets(gomock.Any()).Return(nil, awserr.New("AccessDenied", "not authorized to ChangeResourceRecordSets", nil)) }, expectError: "error adding record to Hosted Zone testhostedzone for VPC Endpoint: AccessDenied: not authorized to ChangeResourceRecordSets", @@ -991,6 +1309,7 @@ func Test_ReconcileHostedZoneRecords(t *testing.T) { cd: cdBuilder.Build(), record: &actuator.DnsRecord{IpAddress: []string{"0.0.0.1"}}, AWSClientConfig: func(m *mock.MockClient) { + m.EXPECT().ListResourceRecordSets(gomock.Any()).Return(&route53.ListResourceRecordSetsOutput{}, nil) m.EXPECT().ChangeResourceRecordSets(&route53.ChangeResourceRecordSetsInput{ HostedZoneId: aws.String(testHostedZone), ChangeBatch: &route53.ChangeBatch{ diff --git a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go index 18dd246ceea..fa254f41e2c 100644 --- a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go +++ b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator.go @@ -179,6 +179,7 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } subnet = newSubnet @@ -198,6 +199,7 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } } @@ -217,6 +219,7 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } logger.Debug("reconciling Endpoint Address") @@ -235,6 +238,7 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } logger.Debug("reconciling Endpoint") @@ -253,6 +257,7 @@ func (a *GCPLinkActuator) Reconcile(cd *hivev1.ClusterDeployment, metadata *hive if err != nil { return reconcile.Result{}, errors.Wrap(err, "failed to update condition on cluster deployment") } + return reconcile.Result{Requeue: true}, nil } // Set the DNS IP Addresses for the hub actuator. diff --git a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go index e4f58518220..5265bb86bad 100644 --- a/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go +++ b/pkg/controller/privatelink/actuator/gcpactuator/gcplinkactuator_test.go @@ -447,9 +447,9 @@ func Test_Reconcile(t *testing.T) { }, { // Should return requeue later when forwarding rule is not found name: "forwarding rule not found", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), - testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -471,9 +471,9 @@ func Test_Reconcile(t *testing.T) { }, { // There should be an error on GetForwardingRule failure name: "GetForwardingRule failure", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), - testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -495,6 +495,7 @@ func Test_Reconcile(t *testing.T) { }, { // There should be an error on GetSubnet failure when existing subnet is specified name: "Existing Subnet, GetSubnet failure", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{ PrivateServiceConnect: &hivev1gcp.PrivateServiceConnect{ ServiceAttachment: &hivev1gcp.ServiceAttachment{ @@ -508,7 +509,6 @@ func Test_Reconcile(t *testing.T) { Region: testRegion, }), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), - testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -543,7 +543,11 @@ func Test_Reconcile(t *testing.T) { }, Region: testRegion, }), - testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachment: mockServiceAttachment.SelfLink, + EndpointAddress: mockAddress.SelfLink, + Endpoint: mockForwardingRule.SelfLink, + }}), testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ @@ -566,12 +570,6 @@ func Test_Reconcile(t *testing.T) { m.EXPECT().GetAddress(gomock.Any(), gomock.Any()).Return(mockAddress, nil) m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) }, - expectConditions: []hivev1.ClusterDeploymentCondition{{ - Status: corev1.ConditionFalse, - Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, - Reason: "ReconciledEndpoint", - Message: "reconciled the Endpoint", - }}, expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ ServiceAttachment: mockServiceAttachment.SelfLink, EndpointAddress: mockAddress.SelfLink, @@ -581,9 +579,9 @@ func Test_Reconcile(t *testing.T) { }, { // There should be an error on GetNetwork failure name: "GetNetwork failure", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), - testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -606,9 +604,9 @@ func Test_Reconcile(t *testing.T) { }, { // There should be an error on ensureServiceAttachmentSubnet failure name: "ensureServiceAttachmentSubnet failure", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), - testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -635,12 +633,50 @@ func Test_Reconcile(t *testing.T) { Message: "googleapi: Error 401: not authorized to GetSubnet, AccessDenied", }}, expectError: "failed to reconcile the Service Attachment Subnet: googleapi: Error 401: not authorized to GetSubnet, AccessDenied", - }, { // There should be an error on ensureServiceAttachmentFirewall failure - name: "ensureServiceAttachmentFirewall failure", + }, { // Ready condition should be updated when subnetModified + name: "subnetModified", cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), + ), + config: &hivev1.GCPPrivateServiceConnectConfig{ + EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ + Network: mockNetwork.Name, + Subnets: []hivev1.GCPPrivateServiceConnectSubnet{{ + Subnet: mockSubnet.SelfLink, + Region: testRegion, + }}, + }}, + }, + gcpClientConfig: func(m *mockclient.MockClient) { + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().ListAddresses(gomock.Any(), gomock.Any()).Return(&compute.AddressList{ + Items: []*compute.Address{mockAddress}, + }, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetNetwork(gomock.Any()).Return(mockNetwork, nil) + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, newGoogleApiError(http.StatusNotFound, "NotFound", "Subnet not found")) + m.EXPECT().CreateSubnet(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledServiceAttachmentSubnet", + Message: "reconciled the Service Attachment Subnet", + }}, + expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + }, + }, { // There should be an error on ensureServiceAttachmentFirewall failure + name: "ensureServiceAttachmentFirewall failure", + cd: cdBuilder.Build( testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + }}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -668,15 +704,55 @@ func Test_Reconcile(t *testing.T) { Message: "googleapi: Error 401: not authorized to GetFirewall, AccessDenied", }}, expectError: "failed to reconcile the Service Attachment Firwall: googleapi: Error 401: not authorized to GetFirewall, AccessDenied", + }, { // Ready condition should be updated when firewallModified + name: "firewallModified", + cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + }}), + ), + config: &hivev1.GCPPrivateServiceConnectConfig{ + EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ + Network: mockNetwork.Name, + Subnets: []hivev1.GCPPrivateServiceConnectSubnet{{ + Subnet: mockSubnet.SelfLink, + Region: testRegion, + }}, + }}, + }, + gcpClientConfig: func(m *mockclient.MockClient) { + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().ListAddresses(gomock.Any(), gomock.Any()).Return(&compute.AddressList{ + Items: []*compute.Address{mockAddress}, + }, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetNetwork(gomock.Any()).Return(mockNetwork, nil) + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().GetFirewall(gomock.Any()).Return(nil, newGoogleApiError(http.StatusNotFound, "NotFound", "Firewall not found")) + m.EXPECT().CreateFirewall(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(mockFirewall, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledServiceAttachmentFirewall", + Message: "reconciled the Service Attachment Firewall", + }}, expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ - ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, }, }, { // There should be an error on ensureServiceAttachment failure name: "ensureServiceAttachment failure", cd: cdBuilder.Build( - testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), - testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + }}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -705,16 +781,60 @@ func Test_Reconcile(t *testing.T) { Message: "googleapi: Error 401: not authorized to GetServiceAttachment, AccessDenied", }}, expectError: "failed to reconcile the Service Attachment: googleapi: Error 401: not authorized to GetServiceAttachment, AccessDenied", + }, { // Ready condition should be updated when serviceAttachmentModified + name: "serviceAttachmentModified", + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + }}), + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + ), + config: &hivev1.GCPPrivateServiceConnectConfig{ + EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ + Network: mockNetwork.Name, + Subnets: []hivev1.GCPPrivateServiceConnectSubnet{{ + Subnet: mockSubnet.SelfLink, + Region: testRegion, + }}, + }}, + }, + gcpClientConfig: func(m *mockclient.MockClient) { + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().ListAddresses(gomock.Any(), gomock.Any()).Return(&compute.AddressList{ + Items: []*compute.Address{mockAddress}, + }, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetNetwork(gomock.Any()).Return(mockNetwork, nil) + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().GetFirewall(gomock.Any()).Return(mockFirewall, nil) + m.EXPECT().GetProjectName().Return(testProjectName) + m.EXPECT().GetServiceAttachment(gomock.Any(), gomock.Any()).Return(nil, newGoogleApiError(http.StatusNotFound, "NotFound", "ServiceAttachment not found")) + m.EXPECT().CreateServiceAttachment(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(mockServiceAttachment, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledServiceAttachment", + Message: "reconciled the Service Attachment", + }}, expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ ServiceAttachmentSubnet: mockSubnet.SelfLink, ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, }, }, { // There should be an error on ensureEndpointAddress failure name: "ensureEndpointAddress failure", cd: cdBuilder.Build( - testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), - testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, + }}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -744,17 +864,63 @@ func Test_Reconcile(t *testing.T) { Message: "googleapi: Error 401: not authorized to GetAddress, AccessDenied", }}, expectError: "failed to reconcile the Endpoint Address: googleapi: Error 401: not authorized to GetAddress, AccessDenied", + }, { // Ready condition should be updated when addressModified + name: "addressModified", + cd: cdBuilder.Build( + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, + }}), + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + ), + config: &hivev1.GCPPrivateServiceConnectConfig{ + EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ + Network: mockNetwork.Name, + Subnets: []hivev1.GCPPrivateServiceConnectSubnet{{ + Subnet: mockSubnet.SelfLink, + Region: testRegion, + }}, + }}, + }, + gcpClientConfig: func(m *mockclient.MockClient) { + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().ListAddresses(gomock.Any(), gomock.Any()).Return(&compute.AddressList{ + Items: []*compute.Address{mockAddress}, + }, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetNetwork(gomock.Any()).Return(mockNetwork, nil) + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().GetFirewall(gomock.Any()).Return(mockFirewall, nil) + m.EXPECT().GetServiceAttachment(gomock.Any(), gomock.Any()).Return(mockServiceAttachment, nil) + m.EXPECT().GetAddress(gomock.Any(), gomock.Any()).Return(nil, newGoogleApiError(http.StatusNotFound, "NotFound", "Address not found")) + m.EXPECT().CreateAddress(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockAddress, nil) + }, + expect: reconcile.Result{Requeue: true}, + expectConditions: []hivev1.ClusterDeploymentCondition{{ + Status: corev1.ConditionFalse, + Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, + Reason: "ReconciledEndpointAddress", + Message: "reconciled the Endpoint Address", + }}, expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ ServiceAttachmentSubnet: mockSubnet.SelfLink, ServiceAttachmentFirewall: mockFirewall.SelfLink, ServiceAttachment: mockServiceAttachment.SelfLink, + EndpointAddress: mockAddress.SelfLink, }, }, { // There should be an error on ensureEndpoint failure name: "ensureEndpoint failure", cd: cdBuilder.Build( - testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), - testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, + EndpointAddress: mockAddress.SelfLink, + }}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ @@ -785,17 +951,16 @@ func Test_Reconcile(t *testing.T) { Message: "googleapi: Error 401: not authorized to GetForwardingRule, AccessDenied", }}, expectError: "failed to reconcile the Endpoint: googleapi: Error 401: not authorized to GetForwardingRule, AccessDenied", - expectStatus: &hivev1gcp.PrivateServiceConnectStatus{ - ServiceAttachmentSubnet: mockSubnet.SelfLink, - ServiceAttachmentFirewall: mockFirewall.SelfLink, - ServiceAttachment: mockServiceAttachment.SelfLink, - EndpointAddress: mockAddress.SelfLink, - }, - }, { // An endpoint IP address should be returned on success - name: "success", + }, { // Ready condition should be updated when endpointModified + name: "endpointModified", cd: cdBuilder.Build( testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), - testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{}}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, + EndpointAddress: mockAddress.SelfLink, + }}), testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), ), config: &hivev1.GCPPrivateServiceConnectConfig{ @@ -818,8 +983,10 @@ func Test_Reconcile(t *testing.T) { m.EXPECT().GetFirewall(gomock.Any()).Return(mockFirewall, nil) m.EXPECT().GetServiceAttachment(gomock.Any(), gomock.Any()).Return(mockServiceAttachment, nil) m.EXPECT().GetAddress(gomock.Any(), gomock.Any()).Return(mockAddress, nil) - m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(nil, newGoogleApiError(http.StatusNotFound, "NotFound", "ForwardingRule not found")) + m.EXPECT().CreateForwardingRule(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) }, + expect: reconcile.Result{Requeue: true}, expectConditions: []hivev1.ClusterDeploymentCondition{{ Status: corev1.ConditionFalse, Type: hivev1.PrivateLinkReadyClusterDeploymentCondition, @@ -833,6 +1000,41 @@ func Test_Reconcile(t *testing.T) { EndpointAddress: mockAddress.SelfLink, Endpoint: mockForwardingRule.SelfLink, }, + }, { // An endpoint IP address should be returned on success + name: "success", + cd: cdBuilder.Build( + testcd.WithClusterMetadata(&hivev1.ClusterMetadata{InfraID: testInfraID}), + testcd.WithGCPPlatform(&hivev1gcp.Platform{Region: testRegion}), + testcd.WithGCPPlatformStatus(&hivev1gcp.PlatformStatus{PrivateServiceConnect: &hivev1gcp.PrivateServiceConnectStatus{ + ServiceAttachmentSubnet: mockSubnet.SelfLink, + ServiceAttachmentFirewall: mockFirewall.SelfLink, + ServiceAttachment: mockServiceAttachment.SelfLink, + EndpointAddress: mockAddress.SelfLink, + Endpoint: mockForwardingRule.SelfLink, + }}), + ), + config: &hivev1.GCPPrivateServiceConnectConfig{ + EndpointVPCInventory: []hivev1.GCPPrivateServiceConnectInventory{{ + Network: mockNetwork.Name, + Subnets: []hivev1.GCPPrivateServiceConnectSubnet{{ + Subnet: mockSubnet.SelfLink, + Region: testRegion, + }}, + }}, + }, + gcpClientConfig: func(m *mockclient.MockClient) { + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().ListAddresses(gomock.Any(), gomock.Any()).Return(&compute.AddressList{ + Items: []*compute.Address{mockAddress}, + }, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + m.EXPECT().GetNetwork(gomock.Any()).Return(mockNetwork, nil) + m.EXPECT().GetSubnet(gomock.Any(), gomock.Any(), gomock.Any()).Return(mockSubnet, nil) + m.EXPECT().GetFirewall(gomock.Any()).Return(mockFirewall, nil) + m.EXPECT().GetServiceAttachment(gomock.Any(), gomock.Any()).Return(mockServiceAttachment, nil) + m.EXPECT().GetAddress(gomock.Any(), gomock.Any()).Return(mockAddress, nil) + m.EXPECT().GetForwardingRule(gomock.Any(), gomock.Any()).Return(mockForwardingRule, nil) + }, expectRecord: &actuator.DnsRecord{IpAddress: []string{mockAddress.Address}}, }} @@ -863,10 +1065,9 @@ func Test_Reconcile(t *testing.T) { } assert.Equal(t, test.expectRecord, dnsRecord) - if test.expectStatus == nil { - test.expectStatus = &hivev1gcp.PrivateServiceConnectStatus{} + if test.expectStatus != nil { + assert.Equal(t, test.expectStatus, test.cd.Status.Platform.GCP.PrivateServiceConnect) } - assert.Equal(t, test.expectStatus, test.cd.Status.Platform.GCP.PrivateServiceConnect) curr := &hivev1.ClusterDeployment{} fakeClient := *gcpLinkActuator.client From ae0290eb7dc9a7239cb99ce6d9dfcc5765d67d11 Mon Sep 17 00:00:00 2001 From: Jeremiah Stuever Date: Mon, 16 Dec 2024 14:01:37 -0800 Subject: [PATCH 4/4] privatelink: selectHostedZoneVPC to return a pointer to the selected vpc --- .../actuator/awsactuator/awshubactuator.go | 18 ++++++++---------- .../awsactuator/awshubactuator_test.go | 10 ++++------ 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go index bc5beb99b2b..4e4f22b9c22 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator.go @@ -217,7 +217,7 @@ func (a *AWSHubActuator) ensureHostedZone(cd *hivev1.ClusterDeployment, metadata return false, "", err } - newHzID, err := a.createHostedZone(&selectedVPC, apiDomain) + newHzID, err := a.createHostedZone(selectedVPC, apiDomain) if err != nil { return false, "", err } @@ -709,9 +709,7 @@ func (a *AWSHubActuator) getEndpointVPC(cd *hivev1.ClusterDeployment, metadata * return endpointVPC, nil } -func (a *AWSHubActuator) selectHostedZoneVPC(cd *hivev1.ClusterDeployment, metadata *hivev1.ClusterMetadata, logger log.FieldLogger) (hivev1.AWSAssociatedVPC, error) { - selectedVPC := hivev1.AWSAssociatedVPC{} - +func (a *AWSHubActuator) selectHostedZoneVPC(cd *hivev1.ClusterDeployment, metadata *hivev1.ClusterMetadata, logger log.FieldLogger) (*hivev1.AWSAssociatedVPC, error) { // For clusterdeployments that are on AWS, use the VPCEndpoint VPC if cd.Status.Platform != nil && cd.Status.Platform.AWS != nil && @@ -720,29 +718,29 @@ func (a *AWSHubActuator) selectHostedZoneVPC(cd *hivev1.ClusterDeployment, metad endpointVPC, err := a.getEndpointVPC(cd, metadata) if err != nil { - return selectedVPC, errors.Wrap(err, "error getting Endpoint VPC") + return nil, errors.Wrap(err, "error getting Endpoint VPC") } if endpointVPC.VPCID == "" { - return selectedVPC, errors.New("unable to select Endpoint VPC: Endpoint not found") + return nil, errors.New("unable to select Endpoint VPC: Endpoint not found") } - return endpointVPC, nil + return &endpointVPC, nil } associatedVPCS, err := a.getAssociatedVPCs(cd, metadata, logger) if err != nil { - return selectedVPC, errors.Wrap(err, "error getting associated VPCs") + return nil, errors.Wrap(err, "error getting associated VPCs") } // Select the first associatedVPC that uses the primary AWS PrivateLink credential. // This is necessary because a Hosted Zone can only be created using a VPC owned by the same account. for _, associatedVPC := range associatedVPCS { if associatedVPC.CredentialsSecretRef == nil || *associatedVPC.CredentialsSecretRef == a.config.CredentialsSecretRef { - return associatedVPC, nil + return &associatedVPC, nil } } // No VPCs found that match the criteria, return an error. - return selectedVPC, errors.New("unable to find an associatedVPC that uses the primary AWS PrivateLink credentials") + return nil, errors.New("unable to find an associatedVPC that uses the primary AWS PrivateLink credentials") } diff --git a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go index 8a6a1adb265..7af6a9f4e73 100644 --- a/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go +++ b/pkg/controller/privatelink/actuator/awsactuator/awshubactuator_test.go @@ -1792,7 +1792,7 @@ func Test_selectHostedZoneVPC(t *testing.T) { AWSClientConfig func(*mock.MockClient) - expect hivev1.AWSAssociatedVPC + expect *hivev1.AWSAssociatedVPC expectError string }{{ // There should be an error if VPCEndpointID is set and getEndpointVPC fails name: "VPCEndpointID, getEndpointVPC failure", @@ -1808,7 +1808,6 @@ func Test_selectHostedZoneVPC(t *testing.T) { AWSClientConfig: func(m *mock.MockClient) { m.EXPECT().DescribeVpcEndpoints(gomock.Any()).Return(nil, awserr.New("AccessDenied", "not authorized to DescribeVpcEndpoints", nil)) }, - expect: hivev1.AWSAssociatedVPC{}, expectError: "error getting Endpoint VPC: error getting the VPC Endpoint: AccessDenied: not authorized to DescribeVpcEndpoints", }, { // There should be an error if VPCEndPointID is set and getEndpointVPC returns an empty VPCID name: "VPCEndpointID, getEndpointVPC return empty vpcid", @@ -1826,7 +1825,6 @@ func Test_selectHostedZoneVPC(t *testing.T) { VpcEndpoints: []*ec2.VpcEndpoint{{VpcId: aws.String("")}}, }, nil) }, - expect: hivev1.AWSAssociatedVPC{}, expectError: "unable to select Endpoint VPC: Endpoint not found", }, { // The AWS VPCEndpointID VPC should be used when set name: "VPCEndpointID, success", @@ -1844,7 +1842,7 @@ func Test_selectHostedZoneVPC(t *testing.T) { VpcEndpoints: []*ec2.VpcEndpoint{mockEndpoint}, }, nil) }, - expect: hivev1.AWSAssociatedVPC{ + expect: &hivev1.AWSAssociatedVPC{ AWSPrivateLinkVPC: hivev1.AWSPrivateLinkVPC{ VPCID: *mockEndpoint.VpcId, Region: testRegion, @@ -1863,7 +1861,7 @@ func Test_selectHostedZoneVPC(t *testing.T) { AWSPrivateLinkVPC: hivev1.AWSPrivateLinkVPC{VPCID: "vpc-2", Region: testRegion}, }}, }, - expect: hivev1.AWSAssociatedVPC{ + expect: &hivev1.AWSAssociatedVPC{ AWSPrivateLinkVPC: hivev1.AWSPrivateLinkVPC{ VPCID: "vpc-2", Region: testRegion, @@ -1881,7 +1879,7 @@ func Test_selectHostedZoneVPC(t *testing.T) { CredentialsSecretRef: &corev1.LocalObjectReference{Name: "credential-1"}, }}, }, - expect: hivev1.AWSAssociatedVPC{ + expect: &hivev1.AWSAssociatedVPC{ AWSPrivateLinkVPC: hivev1.AWSPrivateLinkVPC{ VPCID: "vpc-2", Region: testRegion,