Move deletion validation out of the webhook and into delete request validations (#536)

* Remove packagerev deletion webhook

* fix rpkg-lifecycle e2e cli test

* Re-add resources to packagerevision_test.go

---------

Co-authored-by: Liam Fallon <35595825+liamfallon@users.noreply.github.com>

Author: kushnaidu

pkg/apiserver/webhooks.go

@@ -27,7 +27,6 @@ import (
 	"fmt"
 	"io"
 	"math/big"
-	"net"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -38,7 +37,6 @@ import (
 	"github.com/fsnotify/fsnotify"
 	"go.opentelemetry.io/otel"
 
-	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
 	configapi "github.com/nephio-project/porch/api/porchconfig/v1alpha1"
 	"github.com/nephio-project/porch/pkg/util"
 	admissionv1 "k8s.io/api/admission/v1"
@@ -57,7 +55,6 @@ type WebhookType string
 const (
 	WebhookTypeService           WebhookType = "service"
 	WebhookTypeUrl               WebhookType = "url"
-	serverEndpoint                           = "/validate-deletion"
 	repositoryValidationEndpoint             = "/validate-repository"
 )
 
@@ -66,15 +63,14 @@ var (
 	certModTime time.Time
 )
 
-var tracer = otel.Tracer("deletion-webhook")
+var tracer = otel.Tracer("repository-webhook")
 
-// WebhookConfig defines the configuration for the PackageRevision deletion webhook
+// WebhookConfig defines the configuration for the Repository validation webhook
 type WebhookConfig struct {
 	Type                 WebhookType
 	ServiceName          string // only used if Type == WebhookTypeService
 	ServiceNamespace     string // only used if Type == WebhookTypeService
 	Host                 string // only used if Type == WebhookTypeUrl
-	Path                 string
 	Port                 int32
 	RepositoryPath       string
 	RepoServiceName      string
@@ -102,7 +98,6 @@ func newWebhookConfig(ctx context.Context) *WebhookConfig {
 		cfg.Type = WebhookTypeUrl
 		cfg.Host = getEnv("WEBHOOK_HOST", "localhost")
 	}
-	cfg.Path = serverEndpoint
 	// Always use the WebhookTypeService for repository webhook validation
 	cfg.RepositoryPath = repositoryValidationEndpoint
 	cfg.RepoServiceName, cfg.RepoServiceNamespace = webhookServiceName(ctx)
@@ -168,7 +163,7 @@ func webhookServiceName(ctx context.Context) (serviceName, serviceNamespace stri
 func setupWebhooks(ctx context.Context, clientReader client.Reader) error {
 	cfg := newWebhookConfig(ctx)
 	// TODO: Refactor webhook setup to support optional webhooks and better separation of concerns.
-	// Currently webhooks are always enabled and required for Repository/PackageRevision validation.
+	// Currently webhooks are always enabled and required for Repository validation.
 	// Consider: 1) Making webhooks optional via explicit flag, 2) Separating cert management from webhook lifecycle,
 	// 3) Supporting webhook-less mode for development/testing.
 	if !cfg.CertManWebhook {
@@ -310,39 +305,11 @@ func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []b
 	// Set max timeout value for ValidatingWebhooks
 	cfg.timeout = 30
 	var (
-		validationCfgName = "packagerev-deletion-validating-webhook"
 		repositoryCfgName = "repository-validating-webhook"
 		fail              = admissionregistrationv1.Fail
 		none              = admissionregistrationv1.SideEffectClassNone
 	)
 
-	// Webhook for PackageRevision deletion
-	validateConfig := &admissionregistrationv1.ValidatingWebhookConfiguration{
-		ObjectMeta: metav1.ObjectMeta{
-			Name: validationCfgName,
-		},
-		Webhooks: []admissionregistrationv1.ValidatingWebhook{{
-			Name: "packagerevdeletion.google.com",
-			ClientConfig: admissionregistrationv1.WebhookClientConfig{
-				CABundle: caCert,
-			},
-			Rules: []admissionregistrationv1.RuleWithOperations{{
-				Operations: []admissionregistrationv1.OperationType{
-					admissionregistrationv1.Delete,
-				},
-				Rule: admissionregistrationv1.Rule{
-					APIGroups:   []string{porchapi.SchemeGroupVersion.Group},
-					APIVersions: []string{porchapi.SchemeGroupVersion.Version},
-					Resources:   []string{porchapi.PackageRevisionGVR.Resource},
-				},
-			}},
-			AdmissionReviewVersions: []string{"v1"},
-			SideEffects:             &none,
-			FailurePolicy:           &fail,
-			TimeoutSeconds:          &cfg.timeout,
-		}},
-	}
-
 	// Webhook for Repository validation
 	repositoryWebhook := admissionregistrationv1.ValidatingWebhookConfiguration{
 		ObjectMeta: metav1.ObjectMeta{
@@ -371,37 +338,17 @@ func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []b
 		}},
 	}
 
-	// Set service or URL for both webhooks
-	switch cfg.Type {
-	case WebhookTypeService:
-		validateConfig.Webhooks[0].ClientConfig.Service = &admissionregistrationv1.ServiceReference{
-			Name:      cfg.ServiceName,
-			Namespace: cfg.ServiceNamespace,
-			Path:      &cfg.Path,
-			Port:      &cfg.Port,
-		}
-	case WebhookTypeUrl:
-		url := fmt.Sprintf("https://%s%s", net.JoinHostPort(cfg.Host, fmt.Sprintf("%d", cfg.Port)), cfg.Path)
-		validateConfig.Webhooks[0].ClientConfig.URL = &url
-	default:
-		return fmt.Errorf("invalid webhook type: %s", cfg.Type)
-	}
-
+	// Set service for repository webhook
 	repositoryWebhook.Webhooks[0].ClientConfig.Service = &admissionregistrationv1.ServiceReference{
 		Name:      cfg.RepoServiceName,
 		Namespace: cfg.RepoServiceNamespace,
 		Path:      &cfg.RepositoryPath,
 		Port:      &cfg.Port,
 	}
 
-	// Delete and recreate both webhook to allow updates in webhook configurations
-	_ = kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(ctx, validationCfgName, metav1.DeleteOptions{})
+	// Delete and recreate repository webhook to allow updates in webhook configurations
 	_ = kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(ctx, repositoryCfgName, metav1.DeleteOptions{})
 
-	if _, err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(ctx, validateConfig, metav1.CreateOptions{}); err != nil {
-		return fmt.Errorf("failed to create package revision webhook: %w", err)
-	}
-
 	if _, err := kubeClient.AdmissionregistrationV1().ValidatingWebhookConfigurations().Create(ctx, &repositoryWebhook, metav1.CreateOptions{}); err != nil {
 		return fmt.Errorf("failed to create repository validation webhook: %w", err)
 	}
@@ -489,9 +436,6 @@ func runWebhookServer(ctx context.Context, cfg *WebhookConfig, clientReader clie
 		go watchCertificates(ctx, cfg.CertStorageDir, certFile, keyFile)
 	}
 	klog.Infoln("Starting webhook server")
-	http.HandleFunc(cfg.Path, func(w http.ResponseWriter, r *http.Request) {
-		validateDeletion(w, r, clientReader)
-	})
 	http.HandleFunc(cfg.RepositoryPath, func(w http.ResponseWriter, r *http.Request) {
 		validateRepository(w, r, clientReader)
 	})
@@ -513,66 +457,6 @@ func runWebhookServer(ctx context.Context, cfg *WebhookConfig, clientReader clie
 
 }
 
-func validateDeletion(w http.ResponseWriter, r *http.Request, clientReader client.Reader) {
-	ctx, span := tracer.Start(r.Context(), "validateDeletion")
-	defer span.End()
-	klog.Infoln("received request to validate deletion")
-
-	admissionReviewRequest, err := decodeAdmissionReview(r)
-	if err != nil {
-		errMsg := fmt.Sprintf("error getting admission review from request: %v", err)
-		writeErr(errMsg, &w)
-		return
-	}
-
-	// Verify that we have a PackageRevision resource
-	if admissionReviewRequest.Request.Resource != util.SchemaToMetaGVR(porchapi.PackageRevisionGVR) {
-		errMsg := fmt.Sprintf("did not receive PackageRevision, got %s", admissionReviewRequest.Request.Resource.Resource)
-		writeErr(errMsg, &w)
-		return
-	}
-
-	// Get the package revision using the name and namespace from the request.
-	pr := porchapi.PackageRevision{}
-	if err := clientReader.Get(ctx, client.ObjectKey{
-		Namespace: admissionReviewRequest.Request.Namespace,
-		Name:      admissionReviewRequest.Request.Name,
-	}, &pr); err != nil {
-		klog.Errorf("could not get package revision: %s", err.Error())
-	}
-
-	admissionResponse := &admissionv1.AdmissionResponse{}
-	if pr.Spec.Lifecycle == porchapi.PackageRevisionLifecyclePublished {
-		admissionResponse.Allowed = false
-		admissionResponse.Result = &metav1.Status{
-			Status:  "Failure",
-			Message: fmt.Sprintf("failed to delete package revision %q: published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion", pr.Name),
-			Reason:  "Published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion.",
-		}
-	} else {
-		admissionResponse.Allowed = true
-		admissionResponse.Result = &metav1.Status{
-			Status:  "Success",
-			Message: fmt.Sprintf("Successfully deleted package revision %q", pr.Name),
-		}
-	}
-
-	resp, err := constructResponse(admissionResponse, admissionReviewRequest)
-	if err != nil {
-		errMsg := fmt.Sprintf("error constructing response: %v", err)
-		writeErr(errMsg, &w)
-		return
-	}
-
-	w.Header().Set("Content-Type", "application/json")
-	_, err = w.Write(resp) // #nosec G705
-	if err != nil {
-		errMsg := fmt.Sprintf("error writing response: %v", err)
-		writeErr(errMsg, &w)
-		return
-	}
-}
-
 func decodeAdmissionReview(r *http.Request) (*admissionv1.AdmissionReview, error) {
 	if r.Header.Get("Content-Type") != "application/json" {
 		return nil, fmt.Errorf("expected Content-Type 'application/json'")
@@ -656,6 +540,9 @@ func getEnvInt32(key string, defaultValue int32) int32 {
 }
 
 func validateRepository(w http.ResponseWriter, r *http.Request, clientReader client.Reader) {
+	ctx, span := tracer.Start(r.Context(), "validateRepository")
+	defer span.End()
+
 	admissionReviewRequest, err := decodeAdmissionReview(r)
 	if err != nil {
 		writeErr(fmt.Sprintf("error decoding admission review: %v", err), &w)
@@ -683,7 +570,7 @@ func validateRepository(w http.ResponseWriter, r *http.Request, clientReader cli
 
 	// Check for conflicts with existing repositories
 	var repoList configapi.RepositoryList
-	if err := clientReader.List(context.Background(), &repoList); err != nil {
+	if err := clientReader.List(ctx, &repoList); err != nil {
 		klog.Errorf("failed to list repositories: %v", err)
 		writeErr(fmt.Sprintf("could not list repositories: %v", err), &w)
 		return

pkg/apiserver/webhooks_test.go

@@ -319,64 +319,6 @@ func TestWatchCertificatesGracefulTermination(t *testing.T) {
 	require.NoError(t, err)
 }
 
-func TestValidateDeletion(t *testing.T) {
-	scheme := runtime.NewScheme()
-	configapi.AddToScheme(scheme)
-	fakeClient := fake.NewClientBuilder().WithScheme(scheme).Build()
-	t.Run("invalid content-type", func(t *testing.T) {
-		request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil)
-		require.NoError(t, err)
-		request.Header.Set("Content-Type", "foo")
-		response := httptest.NewRecorder()
-
-		validateDeletion(response, request, fakeClient)
-		require.Equal(t,
-			"error getting admission review from request: expected Content-Type 'application/json'",
-			response.Body.String())
-	})
-	t.Run("valid content-type, but no body", func(t *testing.T) {
-		request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil)
-		require.NoError(t, err)
-		request.Header.Set("Content-Type", "application/json")
-		response := httptest.NewRecorder()
-
-		validateDeletion(response, request, fakeClient)
-		require.Equal(t,
-			"error getting admission review from request: admission review request is empty",
-			response.Body.String())
-	})
-	t.Run("wrong GVK in request", func(t *testing.T) {
-		request, err := http.NewRequest(http.MethodPost, serverEndpoint, nil)
-		require.NoError(t, err)
-
-		request.Header.Set("Content-Type", "application/json")
-		response := httptest.NewRecorder()
-
-		admissionReviewRequest := admissionv1.AdmissionReview{
-			TypeMeta: v1.TypeMeta{
-				Kind:       "AdmissionReview",
-				APIVersion: "admission.k8s.io/v1",
-			},
-			Request: &admissionv1.AdmissionRequest{
-				Resource: v1.GroupVersionResource{
-					Group:    "porch.kpt.dev",
-					Version:  "v1alpha1",
-					Resource: "not-a-package-revision",
-				},
-			},
-		}
-
-		body, err := json.Marshal(admissionReviewRequest)
-		require.NoError(t, err)
-
-		request.Body = io.NopCloser(bytes.NewReader(body))
-		validateDeletion(response, request, fakeClient)
-		require.Equal(t,
-			"did not receive PackageRevision, got not-a-package-revision",
-			response.Body.String())
-	})
-}
-
 func TestValidateRepository(t *testing.T) {
 	scheme := runtime.NewScheme()
 	configapi.AddToScheme(scheme)

pkg/registry/porch/packagecommon.go

@@ -390,6 +390,16 @@ func getLifecycleTransition(oldPkgRev, newPkgRev *porchapi.PackageRevision) stri
 }
 
 func (r *packageCommon) validateDelete(ctx context.Context, deleteValidation rest.ValidateObjectFunc, obj runtime.Object, name, namespace string) (*configapi.Repository, error) {
+	// Check if the PackageRevision is Published and not in DeletionProposed state
+	if pkgRev, ok := obj.(*porchapi.PackageRevision); ok {
+		if pkgRev.Spec.Lifecycle == porchapi.PackageRevisionLifecyclePublished {
+			return nil, apierrors.NewForbidden(
+				porchapi.Resource("packagerevisions"),
+				name,
+				fmt.Errorf("published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion"))
+		}
+	}
+
 	if deleteValidation != nil {
 		err := deleteValidation(ctx, obj)
 		if err != nil {

pkg/registry/porch/packagerevision_test.go

@@ -403,6 +403,7 @@ info:
 	assert.IsType(t, &porchapi.PackageRevision{}, result)
 
 	//=========================================================================================
+
 	// Missing namespace
 	result, deleted, err = packagerevisions.Delete(context.TODO(), pkgRevName, nil, &metav1.DeleteOptions{})
 	assert.Error(t, err)

test/e2e/cli/testdata/rpkg-lifecycle/config.yaml

@@ -110,7 +110,7 @@ commands:
       - delete
       - git.lifecycle-package.lifecycle
       - --namespace=rpkg-lifecycle
-    stderr: "git.lifecycle-package.lifecycle failed (admission webhook \"packagerevdeletion.google.com\" denied the request: failed to delete package revision \"git.lifecycle-package.lifecycle\": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion)\nError: errors:\n  admission webhook \"packagerevdeletion.google.com\" denied the request: failed to delete package revision \"git.lifecycle-package.lifecycle\": published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion \n"
+    stderr: "git.lifecycle-package.lifecycle failed (packagerevisions.porch.kpt.dev \"git.lifecycle-package.lifecycle\" is forbidden: published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion)\nError: errors:\n  packagerevisions.porch.kpt.dev \"git.lifecycle-package.lifecycle\" is forbidden: published PackageRevisions must be proposed for deletion by setting spec.lifecycle to 'DeletionProposed' prior to deletion \n"
     exitCode: 1
   - args:
       - porchctl