fix(webhooks): label sync logic in webhooks

internal/webhook/freight/webhook.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"github.com/technosophos/moniker"
+	admissionv1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -86,20 +87,30 @@ func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
 	freight.Name = freight.GenerateID()
 
 	// Sync the convenience alias field with the alias label
+	req, err := admission.RequestFromContext(ctx)
+	if err != nil {
+		return apierrors.NewInternalError(
+			fmt.Errorf("error getting admission request from context: %w", err),
+		)
+	}
 	if freight.Labels == nil {
 		freight.Labels = make(map[string]string, 1)
 	}
 	if freight.Alias != "" {
+		// Alias field has a value, so just copy it to the label
 		freight.Labels[kargoapi.AliasLabelKey] = freight.Alias
-	} else if freight.Labels[kargoapi.AliasLabelKey] != "" {
-		freight.Alias = freight.Labels[kargoapi.AliasLabelKey]
-	} else {
-		alias, err := w.getAvailableFreightAliasFn(ctx)
-		if err != nil {
+	} else if req.Operation == admissionv1.Create {
+		// Alias field is empty and this is a create operation, so generate a new
+		// alias and assign it to both the alias field and the label
+		var err error
+		if freight.Alias, err = w.getAvailableFreightAliasFn(ctx); err != nil {
 			return fmt.Errorf("get available freight alias: %w", err)
 		}
-		freight.Alias = alias
-		freight.Labels[kargoapi.AliasLabelKey] = alias
+		freight.Labels[kargoapi.AliasLabelKey] = freight.Alias
+	} else {
+		// Alias field is empty and this is an update operation, so ensure the
+		// label does not exist
+		delete(freight.Labels, kargoapi.AliasLabelKey)
 	}
 
 	return nil

internal/webhook/freight/webhook_test.go

@@ -7,12 +7,14 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	admissionv1 "k8s.io/api/admission/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	kargoapi "github.com/akuity/kargo/api/v1alpha1"
 )
@@ -31,41 +33,41 @@ func TestNewWebhook(t *testing.T) {
 func TestDefault(t *testing.T) {
 	testCases := []struct {
 		name       string
+		op         admissionv1.Operation
 		webhook    *webhook
 		freight    *kargoapi.Freight
 		assertions func(*testing.T, *kargoapi.Freight, error)
 	}{
 		{
-			name:    "sync alias label to alias field",
+			name:    "error getting request from context",
 			webhook: &webhook{},
-			freight: &kargoapi.Freight{
-				Alias: "fake-alias",
-			},
-			assertions: func(t *testing.T, freight *kargoapi.Freight, err error) {
-				require.NoError(t, err)
-				require.NotEmpty(t, freight.Name)
-				require.NotEmpty(t, freight.Labels)
-				require.Equal(t, "fake-alias", freight.Labels[kargoapi.AliasLabelKey])
+			freight: &kargoapi.Freight{},
+			assertions: func(t *testing.T, _ *kargoapi.Freight, err error) {
+				require.Error(t, err)
+				require.Contains(
+					t,
+					err.Error(),
+					"error getting admission request from context",
+				)
 			},
 		},
 		{
-			name:    "sync alias field to alias label",
+			name:    "sync alias label to non-empty alias field",
+			op:      admissionv1.Create,
 			webhook: &webhook{},
 			freight: &kargoapi.Freight{
-				ObjectMeta: metav1.ObjectMeta{
-					Labels: map[string]string{
-						kargoapi.AliasLabelKey: "fake-alias",
-					},
-				},
+				Alias: "fake-alias",
 			},
 			assertions: func(t *testing.T, freight *kargoapi.Freight, err error) {
 				require.NoError(t, err)
 				require.NotEmpty(t, freight.Name)
 				require.Equal(t, "fake-alias", freight.Alias)
+				require.Equal(t, "fake-alias", freight.Labels[kargoapi.AliasLabelKey])
 			},
 		},
 		{
 			name: "error getting available alias",
+			op:   admissionv1.Create,
 			webhook: &webhook{
 				getAvailableFreightAliasFn: func(context.Context) (string, error) {
 					return "", errors.New("something went wrong")
@@ -80,6 +82,7 @@ func TestDefault(t *testing.T) {
 		},
 		{
 			name: "success getting available alias",
+			op:   admissionv1.Create,
 			webhook: &webhook{
 				getAvailableFreightAliasFn: func(context.Context) (string, error) {
 					return "fake-alias", nil
@@ -90,17 +93,43 @@ func TestDefault(t *testing.T) {
 				require.NoError(t, err)
 				require.NotEmpty(t, freight.Name)
 				require.Equal(t, "fake-alias", freight.Alias)
-				require.NotEmpty(t, freight.Labels)
 				require.Equal(t, "fake-alias", freight.Labels[kargoapi.AliasLabelKey])
 			},
 		},
+		{
+			name:    "update with empty alias",
+			op:      admissionv1.Update,
+			webhook: &webhook{},
+			freight: &kargoapi.Freight{
+				ObjectMeta: metav1.ObjectMeta{
+					Labels: map[string]string{
+						kargoapi.AliasLabelKey: "fake-alias",
+					},
+				},
+			},
+			assertions: func(t *testing.T, freight *kargoapi.Freight, err error) {
+				require.NoError(t, err)
+				require.NotEmpty(t, freight.Name)
+				require.Empty(t, freight.Alias)
+				_, ok := freight.Labels[kargoapi.AliasLabelKey]
+				require.False(t, ok)
+			},
+		},
 	}
 	for _, testCase := range testCases {
-		t.Run(testCase.name, func(t *testing.T) {
-			err := testCase.webhook.Default(
-				context.Background(),
-				testCase.freight,
+		ctx := context.Background()
+		if testCase.op != "" {
+			ctx = admission.NewContextWithRequest(
+				ctx,
+				admission.Request{
+					AdmissionRequest: admissionv1.AdmissionRequest{
+						Operation: testCase.op,
+					},
+				},
 			)
+		}
+		t.Run(testCase.name, func(t *testing.T) {
+			err := testCase.webhook.Default(ctx, testCase.freight)
 			testCase.assertions(t, testCase.freight, err)
 		})
 	}

internal/webhook/warehouse/webhook.go

@@ -65,14 +65,14 @@ func newWebhook(kubeClient client.Client) *webhook {
 func (w *webhook) Default(_ context.Context, obj runtime.Object) error {
 	warehouse := obj.(*kargoapi.Warehouse) // nolint: forcetypeassert
 
-	// Sync the convenience shard field with the shard label
+	// Sync the shard label to the convenience shard field
 	if warehouse.Spec.Shard != "" {
 		if warehouse.Labels == nil {
 			warehouse.Labels = make(map[string]string, 1)
 		}
 		warehouse.Labels[kargoapi.ShardLabelKey] = warehouse.Spec.Shard
-	} else if warehouse.Labels[kargoapi.ShardLabelKey] != "" {
-		warehouse.Spec.Shard = warehouse.Labels[kargoapi.ShardLabelKey]
+	} else {
+		delete(warehouse.Labels, kargoapi.ShardLabelKey)
 	}
 
 	return nil

internal/webhook/warehouse/webhook_test.go

@@ -40,33 +40,32 @@ func TestDefault(t *testing.T) {
 		require.Empty(t, warehouse.Spec.Shard)
 	})
 
-	t.Run("sync shard label to shard field", func(t *testing.T) {
+	t.Run("sync shard label to non-empty shard field", func(t *testing.T) {
 		warehouse := &kargoapi.Warehouse{
 			Spec: &kargoapi.WarehouseSpec{
 				Shard: testShardName,
 			},
 		}
 		err := w.Default(context.Background(), warehouse)
 		require.NoError(t, err)
-		require.Equal(t, testShardName, warehouse.Labels[kargoapi.ShardLabelKey])
 		require.Equal(t, testShardName, warehouse.Spec.Shard)
+		require.Equal(t, testShardName, warehouse.Labels[kargoapi.ShardLabelKey])
 	})
 
-	t.Run("sync shard field to shard label", func(t *testing.T) {
+	t.Run("sync shard label to empty shard field", func(t *testing.T) {
 		warehouse := &kargoapi.Warehouse{
 			ObjectMeta: metav1.ObjectMeta{
 				Labels: map[string]string{
 					kargoapi.ShardLabelKey: testShardName,
 				},
 			},
-			Spec: &kargoapi.WarehouseSpec{
-				Shard: testShardName,
-			},
+			Spec: &kargoapi.WarehouseSpec{},
 		}
 		err := w.Default(context.Background(), warehouse)
 		require.NoError(t, err)
-		require.Equal(t, testShardName, warehouse.Labels[kargoapi.ShardLabelKey])
-		require.Equal(t, testShardName, warehouse.Spec.Shard)
+		require.Empty(t, warehouse.Spec.Shard)
+		_, ok := warehouse.Labels[kargoapi.ShardLabelKey]
+		require.False(t, ok)
 	})
 }