From 18b34c1d565760532d1c144ec6f40b165b4ea81a Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 25 Sep 2026 15:35:29 -0500 Subject: [PATCH] feat: bound workload deployment names to 41 characters Workload deployment names joined the workload, placement and location names, so a long enough combination passed 63 characters and the objects that run the workload were rejected. Two workloads could also produce the same name. Key changes: - Name deployments with the first 30 characters of the workload name and a 10-character hash of the workload UID, placement and location, so length never depends on placement or location and names never collide - Orphan every deployment of the workload that does not carry a desired name, which replaces deployments created under the old naming - Refuse to update a deployment that belongs to another workload - Require a DNS-1123 label workload name on create; updates are unaffected --- internal/controller/workload_controller.go | 90 ++++++------ .../controller/workload_controller_test.go | 135 +++++++++++++++++- internal/naming/naming.go | 37 +++++ internal/naming/naming_test.go | 62 ++++++++ internal/validation/workload_validation.go | 18 ++- .../validation/workload_validation_test.go | 65 +++++++++ 6 files changed, 365 insertions(+), 42 deletions(-) create mode 100644 internal/naming/naming.go create mode 100644 internal/naming/naming_test.go diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 1bcfc266..8a0f87f4 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -33,6 +33,7 @@ import ( computev1alpha "go.datum.net/compute/api/v1alpha" "go.datum.net/compute/internal/features" "go.datum.net/compute/internal/locations" + "go.datum.net/compute/internal/naming" "go.datum.net/compute/pkg/runtimeclass" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" @@ -208,30 +209,7 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ for _, desiredDeployment := range desired { logger.Info("ensuring workload deployment", "deployment_name", desiredDeployment.Name) - deployment := &computev1alpha.WorkloadDeployment{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: desiredDeployment.Namespace, - Name: desiredDeployment.Name, - }, - } - - _, err := controllerutil.CreateOrUpdate(ctx, cl.GetClient(), deployment, func() error { - if deployment.CreationTimestamp.IsZero() { - logger.Info("creating deployment", "deployment_name", deployment.Name) - if err := controllerutil.SetControllerReference(&workload, deployment, cl.GetScheme()); err != nil { - return fmt.Errorf("failed to set controller on workload deployment: %w", err) - } - } else { - logger.Info("updating deployment", "deployment_name", deployment.Name) - } - - mergeDeploymentMetadata(deployment, &desiredDeployment) - - // TODO(jreese) consider how this plays well with autoscaling - deployment.Spec = desiredDeployment.Spec - return nil - }) - + deployment, err := upsertWorkloadDeployment(ctx, cl.GetClient(), &workload, &desiredDeployment) if err != nil { return ctrl.Result{}, fmt.Errorf("failed mutating workload deployment: %w", err) } @@ -245,6 +223,50 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req mcreconcile.Requ return ctrl.Result{}, r.reconcileWorkloadStatus(ctx, cl.GetClient(), &workload, placementDeployments) } +// upsertWorkloadDeployment creates or updates the deployment named by desired. It +// refuses to update a deployment that belongs to a different workload. +func upsertWorkloadDeployment( + ctx context.Context, + upstreamClient client.Client, + workload *computev1alpha.Workload, + desired *computev1alpha.WorkloadDeployment, +) (*computev1alpha.WorkloadDeployment, error) { + logger := log.FromContext(ctx) + + deployment := &computev1alpha.WorkloadDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: desired.Namespace, + Name: desired.Name, + }, + } + + _, err := controllerutil.CreateOrUpdate(ctx, upstreamClient, deployment, func() error { + if owner := deployment.Spec.WorkloadRef.UID; owner != "" && owner != workload.UID { + return fmt.Errorf("workload deployment %q belongs to workload UID %q, not %q", + deployment.Name, owner, workload.UID) + } + + if deployment.CreationTimestamp.IsZero() { + logger.Info("creating deployment", "deployment_name", deployment.Name) + if err := controllerutil.SetControllerReference(workload, deployment, upstreamClient.Scheme()); err != nil { + return fmt.Errorf("failed to set controller on workload deployment: %w", err) + } + } else { + logger.Info("updating deployment", "deployment_name", deployment.Name) + } + + mergeDeploymentMetadata(deployment, desired) + + // TODO(jreese) consider how this plays well with autoscaling + deployment.Spec = desired.Spec + return nil + }) + if err != nil { + return nil, err + } + return deployment, nil +} + func (r *WorkloadReconciler) reconcileWorkloadStatus( ctx context.Context, upstreamClient client.Client, @@ -521,13 +543,8 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload( return nil, nil, err } - existingDeployments := sets.Set[string]{} desiredDeployments := sets.Set[string]{} - for _, deployment := range deployments.Items { - existingDeployments.Insert(deployment.Name) - } - placementLocations, err := locations.ListPlacementLocations(ctx, upstreamClient, r.LocationSource) if err != nil { return nil, nil, err @@ -554,11 +571,7 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload( } for _, locationName := range locationNames { - // TODO(jreese) should we use GenerateName for deployments and identify - // them via labels instead? Would help with race conditions on workload - // recreation. - - deploymentName := fmt.Sprintf("%s-%s-%s", workload.Name, placement.Name, strings.ToLower(locationName)) + deploymentName := naming.DeploymentName(workload.Name, workload.UID, placement.Name, locationName) desiredDeployments.Insert(deploymentName) desired = append(desired, computev1alpha.WorkloadDeployment{ @@ -588,12 +601,9 @@ func (r *WorkloadReconciler) getDeploymentsForWorkload( } } - // Collect orphans - for _, name := range existingDeployments.Difference(desiredDeployments).UnsortedList() { - for _, deployment := range deployments.Items { - if name == deployment.Name { - orphaned = append(orphaned, deployment) - } + for _, deployment := range deployments.Items { + if !desiredDeployments.Has(deployment.Name) { + orphaned = append(orphaned, deployment) } } diff --git a/internal/controller/workload_controller_test.go b/internal/controller/workload_controller_test.go index 413a8f7d..87e8f32d 100644 --- a/internal/controller/workload_controller_test.go +++ b/internal/controller/workload_controller_test.go @@ -17,6 +17,7 @@ import ( computev1alpha "go.datum.net/compute/api/v1alpha" "go.datum.net/compute/internal/locations" + "go.datum.net/compute/internal/naming" networkingv1alpha "go.datum.net/network-services-operator/api/v1alpha" locationsv1alpha1 "go.miloapis.com/locations/api/v1alpha1" servicesv1alpha1 "go.miloapis.com/service-catalog/api/v1alpha1" @@ -408,7 +409,7 @@ func TestGetDeploymentsForWorkload_LocationSelector(t *testing.T) { require.Len(t, desired, 2) assert.Equal(t, "dfw-a", desired[0].Spec.LocationRef.Name) assert.Equal(t, "dfw-b", desired[1].Spec.LocationRef.Name) - assert.Equal(t, rdTestWorkloadName+"-"+testDefaultPlacement+"-dfw-a", desired[0].Name) + assert.Equal(t, naming.DeploymentName(rdTestWorkloadName, "workload-uid", testDefaultPlacement, "dfw-a"), desired[0].Name) assert.Equal(t, "dfw-a", desired[0].Labels[computev1alpha.LocationLabel]) } @@ -566,3 +567,135 @@ func TestGetDeploymentsForWorkload_RequiresComputeAvailability(t *testing.T) { assert.Equal(t, testLocationName, deployment.Spec.LocationRef.Name) } } + +func newDeploymentMatchingTestWorkload() *computev1alpha.Workload { + return &computev1alpha.Workload{ + ObjectMeta: metav1.ObjectMeta{ + Name: rdTestWorkloadName, + Namespace: testDefaultNamespace, + UID: types.UID("workload-uid"), + }, + Spec: computev1alpha.WorkloadSpec{ + Placements: []computev1alpha.WorkloadPlacement{{ + Name: testDefaultPlacement, + Locations: []locationsv1alpha1.LocationReference{{Name: testLocationName}}, + ScaleSettings: computev1alpha.HorizontalScaleSettings{MinReplicas: 1}, + }}, + }, + } +} + +func newExistingDeployment(name string, workloadUID types.UID, placement string) *computev1alpha.WorkloadDeployment { + return &computev1alpha.WorkloadDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: testDefaultNamespace, + Labels: map[string]string{computev1alpha.WorkloadUIDLabel: string(workloadUID)}, + }, + Spec: computev1alpha.WorkloadDeploymentSpec{ + WorkloadRef: computev1alpha.WorkloadReference{Name: rdTestWorkloadName, UID: workloadUID}, + PlacementName: placement, + LocationRef: locationsv1alpha1.LocationReference{Name: testLocationName}, + }, + } +} + +// TestGetDeploymentsForWorkload_OrphansOtherNames verifies that only the +// deployment carrying the expected name is kept for a placement and location. +// A deployment of the same workload under any other name, such as one created +// before the naming change, is orphaned and replaced. +func TestGetDeploymentsForWorkload_OrphansOtherNames(t *testing.T) { + t.Parallel() + + workload := newDeploymentMatchingTestWorkload() + expectedName := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName) + current := newExistingDeployment(expectedName, workload.UID, testDefaultPlacement) + oldStyle := newExistingDeployment(rdTestWorkloadName+"-"+testDefaultPlacement+"-"+testLocationName, workload.UID, testDefaultPlacement) + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects( + newTestLocationBinding(testLocationName, "DFW"), + newTestComputeAvailability(testLocationName), + current, oldStyle, + ). + WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). + Build() + r := &WorkloadReconciler{} + + desired, orphaned, err := r.getDeploymentsForWorkload(context.Background(), cl, workload) + require.NoError(t, err) + require.Len(t, desired, 1) + assert.Equal(t, expectedName, desired[0].Name) + require.Len(t, orphaned, 1) + assert.Equal(t, oldStyle.Name, orphaned[0].Name) +} + +// TestGetDeploymentsForWorkload_ReplacesOldName verifies that a deployment +// under an old-style name is orphaned and the expected name is desired in its +// place. +func TestGetDeploymentsForWorkload_ReplacesOldName(t *testing.T) { + t.Parallel() + + workload := newDeploymentMatchingTestWorkload() + oldStyle := newExistingDeployment(rdTestWorkloadName+"-"+testDefaultPlacement+"-"+testLocationName, workload.UID, testDefaultPlacement) + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects( + newTestLocationBinding(testLocationName, "DFW"), + newTestComputeAvailability(testLocationName), + oldStyle, + ). + WithIndex(&computev1alpha.WorkloadDeployment{}, deploymentWorkloadUIDIndex, deploymentWorkloadUIDIndexFunc). + Build() + r := &WorkloadReconciler{} + + desired, orphaned, err := r.getDeploymentsForWorkload(context.Background(), cl, workload) + require.NoError(t, err) + require.Len(t, desired, 1) + assert.Equal(t, naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName), desired[0].Name) + require.Len(t, orphaned, 1) + assert.Equal(t, oldStyle.Name, orphaned[0].Name) +} + +// TestUpsertWorkloadDeployment_RefusesForeignWorkload verifies that a +// deployment belonging to another workload is never overwritten. +func TestUpsertWorkloadDeployment_RefusesForeignWorkload(t *testing.T) { + t.Parallel() + + workload := newDeploymentMatchingTestWorkload() + name := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName) + foreign := newExistingDeployment(name, types.UID("other-uid"), "other-placement") + + cl := fake.NewClientBuilder(). + WithScheme(newNetworkingScheme()). + WithObjects(foreign). + Build() + + desired := newExistingDeployment(name, workload.UID, testDefaultPlacement) + _, err := upsertWorkloadDeployment(context.Background(), cl, workload, desired) + require.Error(t, err) + assert.Contains(t, err.Error(), "other-uid") + + var stored computev1alpha.WorkloadDeployment + require.NoError(t, cl.Get(context.Background(), client.ObjectKeyFromObject(foreign), &stored)) + assert.Equal(t, types.UID("other-uid"), stored.Spec.WorkloadRef.UID) + assert.Equal(t, "other-placement", stored.Spec.PlacementName) +} + +// TestUpsertWorkloadDeployment_CreatesOwnedDeployment verifies a new +// deployment is created and controlled by its workload. +func TestUpsertWorkloadDeployment_CreatesOwnedDeployment(t *testing.T) { + t.Parallel() + + workload := newDeploymentMatchingTestWorkload() + name := naming.DeploymentName(workload.Name, workload.UID, testDefaultPlacement, testLocationName) + cl := fake.NewClientBuilder().WithScheme(newNetworkingScheme()).Build() + + desired := newExistingDeployment(name, workload.UID, testDefaultPlacement) + deployment, err := upsertWorkloadDeployment(context.Background(), cl, workload, desired) + require.NoError(t, err) + assert.Equal(t, name, deployment.Name) + assert.True(t, metav1.IsControlledBy(deployment, workload)) +} diff --git a/internal/naming/naming.go b/internal/naming/naming.go new file mode 100644 index 00000000..08e5f219 --- /dev/null +++ b/internal/naming/naming.go @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +// Package naming builds the names of the objects compute derives from a +// workload. +package naming + +import ( + "crypto/sha256" + "encoding/hex" + "strings" + + "k8s.io/apimachinery/pkg/types" +) + +const ( + deploymentNamePrefixLength = 30 + deploymentNameHashLength = 10 + + // MaxDeploymentNameLength is the longest name DeploymentName returns. + MaxDeploymentNameLength = deploymentNamePrefixLength + 1 + deploymentNameHashLength +) + +// DeploymentName returns the name of the WorkloadDeployment that runs a +// workload's placement at a location. The name is a readable prefix of the +// workload name followed by a hash of the workload UID, placement and +// location, so its length never depends on the placement or location and two +// workloads never share a name. +func DeploymentName(workloadName string, workloadUID types.UID, placement, location string) string { + prefix := workloadName + if len(prefix) > deploymentNamePrefixLength { + prefix = prefix[:deploymentNamePrefixLength] + } + prefix = strings.TrimRight(prefix, "-") + + sum := sha256.Sum256([]byte(string(workloadUID) + "\x00" + placement + "\x00" + strings.ToLower(location))) + return prefix + "-" + hex.EncodeToString(sum[:])[:deploymentNameHashLength] +} diff --git a/internal/naming/naming_test.go b/internal/naming/naming_test.go new file mode 100644 index 00000000..9cfe11d6 --- /dev/null +++ b/internal/naming/naming_test.go @@ -0,0 +1,62 @@ +// SPDX-License-Identifier: AGPL-3.0-only + +package naming + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/api/validation" + "k8s.io/apimachinery/pkg/types" +) + +const testUID = types.UID("0f7c9a52-3b1e-4d8e-9a61-2f4c7b8e1d05") + +func TestDeploymentName_Deterministic(t *testing.T) { + a := DeploymentName("checkout-api", testUID, "production", "us-east-1") + b := DeploymentName("checkout-api", testUID, "production", "us-east-1") + assert.Equal(t, a, b) + assert.True(t, strings.HasPrefix(a, "checkout-api-")) + assert.Len(t, a, len("checkout-api-")+deploymentNameHashLength) +} + +func TestDeploymentName_LocationCaseInsensitive(t *testing.T) { + assert.Equal(t, + DeploymentName("app", testUID, "default", "US-EAST-1"), + DeploymentName("app", testUID, "default", "us-east-1"), + ) +} + +func TestDeploymentName_LengthBound(t *testing.T) { + long63 := strings.Repeat("a", 63) + name := DeploymentName(long63, testUID, long63, long63) + assert.Len(t, name, MaxDeploymentNameLength) + assert.LessOrEqual(t, MaxDeploymentNameLength, 41) + assert.Empty(t, validation.NameIsDNSLabel(name, false)) +} + +func TestDeploymentName_DistinctInputs(t *testing.T) { + assert.NotEqual(t, + DeploymentName("app-a", testUID, "b", "loc"), + DeploymentName("app", testUID, "a-b", "loc"), + "joining with a dash must not make different placements collide", + ) + assert.NotEqual(t, + DeploymentName("app", testUID, "default", "loc-a"), + DeploymentName("app", testUID, "default", "loc-b"), + ) + assert.NotEqual(t, + DeploymentName("app", testUID, "default", "loc"), + DeploymentName("app", types.UID("another-uid"), "default", "loc"), + "a recreated workload must get fresh names", + ) +} + +func TestDeploymentName_TrimsTrailingDash(t *testing.T) { + workloadName := strings.Repeat("a", 29) + "-bcd" + name := DeploymentName(workloadName, testUID, "default", "loc") + assert.True(t, strings.HasPrefix(name, strings.Repeat("a", 29)+"-")) + assert.NotContains(t, name, "--") + assert.Empty(t, validation.NameIsDNSLabel(name, false)) +} diff --git a/internal/validation/workload_validation.go b/internal/validation/workload_validation.go index fa3d4de6..7d170049 100644 --- a/internal/validation/workload_validation.go +++ b/internal/validation/workload_validation.go @@ -28,13 +28,29 @@ import ( func ValidateWorkloadCreate(w *computev1alpha.Workload, opts WorkloadValidationOptions) field.ErrorList { allErrs := field.ErrorList{} - // allErrs = append(allErrs, validateWorkloadMetadata(w)...) + allErrs = append(allErrs, validateWorkloadName(w.Name)...) allErrs = append(allErrs, validateWorkloadSpec(w.Spec, opts)...) allErrs = append(allErrs, validateWorkloadImages(w, nil)...) return allErrs } +// validateWorkloadName requires a DNS-1123 label, so every name derived from +// the workload stays within the limits of the objects that run it. Only +// creates are checked, so workloads stored under older rules stay updatable. +func validateWorkloadName(name string) field.ErrorList { + namePath := field.NewPath("metadata", "name") + if len(name) == 0 { + return field.ErrorList{field.Required(namePath, "")} + } + + var allErrs field.ErrorList + for _, msg := range apimachineryvalidation.NameIsDNSLabel(name, false) { + allErrs = append(allErrs, field.Invalid(namePath, name, msg)) + } + return allErrs +} + // ValidateWorkloadUpdate validates a workload update. It applies the // create-time rules, plus the rules that need the previous state. func ValidateWorkloadUpdate(w, oldWorkload *computev1alpha.Workload, opts WorkloadValidationOptions) field.ErrorList { diff --git a/internal/validation/workload_validation_test.go b/internal/validation/workload_validation_test.go index 5e8a7a68..e131601e 100644 --- a/internal/validation/workload_validation_test.go +++ b/internal/validation/workload_validation_test.go @@ -46,12 +46,35 @@ func TestValidateWorkloads(t *testing.T) { "basic fields create": { workload: &computev1alpha.Workload{}, expectedErrors: field.ErrorList{ + field.Required(field.NewPath("metadata.name"), ""), field.NotSupported(field.NewPath("spec.template.spec.runtime.resources"), "", []string{}), field.Required(field.NewPath("spec.template.spec.runtime"), ""), field.Required(field.NewPath("spec.template.spec.networkInterfaces"), ""), field.Required(field.NewPath("spec.placements"), ""), }, }, + "workload name of 63 characters": { + workload: MakeSandboxWorkload(strings.Repeat("a", 63)), + expectedErrors: field.ErrorList{}, + }, + "workload name longer than 63 characters": { + workload: MakeSandboxWorkload(strings.Repeat("a", 64)), + expectedErrors: field.ErrorList{ + field.Invalid(field.NewPath("metadata.name"), "", ""), + }, + }, + "workload name with a dot": { + workload: MakeSandboxWorkload("web.app"), + expectedErrors: field.ErrorList{ + field.Invalid(field.NewPath("metadata.name"), "", ""), + }, + }, + "workload name with uppercase": { + workload: MakeSandboxWorkload("Web"), + expectedErrors: field.ErrorList{ + field.Invalid(field.NewPath("metadata.name"), "", ""), + }, + }, "location selector by city code": { workload: MakeSandboxWorkload( "test", @@ -1160,3 +1183,45 @@ func TestValidateWorkloadUpdate_UnchangedImage(t *testing.T) { cmpErrs(t, wantErrs, errs) }) } + +// TestValidateWorkloadUpdate_AllowsExistingName verifies that a workload +// stored under a name the create-time rules now reject stays updatable. +func TestValidateWorkloadUpdate_AllowsExistingName(t *testing.T) { + scheme := k8sruntime.NewScheme() + utilruntime.Must(computev1alpha.AddToScheme(scheme)) + utilruntime.Must(networkingv1alpha.AddToScheme(scheme)) + utilruntime.Must(clientgoscheme.AddToScheme(scheme)) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if sar, ok := obj.(*authorizationv1.SubjectAccessReview); ok { + sar.GenerateName = sarGenerateName + sar.Status.Allowed = true + } + return c.Create(ctx, obj, opts...) + }, + }). + WithObjects(&networkingv1alpha.Network{ + ObjectMeta: metav1.ObjectMeta{Namespace: testDefaultNamespace, Name: testDefaultNamespace}, + }). + Build() + + name := strings.Repeat("a", 70) + ".example" + oldWorkload := MakeSandboxWorkload(name) + newWorkload := oldWorkload.DeepCopy() + opts := WorkloadValidationOptions{ + Client: fakeClient, + Context: context.Background(), + ValidLocations: []string{testCityCodeDFW}, + Workload: newWorkload, + } + + if errs := ValidateWorkloadUpdate(newWorkload, oldWorkload, opts); len(errs) != 0 { + t.Errorf("expected no errors, got: %v", errs) + } + if errs := ValidateWorkloadCreate(newWorkload.DeepCopy(), opts); len(errs) == 0 { + t.Error("expected the same name to be rejected on create") + } +}