From 4899f0ac0f48ae7bd839574f3a1656ad92ec21b8 Mon Sep 17 00:00:00 2001 From: Kent Rancourt Date: Thu, 28 Mar 2024 09:54:50 -0400 Subject: [PATCH] fix la bel sync logic in webhooks Signed-off-by: Kent Rancourt --- internal/webhook/freight/webhook.go | 25 +++++--- internal/webhook/freight/webhook_test.go | 69 +++++++++++++++------- internal/webhook/warehouse/webhook.go | 6 +- internal/webhook/warehouse/webhook_test.go | 15 +++-- 4 files changed, 77 insertions(+), 38 deletions(-) diff --git a/internal/webhook/freight/webhook.go b/internal/webhook/freight/webhook.go index 456b82f37..456ffdf13 100644 --- a/internal/webhook/freight/webhook.go +++ b/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 diff --git a/internal/webhook/freight/webhook_test.go b/internal/webhook/freight/webhook_test.go index 04232b010..db106fe5b 100644 --- a/internal/webhook/freight/webhook_test.go +++ b/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) }) } diff --git a/internal/webhook/warehouse/webhook.go b/internal/webhook/warehouse/webhook.go index a851bd9e3..7039ead33 100644 --- a/internal/webhook/warehouse/webhook.go +++ b/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 diff --git a/internal/webhook/warehouse/webhook_test.go b/internal/webhook/warehouse/webhook_test.go index b75f825d5..271016ee2 100644 --- a/internal/webhook/warehouse/webhook_test.go +++ b/internal/webhook/warehouse/webhook_test.go @@ -40,7 +40,7 @@ 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, @@ -48,25 +48,24 @@ func TestDefault(t *testing.T) { } 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) }) }