diff --git a/pkg/controllers/multiclusterservice/controller.go b/pkg/controllers/multiclusterservice/controller.go index ecd9d1c1..e2caf672 100644 --- a/pkg/controllers/multiclusterservice/controller.go +++ b/pkg/controllers/multiclusterservice/controller.go @@ -122,7 +122,7 @@ func (r *Reconciler) handleDelete(ctx context.Context, mcs *fleetnetv1alpha1.Mul // delete derived service in the fleet-system namespace serviceName := r.derivedServiceFromLabel(mcs) - if err := r.deleteDerivedService(ctx, serviceName); err != nil { + if err := r.deleteDerivedService(ctx, serviceName, mcs); err != nil { klog.ErrorS(err, "Failed to remove derived service of mcs", "multiClusterService", mcsKObj) if !errors.IsNotFound(err) { return ctrl.Result{}, err @@ -146,17 +146,31 @@ func (r *Reconciler) handleDelete(ctx context.Context, mcs *fleetnetv1alpha1.Mul return ctrl.Result{}, nil } -func (r *Reconciler) deleteDerivedService(ctx context.Context, serviceName *types.NamespacedName) error { +func (r *Reconciler) deleteDerivedService(ctx context.Context, serviceName *types.NamespacedName, mcs *fleetnetv1alpha1.MultiClusterService) error { if serviceName == nil { return nil } - service := corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: serviceName.Namespace, - Name: serviceName.Name, - }, + + // Retrieve the service first. + derivedSvc := &corev1.Service{} + if err := r.Client.Get(ctx, *serviceName, derivedSvc); err != nil { + return fmt.Errorf("failed to get derived service: %w", err) + } + + // Note that here the controller only checks for the presence of the owner object namespace label as the two labels + // are always set together when the derived service is created/updated. + ownerMCSNamespace, foundOwnerNS := derivedSvc.GetLabels()[serviceLabelMCSNamespace] + ownerMCSName := derivedSvc.GetLabels()[serviceLabelMCSName] + if foundOwnerNS && (ownerMCSNamespace != mcs.Namespace || ownerMCSName != mcs.Name) { + // The derived service is owned by another MCS, which signals a name collision situation. No action needs + // to be taken on the linked derived service any more, as it is managed by a different MCS. + klog.V(2).InfoS("The derived service is owned by another MCS, no cleanup needed", + "multiClusterService", klog.KObj(mcs), + "derivedService", klog.KRef(serviceName.Namespace, serviceName.Name), + "ownerMCS", klog.KRef(ownerMCSNamespace, ownerMCSName)) + return nil } - return r.Client.Delete(ctx, &service) + return r.Client.Delete(ctx, derivedSvc) } func (r *Reconciler) deleteServiceImport(ctx context.Context, serviceImportName *types.NamespacedName) error { @@ -247,11 +261,59 @@ func (r *Reconciler) handleUpdate(ctx context.Context, mcs *fleetnetv1alpha1.Mul serviceName := r.derivedServiceFromLabel(mcs) if serviceName == nil { - serviceName = r.generateDerivedServiceName(mcs) - klog.V(4).InfoS("Generated derived service name", "multiClusterService", mcsKObj, "service", serviceName) + var err error + serviceName, err = r.uniqueDerivedServiceName(mcs) + if err != nil { + klog.ErrorS(err, "Failed to generate a unique derived service name for mcs", "multiClusterService", mcsKObj) + return ctrl.Result{}, fmt.Errorf("failed to generate a unique derived service name: %w", err) + } + klog.V(4).InfoS("Generated derived service name", "multiClusterService", mcsKObj, "derivedService", *serviceName) } // update mcs service label first to prevent the controller abort before we create the resource if err := r.updateMultiClusterLabel(ctx, mcs, objectmeta.MultiClusterServiceLabelDerivedService, serviceName.Name); err != nil { + klog.ErrorS(err, "Failed to update MCS with derived service name labels", "multiClusterService", mcsKObj, "derivedService", *serviceName) + return ctrl.Result{}, fmt.Errorf("failed to update MCS with derived service name labels: %w", err) + } + + // To address a name collision issue, the controller has been updated to generate derived service names differently, + // from the format [MCS-NAMESPACE]-[MCS-NAME] to [MCS-NAMESPACE]-[MCS-NAME]-[HASH-SUFFIX]. + // + // However, there might be existing MCS that were created before this change, which already had a derived service + // created in the old format. To ensure that such MCS continues to function with no interruption, here the controller + // performs an extra round of check: if the MCS has been linked with a derived service, we check if the derived service + // has been labeled with the namespace and name of the owner MCS; should the labels exist but do not match with that of the current + // MCS, we know that a name collision has occurred, and the MCS being reconciled will be assigned a new derived service + // using the new name format. Otherwise the MCS will continue to use the existing derived service. + res, err := r.verifyDerivedServiceOwnership(ctx, mcs, serviceName.Name) + if err != nil { + klog.ErrorS(err, "Failed to verify derived service ownership", "multiClusterService", mcsKObj, "derivedService", *serviceName) + return ctrl.Result{}, fmt.Errorf("failed to verify derived service ownership: %w", err) + } + switch res { + case derivedSvcOwnerVeriResNotFound: + // The derived service has not been created yet; no further action to take here, as the following + // createOrUpdate step will create the derived service. + case derivedSvcOwnerVeriResOrphaned: + // The derived service exists but is missing owner information; let the following createOrUpdate step to overwrite + // it with the correct owner information. This normally wouldn't happen. + klog.V(2).InfoS("Derived service has no owner information set", "multiClusterService", mcsKObj, "derivedService", *serviceName) + case derivedSvcOwnerVeriResOwnedByOthers: + // The derived service is owned by another MCS, which signals a name collision situation. Remove the current + // derived service name label and requeue the request to let the controller generate a new derived service name + // for the MCS being reconciled. + klog.V(2).InfoS("A name collision has been found; correct the situation by removing the current derived service name label and requeue", "multiClusterService", mcsKObj, "derivedService", *serviceName) + delete(mcs.GetLabels(), objectmeta.MultiClusterServiceLabelDerivedService) + if err := r.Client.Update(ctx, mcs); err != nil { + klog.ErrorS(err, "Failed to remove the derived service name label", "multiClusterService", mcsKObj, "derivedService", *serviceName) + return ctrl.Result{}, fmt.Errorf("failed to remove the derived service name label: %w", err) + } + return ctrl.Result{Requeue: true}, nil + case derivedSvcOwnerVeriResSelfOwned: + // The derived service is owned by the MCS being reconciled, which is the expected case; no further action needed. + default: + // An unexpected result is found. + err := fmt.Errorf("unexpected result when verifying derived service ownership: %s", res) + klog.ErrorS(err, "", "multiClusterService", mcsKObj, "derivedService", *serviceName, "result", res) return ctrl.Result{}, err } @@ -309,7 +371,7 @@ func (r *Reconciler) handleInvalidServiceImport(ctx context.Context, mcs *fleetn return nil // do nothing } svcKRef := klog.KRef(serviceName.Namespace, serviceName.Name) - if err := r.deleteDerivedService(ctx, serviceName); err != nil && !errors.IsNotFound(err) { + if err := r.deleteDerivedService(ctx, serviceName, mcs); err != nil && !errors.IsNotFound(err) { klog.ErrorS(err, "Failed to remove derived service of mcs", "multiClusterService", mcsKObj, "service", svcKRef) return err } @@ -352,7 +414,61 @@ func configureInternalLoadBalancer(mcs *fleetnetv1alpha1.MultiClusterService, se service.Annotations[serviceAnnotationInternalLoadBalancer] = "true" } +type derivedSvcOwnerVeriRes string + +const ( + derivedSvcOwnerVeriResNotFound derivedSvcOwnerVeriRes = "NotFound" + derivedSvcOwnerVeriResOrphaned derivedSvcOwnerVeriRes = "Orphaned" + derivedSvcOwnerVeriResOwnedByOthers derivedSvcOwnerVeriRes = "OwnedByOthers" + derivedSvcOwnerVeriResSelfOwned derivedSvcOwnerVeriRes = "SelfOwned" + derivedSvcOwnerVeriResUnknown derivedSvcOwnerVeriRes = "Unknown" +) + +func (r *Reconciler) verifyDerivedServiceOwnership( + ctx context.Context, mcs *fleetnetv1alpha1.MultiClusterService, derivedServiceName string) (derivedSvcOwnerVeriRes, error) { + derivedService := &corev1.Service{} + if err := r.Get(ctx, types.NamespacedName{Namespace: r.FleetSystemNamespace, Name: derivedServiceName}, derivedService); err != nil { + if errors.IsNotFound(err) { + return derivedSvcOwnerVeriResNotFound, nil + } + return derivedSvcOwnerVeriResUnknown, fmt.Errorf("failed to get derived service: %w", err) + } + + // Note that here the controller only checks for the presence of the owner object namespace label as the two labels + // are always set together when the derived service is created/updated. + ownerMCSNamespace, foundOwnerNS := derivedService.GetLabels()[serviceLabelMCSNamespace] + ownerMCSName := derivedService.GetLabels()[serviceLabelMCSName] + switch { + case !foundOwnerNS: + // The owner information is missing, this normally wouldn't happen, as the derived service is created in one go + // with the owner information set as labels. Still, the controller handles this by letting the following createOrUpdate + // step to overwrite the derived service with the correct owner information. + return derivedSvcOwnerVeriResOrphaned, nil + case ownerMCSNamespace != mcs.Namespace || ownerMCSName != mcs.Name: + // The derived service is owned by another MCS, which signals a name collision situation. Set the controller to re-gen + // a new derived service name for the MCS being reconciled. + return derivedSvcOwnerVeriResOwnedByOthers, nil + default: + // The derived service is owned by the MCS being reconciled, which is the expected case. + return derivedSvcOwnerVeriResSelfOwned, nil + } +} + func (r *Reconciler) ensureDerivedService(mcs *fleetnetv1alpha1.MultiClusterService, serviceImport *fleetnetv1alpha1.ServiceImport, service *corev1.Service) error { + // Verify the owner reference; throw an error if the controller is trying to update a derived service + // that is owned by another MCS. + // + // Note that here the controller only checks for the presence of the owner object namespace label as the two labels + // are always set together when the derived service is created/updated. + ownerMCSNamespace, foundOwnerNS := service.GetLabels()[serviceLabelMCSNamespace] + ownerMCSName := service.GetLabels()[serviceLabelMCSName] + if foundOwnerNS && (ownerMCSNamespace != mcs.Namespace || ownerMCSName != mcs.Name) { + // The derived service is owned by another MCS, which signals a name collision situation. Fail the createOrUpdate + // step now. + return fmt.Errorf("the derived service %s/%s is owned by another MCS %s/%s (expected %s/%s); there might be a name collision", + service.Namespace, service.Name, ownerMCSNamespace, ownerMCSName, mcs.Namespace, mcs.Name) + } + svcPorts := make([]corev1.ServicePort, len(serviceImport.Status.Ports)) for i, importPort := range serviceImport.Status.Ports { svcPorts[i] = importPort.ToServicePort() @@ -370,14 +486,6 @@ func (r *Reconciler) ensureDerivedService(mcs *fleetnetv1alpha1.MultiClusterServ return nil } -// generateDerivedServiceName appends multiclusterservice name and namespace as the derived service name since a service -// import may be exported by the multiple MCSs. -// It makes sure the service name is unique and less than 63 characters. -func (r *Reconciler) generateDerivedServiceName(mcs *fleetnetv1alpha1.MultiClusterService) *types.NamespacedName { - // TODO make sure the service name is unique and less than 63 characters. - return &types.NamespacedName{Namespace: r.FleetSystemNamespace, Name: fmt.Sprintf("%v-%v", mcs.Namespace, mcs.Name)} -} - // updateMultiClusterServiceStatus updates mcs condition and status based on the service import and service status. func (r *Reconciler) updateMultiClusterServiceStatus(ctx context.Context, mcs *fleetnetv1alpha1.MultiClusterService, serviceImport *fleetnetv1alpha1.ServiceImport, service *corev1.Service) error { currentCond := meta.FindStatusCondition(mcs.Status.Conditions, string(fleetnetv1alpha1.MultiClusterServiceValid)) diff --git a/pkg/controllers/multiclusterservice/controller_integration_test.go b/pkg/controllers/multiclusterservice/controller_integration_test.go index 1ac8205a..c31e90ab 100644 --- a/pkg/controllers/multiclusterservice/controller_integration_test.go +++ b/pkg/controllers/multiclusterservice/controller_integration_test.go @@ -338,3 +338,364 @@ var _ = Describe("Test MultiClusterService Controller", func() { }) }) }) + +var _ = Describe("Name collision fix", func() { + const ( + timeout = time.Second * 10 + duration = time.Second * 5 + interval = time.Millisecond * 250 + ) + + Context("New MCS with no derived service", Ordered, func() { + var ( + mcs *fleetnetv1alpha1.MultiClusterService + mcsLookupKey types.NamespacedName + serviceImportKey types.NamespacedName + derivedServiceName string + ) + + BeforeAll(func() { + By("By creating a new MultiClusterService") + mcs = multiClusterServiceForTest() + Expect(k8sClient.Create(ctx, mcs)).Should(Succeed()) + + mcsLookupKey = types.NamespacedName{Name: testName, Namespace: testNamespace} + serviceImportKey = types.NamespacedName{Name: testServiceName, Namespace: testNamespace} + }) + + It("Should populate the service import status", func() { + By("By waiting for the service import to be created") + serviceImport := &fleetnetv1alpha1.ServiceImport{} + Eventually(func() error { + return k8sClient.Get(ctx, serviceImportKey, serviceImport) + }, timeout, interval).Should(Succeed()) + + By("By updating the service import status") + serviceImport.Status = fleetnetv1alpha1.ServiceImportStatus{ + Type: fleetnetv1alpha1.ClusterSetIP, + Clusters: []fleetnetv1alpha1.ClusterStatus{ + {Cluster: "member1"}, + {Cluster: "member2"}, + }, + Ports: []fleetnetv1alpha1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + } + Expect(k8sClient.Status().Update(ctx, serviceImport)).Should(Succeed()) + }) + + It("Should add the derived service label with the expected name", func() { + createdMultiClusterService := &fleetnetv1alpha1.MultiClusterService{} + Eventually(func() error { + if err := k8sClient.Get(ctx, mcsLookupKey, createdMultiClusterService); err != nil { + return err + } + got, ok := createdMultiClusterService.GetLabels()[objectmeta.MultiClusterServiceLabelDerivedService] + if !ok { + return fmt.Errorf("derived service label is not set") + } + want, err := multiClusterServiceReconciler(k8sClient).uniqueDerivedServiceName(createdMultiClusterService) + if err != nil { + return err + } + if got != want.Name { + return fmt.Errorf("derived service label = %s, want %s", got, want.Name) + } + derivedServiceName = got + return nil + }, timeout, interval).Should(Succeed()) + }) + + It("Should create the derived service", func() { + derivedServiceLookupKey := types.NamespacedName{Name: derivedServiceName, Namespace: systemNamespace} + createdService := &corev1.Service{} + Eventually(func() error { + return k8sClient.Get(ctx, derivedServiceLookupKey, createdService) + }, timeout, interval).Should(Succeed()) + }) + + AfterAll(func() { + By("By deleting the MultiClusterService") + Expect(k8sClient.Delete(ctx, mcs)).Should(Succeed()) + + By("By checking the MultiClusterService is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, mcsLookupKey, &fleetnetv1alpha1.MultiClusterService{})) + }, timeout, interval).Should(BeTrue()) + }) + }) + + Context("Existing MCS with a derived service in the old name format", Ordered, func() { + var ( + mcs *fleetnetv1alpha1.MultiClusterService + mcsLookupKey types.NamespacedName + serviceImportKey types.NamespacedName + derivedServiceKey types.NamespacedName + derivedServiceUID types.UID + ) + + BeforeAll(func() { + mcsLookupKey = types.NamespacedName{Name: testName, Namespace: testNamespace} + serviceImportKey = types.NamespacedName{Name: testServiceName, Namespace: testNamespace} + derivedServiceKey = types.NamespacedName{Name: derivedServiceName, Namespace: systemNamespace} + + By("By creating a derived service using the old name format") + derivedService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: derivedServiceName, + Namespace: systemNamespace, + Labels: map[string]string{ + serviceLabelMCSName: testName, + serviceLabelMCSNamespace: testNamespace, + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, derivedService)).Should(Succeed()) + Expect(k8sClient.Get(ctx, derivedServiceKey, derivedService)).Should(Succeed()) + derivedServiceUID = derivedService.UID + + By("By creating a service import with a populated status") + serviceImport := &fleetnetv1alpha1.ServiceImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceName, + Namespace: testNamespace, + }, + } + Expect(k8sClient.Create(ctx, serviceImport)).Should(Succeed()) + serviceImport.Status = fleetnetv1alpha1.ServiceImportStatus{ + Type: fleetnetv1alpha1.ClusterSetIP, + Clusters: []fleetnetv1alpha1.ClusterStatus{ + {Cluster: "member1"}, + }, + Ports: []fleetnetv1alpha1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + } + Expect(k8sClient.Status().Update(ctx, serviceImport)).Should(Succeed()) + + By("By creating a MultiClusterService already linked to the old derived service") + mcs = multiClusterServiceForTest() + mcs.Labels = map[string]string{ + multiClusterServiceLabelServiceImport: testServiceName, + objectmeta.MultiClusterServiceLabelDerivedService: derivedServiceName, + } + Expect(k8sClient.Create(ctx, mcs)).Should(Succeed()) + }) + + It("Should not take any action on the existing derived service", func() { + Consistently(func() error { + service := &corev1.Service{} + if err := k8sClient.Get(ctx, derivedServiceKey, service); err != nil { + return err + } + if service.UID != derivedServiceUID { + return fmt.Errorf("derived service UID = %s, want %s; the derived service was recreated", service.UID, derivedServiceUID) + } + mcsObj := &fleetnetv1alpha1.MultiClusterService{} + if err := k8sClient.Get(ctx, mcsLookupKey, mcsObj); err != nil { + return err + } + if got := mcsObj.GetLabels()[objectmeta.MultiClusterServiceLabelDerivedService]; got != derivedServiceName { + return fmt.Errorf("derived service label = %s, want %s", got, derivedServiceName) + } + serviceList := &corev1.ServiceList{} + if err := k8sClient.List(ctx, serviceList, &client.ListOptions{Namespace: systemNamespace}); err != nil { + return err + } + if len(serviceList.Items) != 1 { + return fmt.Errorf("derived service count = %d, want 1; a new derived service may have been created", len(serviceList.Items)) + } + return nil + }, duration, interval).Should(Succeed()) + }) + + AfterAll(func() { + By("By deleting the MultiClusterService") + Expect(k8sClient.Delete(ctx, mcs)).Should(Succeed()) + + By("By checking the MultiClusterService is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, mcsLookupKey, &fleetnetv1alpha1.MultiClusterService{})) + }, timeout, interval).Should(BeTrue()) + + By("By checking the derived service is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, derivedServiceKey, &corev1.Service{})) + }, timeout, interval).Should(BeTrue()) + + By("By checking the service import is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, serviceImportKey, &fleetnetv1alpha1.ServiceImport{})) + }, timeout, interval).Should(BeTrue()) + }) + }) + + Context("Existing MCS whose derived service is owned by another MCS", Ordered, func() { + var ( + mcs *fleetnetv1alpha1.MultiClusterService + mcsLookupKey types.NamespacedName + serviceImportKey types.NamespacedName + originalServiceKey types.NamespacedName + originalServiceUID types.UID + newDerivedServiceName string + ) + + BeforeAll(func() { + mcsLookupKey = types.NamespacedName{Name: testName, Namespace: testNamespace} + serviceImportKey = types.NamespacedName{Name: testServiceName, Namespace: testNamespace} + originalServiceKey = types.NamespacedName{Name: derivedServiceName, Namespace: systemNamespace} + + By("By creating a derived service (old name format) owned by another MCS") + originalService := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: derivedServiceName, + Namespace: systemNamespace, + Labels: map[string]string{ + serviceLabelMCSName: "another-mcs", + serviceLabelMCSNamespace: "another-ns", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, originalService)).Should(Succeed()) + Expect(k8sClient.Get(ctx, originalServiceKey, originalService)).Should(Succeed()) + originalServiceUID = originalService.UID + + By("By creating a service import with a populated status") + serviceImport := &fleetnetv1alpha1.ServiceImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: testServiceName, + Namespace: testNamespace, + }, + } + Expect(k8sClient.Create(ctx, serviceImport)).Should(Succeed()) + serviceImport.Status = fleetnetv1alpha1.ServiceImportStatus{ + Type: fleetnetv1alpha1.ClusterSetIP, + Clusters: []fleetnetv1alpha1.ClusterStatus{ + {Cluster: "member1"}, + }, + Ports: []fleetnetv1alpha1.ServicePort{ + { + Name: "http", + Port: 8080, + Protocol: corev1.ProtocolTCP, + }, + }, + } + Expect(k8sClient.Status().Update(ctx, serviceImport)).Should(Succeed()) + + By("By creating a MultiClusterService linked to the derived service owned by another MCS") + mcs = multiClusterServiceForTest() + mcs.Labels = map[string]string{ + multiClusterServiceLabelServiceImport: testServiceName, + objectmeta.MultiClusterServiceLabelDerivedService: derivedServiceName, + } + Expect(k8sClient.Create(ctx, mcs)).Should(Succeed()) + }) + + It("Should assign the MCS a new derived service name", func() { + createdMultiClusterService := &fleetnetv1alpha1.MultiClusterService{} + Eventually(func() error { + if err := k8sClient.Get(ctx, mcsLookupKey, createdMultiClusterService); err != nil { + return err + } + got, ok := createdMultiClusterService.GetLabels()[objectmeta.MultiClusterServiceLabelDerivedService] + if !ok { + return fmt.Errorf("derived service label is not set") + } + if got == derivedServiceName { + return fmt.Errorf("derived service label = %s, want a new name different from the colliding one", got) + } + want, err := multiClusterServiceReconciler(k8sClient).uniqueDerivedServiceName(createdMultiClusterService) + if err != nil { + return err + } + if got != want.Name { + return fmt.Errorf("derived service label = %s, want %s", got, want.Name) + } + newDerivedServiceName = got + return nil + }, timeout, interval).Should(Succeed()) + }) + + It("Should create the new derived service", func() { + newServiceKey := types.NamespacedName{Name: newDerivedServiceName, Namespace: systemNamespace} + createdService := &corev1.Service{} + Eventually(func() error { + return k8sClient.Get(ctx, newServiceKey, createdService) + }, timeout, interval).Should(Succeed()) + }) + + It("Should not take any action on the original derived service", func() { + Consistently(func() error { + service := &corev1.Service{} + if err := k8sClient.Get(ctx, originalServiceKey, service); err != nil { + return err + } + if service.UID != originalServiceUID { + return fmt.Errorf("original derived service UID = %s, want %s; the original service was recreated", service.UID, originalServiceUID) + } + if got := service.GetLabels()[serviceLabelMCSName]; got != "another-mcs" { + return fmt.Errorf("original derived service owner name label = %s, want another-mcs", got) + } + if got := service.GetLabels()[serviceLabelMCSNamespace]; got != "another-ns" { + return fmt.Errorf("original derived service owner namespace label = %s, want another-ns", got) + } + return nil + }, duration, interval).Should(Succeed()) + }) + + AfterAll(func() { + By("By deleting the MultiClusterService") + Expect(k8sClient.Delete(ctx, mcs)).Should(Succeed()) + + By("By checking the MultiClusterService is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, mcsLookupKey, &fleetnetv1alpha1.MultiClusterService{})) + }, timeout, interval).Should(BeTrue()) + + By("By checking the service import is deleted") + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, serviceImportKey, &fleetnetv1alpha1.ServiceImport{})) + }, timeout, interval).Should(BeTrue()) + + By("By deleting the original derived service") + Expect(k8sClient.Delete(ctx, &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: derivedServiceName, + Namespace: systemNamespace, + }, + })).Should(Succeed()) + Eventually(func() bool { + return errors.IsNotFound(k8sClient.Get(ctx, originalServiceKey, &corev1.Service{})) + }, timeout, interval).Should(BeTrue()) + }) + }) +}) diff --git a/pkg/controllers/multiclusterservice/controller_test.go b/pkg/controllers/multiclusterservice/controller_test.go index 35251759..99659697 100644 --- a/pkg/controllers/multiclusterservice/controller_test.go +++ b/pkg/controllers/multiclusterservice/controller_test.go @@ -286,18 +286,26 @@ func TestHandleUpdate(t *testing.T) { serviceLabelMCSNamespace: testNamespace, } + // generatedDerivedServiceName is the hash-suffixed name assigned when the mcs has no derived service label yet. + generatedDerivedService, err := (&Reconciler{FleetSystemNamespace: systemNamespace}).uniqueDerivedServiceName(multiClusterServiceForTest()) + if err != nil { + t.Fatalf("failed to generate derived service name: %v", err) + } + generatedDerivedServiceName := generatedDerivedService.Name + tests := []struct { - name string - labels map[string]string - annotations map[string]string - status *fleetnetv1alpha1.MultiClusterServiceStatus - serviceImport *fleetnetv1alpha1.ServiceImport - hasOldServiceImport bool - service *corev1.Service - want ctrl.Result - wantServiceImport *fleetnetv1alpha1.ServiceImport - wantDerivedService *corev1.Service - wantMCS *fleetnetv1alpha1.MultiClusterService + name string + labels map[string]string + annotations map[string]string + status *fleetnetv1alpha1.MultiClusterServiceStatus + serviceImport *fleetnetv1alpha1.ServiceImport + hasOldServiceImport bool + service *corev1.Service + want ctrl.Result + wantServiceImport *fleetnetv1alpha1.ServiceImport + wantDerivedService *corev1.Service + wantDerivedServiceName string + wantMCS *fleetnetv1alpha1.MultiClusterService }{ { name: "no service import and its label", // mcs is just created @@ -626,9 +634,10 @@ func TestHandleUpdate(t *testing.T) { }, }, }, + wantDerivedServiceName: generatedDerivedServiceName, wantDerivedService: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ - Name: derivedServiceName, + Name: generatedDerivedServiceName, Namespace: systemNamespace, Labels: serviceLabel, }, @@ -644,7 +653,7 @@ func TestHandleUpdate(t *testing.T) { Namespace: testNamespace, Labels: map[string]string{ multiClusterServiceLabelServiceImport: testServiceName, - objectmeta.MultiClusterServiceLabelDerivedService: derivedServiceName, + objectmeta.MultiClusterServiceLabelDerivedService: generatedDerivedServiceName, }, }, Spec: fleetnetv1alpha1.MultiClusterServiceSpec{ @@ -1023,7 +1032,11 @@ func TestHandleUpdate(t *testing.T) { } service := corev1.Service{} - name = types.NamespacedName{Namespace: systemNamespace, Name: derivedServiceName} + wantSvcName := tc.wantDerivedServiceName + if wantSvcName == "" { + wantSvcName = derivedServiceName + } + name = types.NamespacedName{Namespace: systemNamespace, Name: wantSvcName} if err := fakeClient.Get(ctx, name, &service); err != nil { if tc.wantDerivedService != nil || !errors.IsNotFound(err) { t.Fatalf("ServiceImport Get() got error %v, want no error", err) diff --git a/pkg/controllers/multiclusterservice/uniquename.go b/pkg/controllers/multiclusterservice/uniquename.go new file mode 100644 index 00000000..40d3cd4b --- /dev/null +++ b/pkg/controllers/multiclusterservice/uniquename.go @@ -0,0 +1,90 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package multiclusterservice + +import ( + "crypto/sha256" + "encoding/json" + "fmt" + "strings" + + "k8s.io/apimachinery/pkg/types" + + fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" +) + +type derivedServiceNameHasherInput struct { + // Note: This struct use exported fields as Golang's JSON marshaller ignores unexported fields. + MCSNamespace string + MCSName string + MCSUID string +} + +func (r *Reconciler) uniqueDerivedServiceName(mcs *fleetnetv1alpha1.MultiClusterService) (*types.NamespacedName, error) { + // The name of a derived service is formatted as [MCS-NAMESPACE]-[MCS-NAME]-[HASH-SUFFIX]. + // + // We might truncate the namespace and name segments to ensure that the total length of the derived service + // name does not exceed 63 characters, which is the maximum length for a Kubernetes service name. + // + // A hash suffix is added as the format [MCS-NAMESPACE]-[MCS-NAME] may lead to name collisions, either due to + // unexpected dashes in the namespace/name segment, or due to truncation complications. + // + // For example, if one MCS has the namespace "team-a" and name "service", and another MCS has the namespace "team" and + // name "a-service", both would result in the derived service name "team-a-service". + + // Calculate the hash suffix. + hasherInput := derivedServiceNameHasherInput{ + MCSNamespace: mcs.Namespace, + MCSName: mcs.Name, + MCSUID: string(mcs.UID), + } + hash, err := hashOf(hasherInput) + if err != nil { + return nil, fmt.Errorf("failed to calculate hash: %w", err) + } + // Use the first 12 characters of the hash as a suffix. + // + // Note (chenyu1): a 12 char hash suffix might not be able to fully eliminate collisions, though the chances are extremely low. + // Should a collision still occur, manual intervention is needed to correct the situation. + hashSuffix := hash[:12] + + // Truncate the namespace and name segments if needed. + + // The available length of the namespace and name segments (49) is the maximum service name length (63) + // minus the length of the hash suffix (12) and two dashes (2). + // + // Each segment then has a maximum length of 24, which is the available length (49) divided by two, rounded down. + nameSegMaxLen := 24 + mcsNamespace := mcs.Namespace + mcsName := mcs.Name + + // Remove all dots from the namespace and name segments, and prefix the namespace with "ns-" if it + // starts with a numeric character, so that the derived service name remains a valid Kubernetes name. + mcsNamespace = strings.ReplaceAll(mcsNamespace, ".", "") + mcsName = strings.ReplaceAll(mcsName, ".", "") + if len(mcsNamespace) > 0 && mcsNamespace[0] >= '0' && mcsNamespace[0] <= '9' { + mcsNamespace = "ns-" + mcsNamespace + } + + if len(mcsNamespace) > nameSegMaxLen { + mcsNamespace = mcsNamespace[:nameSegMaxLen] + } + if len(mcsName) > nameSegMaxLen { + mcsName = mcsName[:nameSegMaxLen] + } + + serviceName := fmt.Sprintf("%s-%s-%s", mcsNamespace, mcsName, hashSuffix) + + return &types.NamespacedName{Namespace: r.FleetSystemNamespace, Name: serviceName}, nil +} + +func hashOf(input derivedServiceNameHasherInput) (string, error) { + jsonBytes, err := json.Marshal(input) + if err != nil { + return "", fmt.Errorf("failed to marshal object into JSON: %w", err) + } + return fmt.Sprintf("%x", sha256.Sum256(jsonBytes)), nil +} diff --git a/pkg/controllers/multiclusterservice/uniquename_test.go b/pkg/controllers/multiclusterservice/uniquename_test.go new file mode 100644 index 00000000..002ebfde --- /dev/null +++ b/pkg/controllers/multiclusterservice/uniquename_test.go @@ -0,0 +1,116 @@ +/* +Copyright (c) Microsoft Corporation. +Licensed under the MIT license. +*/ + +package multiclusterservice + +import ( + "regexp" + "strings" + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + fleetnetv1alpha1 "go.goms.io/fleet-networking/api/v1alpha1" +) + +func TestUniqueDerivedServiceName(t *testing.T) { + tests := []struct { + name string + namespace string + mcsName string + uid string + wantPrefix string + }{ + { + name: "no trimming needed", + namespace: "mynamespace", + mcsName: "myservice", + uid: "11111111-1111-1111-1111-111111111111", + wantPrefix: "mynamespace-myservice", + }, + { + name: "trims namespace and name to their quotas when too long", + namespace: strings.Repeat("a", 63), + mcsName: strings.Repeat("b", 63), + uid: "22222222-2222-2222-2222-222222222222", + wantPrefix: strings.Repeat("a", 24) + "-" + strings.Repeat("b", 24), + }, + { + name: "boundary fits exactly", + namespace: strings.Repeat("a", 24), + mcsName: strings.Repeat("b", 24), + uid: "33333333-3333-3333-3333-333333333333", + wantPrefix: strings.Repeat("a", 24) + "-" + strings.Repeat("b", 24), + }, + { + name: "per-field quotas apply even when total overflow is odd", + namespace: strings.Repeat("a", 30), + mcsName: strings.Repeat("b", 30), + uid: "44444444-4444-4444-4444-444444444444", + wantPrefix: strings.Repeat("a", 24) + "-" + strings.Repeat("b", 24), + }, + { + name: "long name is trimmed to its own quota", + namespace: "shortns", + mcsName: strings.Repeat("b", 60), + uid: "55555555-5555-5555-5555-555555555555", + wantPrefix: "shortns-" + strings.Repeat("b", 24), + }, + { + name: "removes dots from namespace and name", + namespace: "my.name.space", + mcsName: "my.service", + uid: "66666666-6666-6666-6666-666666666666", + wantPrefix: "mynamespace-myservice", + }, + { + name: "prefixes numeric-leading namespace with ns-", + namespace: "0namespace", + mcsName: "myservice", + uid: "77777777-7777-7777-7777-777777777777", + wantPrefix: "ns-0namespace-myservice", + }, + { + name: "prefixes numeric-leading namespace after removing dots", + namespace: "9.namespace", + mcsName: "myservice", + uid: "88888888-8888-8888-8888-888888888888", + wantPrefix: "ns-9namespace-myservice", + }, + { + name: "trims numeric-leading namespace to its quota after prefixing", + namespace: "1" + strings.Repeat("a", 62), + mcsName: "myservice", + uid: "99999999-9999-9999-9999-999999999999", + wantPrefix: ("ns-1" + strings.Repeat("a", 62))[:24] + "-myservice", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + r := &Reconciler{FleetSystemNamespace: "fleet-system"} + mcs := &fleetnetv1alpha1.MultiClusterService{ObjectMeta: metav1.ObjectMeta{Namespace: tc.namespace, Name: tc.mcsName, UID: types.UID(tc.uid)}} + + got, err := r.uniqueDerivedServiceName(mcs) + if err != nil { + t.Fatalf("uniqueDerivedServiceName() error = %v", err) + } + + if got.Namespace != r.FleetSystemNamespace { + t.Fatalf("result namespace = %q, want %q", got.Namespace, r.FleetSystemNamespace) + } + + wantPattern := "^" + regexp.QuoteMeta(tc.wantPrefix) + "-[0-9a-f]{12}$" + matched, err := regexp.MatchString(wantPattern, got.Name) + if err != nil { + t.Fatalf("failed to match pattern %q: %v", wantPattern, err) + } + if !matched { + t.Fatalf("uniqueDerivedServiceName() = %q, want match %q", got.Name, wantPattern) + } + }) + } +}