Merge pull request #93 from nokia/webhook-defaults

More sensible defaults for webhook environment variables

deployments/porch/5-rbac.yaml

@@ -36,6 +36,9 @@ rules:
   - apiGroups: ["config.porch.kpt.dev"]
     resources: ["packagerevs", "packagerevs/status"]
     verbs: ["get", "list", "watch", "create", "update", "patch", "delete"]
+  - apiGroups: ["apiregistration.k8s.io"]
+    resources: ["apiservices"]
+    verbs: ["get"]
   # Needed for priority and fairness
   - apiGroups: ["flowcontrol.apiserver.k8s.io"]
     resources: ["flowschemas", "prioritylevelconfigurations"]

pkg/apiserver/webhooks.go

@@ -35,8 +35,9 @@ import (
 
 	"github.com/fsnotify/fsnotify"
 
-	"github.com/nephio-project/porch/api/porch/v1alpha1"
+	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
 	"github.com/nephio-project/porch/internal/kpt/util/porch"
+	"github.com/nephio-project/porch/pkg/util"
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -49,15 +50,17 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
 )
 
-const (
-	serverEndpoint = "/validate-deletion"
-)
-
 type WebhookType string
 
 const (
 	WebhookTypeService WebhookType = "service"
 	WebhookTypeUrl     WebhookType = "url"
+	serverEndpoint                 = "/validate-deletion"
+)
+
+var (
+	cert        tls.Certificate
+	certModTime time.Time
 )
 
 // WebhookConfig defines the configuration for the PackageRevision deletion webhook
@@ -72,7 +75,8 @@ type WebhookConfig struct {
 	CertManWebhook   bool
 }
 
-func NewWebhookConfig() *WebhookConfig {
+// newWebhookConfig creates a new WebhookConfig object filled with values read from environment variables
+func newWebhookConfig(ctx context.Context) *WebhookConfig {
 	var cfg WebhookConfig
 	// NOTE: CERT_NAMESPACE is supported for backward compatibility.
 	// TODO: We may consider using only WEBHOOK_SERVICE_NAMESPACE instead.
@@ -82,9 +86,7 @@ func NewWebhookConfig() *WebhookConfig {
 		!hasEnv("WEBHOOK_HOST") {
 
 		cfg.Type = WebhookTypeService
-		cfg.ServiceName = getEnv("WEBHOOK_SERVICE_NAME", "api")
-		cfg.ServiceNamespace = getEnv("WEBHOOK_SERVICE_NAMESPACE", "porch-system")
-		cfg.ServiceNamespace = getEnv("CERT_NAMESPACE", cfg.ServiceNamespace)
+		cfg.ServiceName, cfg.ServiceNamespace = webhookServiceName(ctx)
 		cfg.Host = fmt.Sprintf("%s.%s.svc", cfg.ServiceName, cfg.ServiceNamespace)
 	} else {
 		cfg.Type = WebhookTypeUrl
@@ -97,13 +99,59 @@ func NewWebhookConfig() *WebhookConfig {
 	return &cfg
 }
 
-var (
-	cert        tls.Certificate
-	certModTime time.Time
-)
+// webhookServiceName returns the name and namespace of Kubernetes service belonging to the webhook
+func webhookServiceName(ctx context.Context) (serviceName, serviceNamespace string) {
+	var apiSvcNs string
+
+	// the webhook service namespace gets it value from the following sources in order of precedence:
+	// - WEBHOOK_SERVICE_NAME environment variable
+	// - the name of the service referenced in porch's APIService object
+	serviceName = os.Getenv("WEBHOOK_SERVICE_NAME")
+	if serviceName == "" { // empty value and unset envvar are the same for our purposes
+		// if WEBHOOK_SERVICE_NAME is not set, try to use the porch API service name
+		apiSvc, err := util.GetPorchApiServiceKey(ctx)
+		if err != nil {
+			panic(fmt.Sprintf("WEBHOOK_SERVICE_NAME environment variable is not set, and could not find porch's APIservice: %v", err))
+		}
+		serviceName = apiSvc.Name
+		apiSvcNs = apiSvc.Namespace // cache the namespace value to avoid duplicate calls of GetPorchApiServiceKey()
+	}
+
+	// the webhook service namespace gets it value from the following sources in order of precedence:
+	// - WEBHOOK_SERVICE_NAMESPACE environment variable
+	// - CERT_NAMESPACE environment variable
+	// - the namespace of the service referenced in porch's APIService object
+	// - namespace of the current process (if running in a pod)
+	serviceNamespace = os.Getenv("WEBHOOK_SERVICE_NAMESPACE")
+	if serviceNamespace == "" {
+		serviceNamespace = os.Getenv("CERT_NAMESPACE")
+	}
+	if serviceNamespace == "" {
+		serviceNamespace = apiSvcNs
+	}
+	if serviceNamespace == "" {
+		apiSvc, err := util.GetPorchApiServiceKey(ctx)
+		if err == nil {
+			serviceNamespace = apiSvc.Namespace
+		}
+	}
+	if serviceNamespace == "" {
+		var err error
+		serviceNamespace, err = util.GetInClusterNamespace()
+		if err != nil {
+			// this was our last resort, so panic if failed
+			panic(fmt.Sprintf("WEBHOOK_SERVICE_NAMESPACE environment variable is not set, and couldn't deduce its value either: %v", err))
+		}
+	}
+	// theoretically this should never happen, but this is a failsafe
+	if serviceName == "" || serviceNamespace == "" {
+		panic("Couldn't automatically determine a valid value for WEBHOOK_SERVICE_NAME and WEBHOOK_SERVICE_NAMESPACE environment variables. Please set them explicitly!")
+	}
+	return
+}
 
 func setupWebhooks(ctx context.Context) error {
-	cfg := NewWebhookConfig()
+	cfg := newWebhookConfig(ctx)
 	if !cfg.CertManWebhook {
 		caBytes, err := createCerts(cfg)
 		if err != nil {
@@ -253,9 +301,9 @@ func createValidatingWebhook(ctx context.Context, cfg *WebhookConfig, caCert []b
 			Rules: []admissionregistrationv1.RuleWithOperations{{Operations: []admissionregistrationv1.OperationType{
 				admissionregistrationv1.Delete},
 				Rule: admissionregistrationv1.Rule{
-					APIGroups:   []string{"porch.kpt.dev"},
-					APIVersions: []string{"v1alpha1"},
-					Resources:   []string{"packagerevisions"},
+					APIGroups:   []string{porchapi.SchemeGroupVersion.Group},
+					APIVersions: []string{porchapi.SchemeGroupVersion.Version},
+					Resources:   []string{porchapi.PackageRevisionGVR.Resource},
 				},
 			}},
 			AdmissionReviewVersions: []string{"v1", "v1beta1"},
@@ -399,8 +447,7 @@ func validateDeletion(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// Verify that we have a PackageRevision resource
-	pkgRevGVK := metav1.GroupVersionResource{Group: "porch.kpt.dev", Version: "v1alpha1", Resource: "packagerevisions"}
-	if admissionReviewRequest.Request.Resource != pkgRevGVK {
+	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
@@ -413,7 +460,7 @@ func validateDeletion(w http.ResponseWriter, r *http.Request) {
 		writeErr(errMsg, &w)
 		return
 	}
-	pr := v1alpha1.PackageRevision{}
+	pr := porchapi.PackageRevision{}
 	if err := porchClient.Get(context.Background(), client.ObjectKey{
 		Namespace: admissionReviewRequest.Request.Namespace,
 		Name:      admissionReviewRequest.Request.Name,
@@ -422,7 +469,7 @@ func validateDeletion(w http.ResponseWriter, r *http.Request) {
 	}
 
 	admissionResponse := &admissionv1.AdmissionResponse{}
-	if pr.Spec.Lifecycle == v1alpha1.PackageRevisionLifecyclePublished {
+	if pr.Spec.Lifecycle == porchapi.PackageRevisionLifecyclePublished {
 		admissionResponse.Allowed = false
 		admissionResponse.Result = &metav1.Status{
 			Status:  "Failure",

pkg/util/util.go

@@ -15,10 +15,19 @@
 package util
 
 import (
+	"context"
 	"crypto/sha1"
 	"encoding/hex"
 	"fmt"
 	"os"
+
+	porchapi "github.com/nephio-project/porch/api/porch/v1alpha1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	registrationapi "k8s.io/kube-aggregator/pkg/apis/apiregistration/v1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/config"
 )
 
 // KubernetesName returns the passed id if it less than maxLen, otherwise
@@ -45,7 +54,49 @@ func KubernetesName(id string, hashLen, maxLen int) string {
 func GetInClusterNamespace() (string, error) {
 	ns, err := os.ReadFile("/var/run/secrets/kubernetes.io/serviceaccount/namespace")
 	if err != nil {
-		return "", fmt.Errorf("failed to read namespace: %w", err)
+		return "", fmt.Errorf("failed to read in-cluster namespace: %w", err)
 	}
 	return string(ns), nil
 }
+
+func GetPorchApiServiceKey(ctx context.Context) (client.ObjectKey, error) {
+	cfg, err := config.GetConfig()
+	if err != nil {
+		return client.ObjectKey{}, fmt.Errorf("failed to get K8s config: %w", err)
+	}
+
+	scheme := runtime.NewScheme()
+	err = registrationapi.AddToScheme(scheme)
+	if err != nil {
+		return client.ObjectKey{}, fmt.Errorf("failed to add apiregistration API to scheme: %w", err)
+	}
+
+	c, err := client.New(cfg, client.Options{
+		Scheme: scheme,
+	})
+	if err != nil {
+		return client.ObjectKey{}, fmt.Errorf("failed to create K8s client: %w", err)
+	}
+
+	apiSvc := registrationapi.APIService{}
+	apiSvcName := porchapi.SchemeGroupVersion.Version + "." + porchapi.SchemeGroupVersion.Group
+	err = c.Get(ctx, client.ObjectKey{
+		Name: apiSvcName,
+	}, &apiSvc)
+	if err != nil {
+		return client.ObjectKey{}, fmt.Errorf("failed to get APIService %q: %w", apiSvcName, err)
+	}
+
+	return client.ObjectKey{
+		Namespace: apiSvc.Spec.Service.Namespace,
+		Name:      apiSvc.Spec.Service.Name,
+	}, nil
+}
+
+func SchemaToMetaGVR(gvr schema.GroupVersionResource) metav1.GroupVersionResource {
+	return metav1.GroupVersionResource{
+		Group:    gvr.Group,
+		Version:  gvr.Version,
+		Resource: gvr.Resource,
+	}
+}

test/e2e/suite.go

@@ -158,7 +158,7 @@ func (t *TestSuite) IsPorchServerInCluster() bool {
 	porch := aggregatorv1.APIService{}
 	ctx := context.TODO()
 	t.GetF(ctx, client.ObjectKey{
-		Name: "v1alpha1.porch.kpt.dev",
+		Name: porchapi.SchemeGroupVersion.Version + "." + porchapi.SchemeGroupVersion.Group,
 	}, &porch)
 	service := coreapi.Service{}
 	t.GetF(ctx, client.ObjectKey{
@@ -173,7 +173,7 @@ func (t *TestSuite) IsTestRunnerInCluster() bool {
 	porch := aggregatorv1.APIService{}
 	ctx := context.TODO()
 	t.GetF(ctx, client.ObjectKey{
-		Name: "v1alpha1.porch.kpt.dev",
+		Name: porchapi.SchemeGroupVersion.Version + "." + porchapi.SchemeGroupVersion.Group,
 	}, &porch)
 	service := coreapi.Service{}
 	err := t.client.Get(ctx, client.ObjectKey{