fix: ensure old object to be assigned in webhook (#1868)

Signed-off-by: Sunghoon Kang <[email protected]>
Co-authored-by: Hidde Beydals <[email protected]>

internal/webhook/stage/webhook.go

@@ -25,7 +25,8 @@ var (
 )
 
 type webhook struct {
-	client client.Client
+	client  client.Client
+	decoder *admission.Decoder
 
 	// The following behaviors are overridable for testing purposes:
 
@@ -49,7 +50,11 @@ func SetupWebhookWithManager(
 	cfg libWebhook.Config,
 	mgr ctrl.Manager,
 ) error {
-	w := newWebhook(cfg, mgr.GetClient())
+	w := newWebhook(
+		cfg,
+		mgr.GetClient(),
+		admission.NewDecoder(mgr.GetScheme()),
+	)
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(&kargoapi.Stage{}).
 		WithDefaulter(w).
@@ -60,9 +65,11 @@ func SetupWebhookWithManager(
 func newWebhook(
 	cfg libWebhook.Config,
 	kubeClient client.Client,
+	decoder *admission.Decoder,
 ) *webhook {
 	w := &webhook{
-		client: kubeClient,
+		client:  kubeClient,
+		decoder: decoder,
 	}
 	w.admissionRequestFromContextFn = admission.RequestFromContext
 	w.validateProjectFn = libWebhook.ValidateProject
@@ -92,8 +99,12 @@ func (w *webhook) Default(ctx context.Context, obj runtime.Object) error {
 	}
 
 	var oldStage *kargoapi.Stage
-	if req.OldObject.Object != nil {
-		oldStage = req.OldObject.Object.(*kargoapi.Stage) // nolint: forcetypeassert
+	// We need to decode old object manually since controller-runtime doesn't decode it for us.
+	if req.Operation == admissionv1.Update {
+		oldStage = &kargoapi.Stage{}
+		if err := w.decoder.DecodeRaw(req.OldObject, oldStage); err != nil {
+			return fmt.Errorf("decode old object: %w", err)
+		}
 	}
 
 	if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update {

internal/webhook/stage/webhook_test.go

@@ -2,6 +2,7 @@ package stage
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"testing"
 
@@ -25,6 +26,7 @@ func TestNewWebhook(t *testing.T) {
 	w := newWebhook(
 		libWebhook.Config{},
 		kubeClient,
+		admission.NewDecoder(kubeClient.Scheme()),
 	)
 	// Assert that all overridable behaviors were initialized to a default:
 	require.NotNil(t, w.admissionRequestFromContextFn)
@@ -36,6 +38,9 @@ func TestNewWebhook(t *testing.T) {
 
 func TestDefault(t *testing.T) {
 	const testShardName = "fake-shard"
+	scheme := runtime.NewScheme()
+	require.NoError(t, kargoapi.AddToScheme(scheme))
+
 	testCases := []struct {
 		name       string
 		webhook    *webhook
@@ -360,6 +365,9 @@ func TestDefault(t *testing.T) {
 					UserInfo: authnv1.UserInfo{
 						Username: "real-user",
 					},
+					OldObject: runtime.RawExtension{
+						Object: &kargoapi.Stage{},
+					},
 				},
 			},
 			stage: &kargoapi.Stage{
@@ -391,6 +399,9 @@ func TestDefault(t *testing.T) {
 					UserInfo: authnv1.UserInfo{
 						Username: "real-user",
 					},
+					OldObject: runtime.RawExtension{
+						Object: &kargoapi.Stage{},
+					},
 				},
 			},
 			stage: &kargoapi.Stage{
@@ -704,6 +715,9 @@ func TestDefault(t *testing.T) {
 					UserInfo: authnv1.UserInfo{
 						Username: "real-user",
 					},
+					OldObject: runtime.RawExtension{
+						Object: &kargoapi.Stage{},
+					},
 				},
 			},
 			stage: &kargoapi.Stage{
@@ -735,6 +749,9 @@ func TestDefault(t *testing.T) {
 					UserInfo: authnv1.UserInfo{
 						Username: "real-user",
 					},
+					OldObject: runtime.RawExtension{
+						Object: &kargoapi.Stage{},
+					},
 				},
 			},
 			stage: &kargoapi.Stage{
@@ -810,6 +827,19 @@ func TestDefault(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
+
+			// Apply default decoder to all test cases
+			tc.webhook.decoder = admission.NewDecoder(scheme)
+
+			// Make sure old object has corresponding Raw data instead of Object
+			// since controller-runtime doesn't decode the old object.
+			if tc.req.OldObject.Object != nil {
+				data, err := json.Marshal(tc.req.OldObject.Object)
+				require.NoError(t, err)
+				tc.req.OldObject.Raw = data
+				tc.req.OldObject.Object = nil
+			}
+
 			ctx := admission.NewContextWithRequest(
 				context.Background(),
 				tc.req,