From 06ca132649e6779b787e86e3fd312a070581c060 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Thu, 22 Aug 2024 18:50:24 +0200 Subject: [PATCH 1/5] More sensible default for webhook environment variables --- pkg/apiserver/webhooks.go | 50 +++++++++++++++++++++++++++++++++++---- pkg/util/util.go | 41 +++++++++++++++++++++++++++++++- 2 files changed, 85 insertions(+), 6 deletions(-) diff --git a/pkg/apiserver/webhooks.go b/pkg/apiserver/webhooks.go index ed60d51b..5819f56a 100644 --- a/pkg/apiserver/webhooks.go +++ b/pkg/apiserver/webhooks.go @@ -37,6 +37,7 @@ import ( "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" @@ -72,7 +73,7 @@ type WebhookConfig struct { CertManWebhook bool } -func NewWebhookConfig() *WebhookConfig { +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 +83,48 @@ 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) + + var apiSvcNs string + cfg.ServiceName = os.Getenv("WEBHOOK_SERVICE_NAME") + if cfg.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 APIservice: %v", err)) + } + cfg.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 + // - porch API service namespace + // - namespace of the current process (if running in a pod) + cfg.ServiceNamespace = os.Getenv("WEBHOOK_SERVICE_NAMESPACE") + if cfg.ServiceNamespace == "" { + cfg.ServiceNamespace = os.Getenv("CERT_NAMESPACE") + } + if cfg.ServiceNamespace == "" { + cfg.ServiceNamespace = apiSvcNs + } + if cfg.ServiceNamespace == "" { + apiSvc, err := util.GetPorchApiServiceKey(ctx) + if err == nil { + cfg.ServiceNamespace = apiSvc.Namespace + } + } + if cfg.ServiceNamespace == "" { + var err error + cfg.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 could determine in-cluster namespace: %v", err)) + } + } + // theoretically this should never happen, but this is a failsafe + if cfg.ServiceName == "" || cfg.ServiceNamespace == "" { + panic("Couldn't automatically determine a valid value for WEBHOOK_SERVICE_NAME and WEBHOOK_SERVICE_NAMESPACE environment variables. Please set them explicitly!") + } cfg.Host = fmt.Sprintf("%s.%s.svc", cfg.ServiceName, cfg.ServiceNamespace) } else { cfg.Type = WebhookTypeUrl @@ -103,7 +143,7 @@ var ( ) func setupWebhooks(ctx context.Context) error { - cfg := NewWebhookConfig() + cfg := NewWebhookConfig(ctx) if !cfg.CertManWebhook { caBytes, err := createCerts(cfg) if err != nil { diff --git a/pkg/util/util.go b/pkg/util/util.go index c851ad05..248aa333 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -15,10 +15,16 @@ package util import ( + "context" "crypto/sha1" "encoding/hex" "fmt" "os" + + "k8s.io/apimachinery/pkg/runtime" + 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 +51,40 @@ 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{} + err = c.Get(ctx, client.ObjectKey{ + Name: "v1alpha1.porch.kpt.dev", + }, &apiSvc) + if err != nil { + return client.ObjectKey{}, fmt.Errorf("failed to get APIService 'v1alpha1.porch.kpt.dev': %w", err) + } + + return client.ObjectKey{ + Namespace: apiSvc.Spec.Service.Namespace, + Name: apiSvc.Spec.Service.Name, + }, nil +} From 6b75964a5fa352cafeeccedc530a20325be961fb Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Fri, 23 Aug 2024 01:49:51 +0200 Subject: [PATCH 2/5] Allow porch-server to read APIService objects --- deployments/porch/5-rbac.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/deployments/porch/5-rbac.yaml b/deployments/porch/5-rbac.yaml index ff2d52c5..94b22e5e 100644 --- a/deployments/porch/5-rbac.yaml +++ b/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", "watch", "list"] # Needed for priority and fairness - apiGroups: ["flowcontrol.apiserver.k8s.io"] resources: ["flowschemas", "prioritylevelconfigurations"] From ae138e7d820da3556328101cb2b50f05f2f9b544 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Fri, 23 Aug 2024 02:48:34 +0200 Subject: [PATCH 3/5] Reduce the number of hardwired string literals --- pkg/apiserver/webhooks.go | 15 +++++++-------- pkg/util/util.go | 16 ++++++++++++++-- test/e2e/suite.go | 4 ++-- 3 files changed, 23 insertions(+), 12 deletions(-) diff --git a/pkg/apiserver/webhooks.go b/pkg/apiserver/webhooks.go index 5819f56a..5128ea86 100644 --- a/pkg/apiserver/webhooks.go +++ b/pkg/apiserver/webhooks.go @@ -35,7 +35,7 @@ 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" @@ -293,9 +293,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"}, @@ -439,8 +439,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 @@ -453,7 +452,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, @@ -462,7 +461,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", diff --git a/pkg/util/util.go b/pkg/util/util.go index 248aa333..5dab0f90 100644 --- a/pkg/util/util.go +++ b/pkg/util/util.go @@ -21,7 +21,10 @@ import ( "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" @@ -76,11 +79,12 @@ func GetPorchApiServiceKey(ctx context.Context) (client.ObjectKey, error) { } apiSvc := registrationapi.APIService{} + apiSvcName := porchapi.SchemeGroupVersion.Version + "." + porchapi.SchemeGroupVersion.Group err = c.Get(ctx, client.ObjectKey{ - Name: "v1alpha1.porch.kpt.dev", + Name: apiSvcName, }, &apiSvc) if err != nil { - return client.ObjectKey{}, fmt.Errorf("failed to get APIService 'v1alpha1.porch.kpt.dev': %w", err) + return client.ObjectKey{}, fmt.Errorf("failed to get APIService %q: %w", apiSvcName, err) } return client.ObjectKey{ @@ -88,3 +92,11 @@ func GetPorchApiServiceKey(ctx context.Context) (client.ObjectKey, error) { 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, + } +} diff --git a/test/e2e/suite.go b/test/e2e/suite.go index 6a37e930..c55d0dbd 100644 --- a/test/e2e/suite.go +++ b/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{ From f0ef6f5111c18d92e51334ff51a0d6d248ba6d26 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Wed, 28 Aug 2024 14:05:47 +0200 Subject: [PATCH 4/5] Changes suggested by review comments --- deployments/porch/5-rbac.yaml | 2 +- pkg/apiserver/webhooks.go | 108 ++++++++++++++++++---------------- 2 files changed, 59 insertions(+), 51 deletions(-) diff --git a/deployments/porch/5-rbac.yaml b/deployments/porch/5-rbac.yaml index 94b22e5e..8346c83c 100644 --- a/deployments/porch/5-rbac.yaml +++ b/deployments/porch/5-rbac.yaml @@ -38,7 +38,7 @@ rules: verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] - apiGroups: ["apiregistration.k8s.io"] resources: ["apiservices"] - verbs: ["get", "watch", "list"] + verbs: ["get"] # Needed for priority and fairness - apiGroups: ["flowcontrol.apiserver.k8s.io"] resources: ["flowschemas", "prioritylevelconfigurations"] diff --git a/pkg/apiserver/webhooks.go b/pkg/apiserver/webhooks.go index 5128ea86..1a736394 100644 --- a/pkg/apiserver/webhooks.go +++ b/pkg/apiserver/webhooks.go @@ -50,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 @@ -73,6 +75,7 @@ type WebhookConfig struct { CertManWebhook bool } +// 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. @@ -83,48 +86,7 @@ func NewWebhookConfig(ctx context.Context) *WebhookConfig { !hasEnv("WEBHOOK_HOST") { cfg.Type = WebhookTypeService - - var apiSvcNs string - cfg.ServiceName = os.Getenv("WEBHOOK_SERVICE_NAME") - if cfg.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 APIservice: %v", err)) - } - cfg.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 - // - porch API service namespace - // - namespace of the current process (if running in a pod) - cfg.ServiceNamespace = os.Getenv("WEBHOOK_SERVICE_NAMESPACE") - if cfg.ServiceNamespace == "" { - cfg.ServiceNamespace = os.Getenv("CERT_NAMESPACE") - } - if cfg.ServiceNamespace == "" { - cfg.ServiceNamespace = apiSvcNs - } - if cfg.ServiceNamespace == "" { - apiSvc, err := util.GetPorchApiServiceKey(ctx) - if err == nil { - cfg.ServiceNamespace = apiSvc.Namespace - } - } - if cfg.ServiceNamespace == "" { - var err error - cfg.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 could determine in-cluster namespace: %v", err)) - } - } - // theoretically this should never happen, but this is a failsafe - if cfg.ServiceName == "" || cfg.ServiceNamespace == "" { - panic("Couldn't automatically determine a valid value for WEBHOOK_SERVICE_NAME and WEBHOOK_SERVICE_NAMESPACE environment variables. Please set them explicitly!") - } + cfg.ServiceName, cfg.ServiceNamespace = WebhookServiceName(ctx) cfg.Host = fmt.Sprintf("%s.%s.svc", cfg.ServiceName, cfg.ServiceNamespace) } else { cfg.Type = WebhookTypeUrl @@ -137,10 +99,56 @@ func NewWebhookConfig(ctx context.Context) *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(ctx) From 8a3a3ccdb88f88ccb44593e0b0ebcf5986ff7430 Mon Sep 17 00:00:00 2001 From: Istvan Kispal Date: Wed, 28 Aug 2024 17:36:20 +0200 Subject: [PATCH 5/5] Make package internal functions private --- pkg/apiserver/webhooks.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/apiserver/webhooks.go b/pkg/apiserver/webhooks.go index 1a736394..c898d255 100644 --- a/pkg/apiserver/webhooks.go +++ b/pkg/apiserver/webhooks.go @@ -75,8 +75,8 @@ type WebhookConfig struct { CertManWebhook bool } -// NewWebhookConfig creates a new WebhookConfig object filled with values read from environment variables -func NewWebhookConfig(ctx context.Context) *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. @@ -86,7 +86,7 @@ func NewWebhookConfig(ctx context.Context) *WebhookConfig { !hasEnv("WEBHOOK_HOST") { cfg.Type = WebhookTypeService - cfg.ServiceName, cfg.ServiceNamespace = WebhookServiceName(ctx) + cfg.ServiceName, cfg.ServiceNamespace = webhookServiceName(ctx) cfg.Host = fmt.Sprintf("%s.%s.svc", cfg.ServiceName, cfg.ServiceNamespace) } else { cfg.Type = WebhookTypeUrl @@ -99,8 +99,8 @@ func NewWebhookConfig(ctx context.Context) *WebhookConfig { return &cfg } -// WebhookServiceName returns the name and namespace of Kubernetes service belonging to the webhook -func WebhookServiceName(ctx context.Context) (serviceName, serviceNamespace string) { +// 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: @@ -151,7 +151,7 @@ func WebhookServiceName(ctx context.Context) (serviceName, serviceNamespace stri } func setupWebhooks(ctx context.Context) error { - cfg := NewWebhookConfig(ctx) + cfg := newWebhookConfig(ctx) if !cfg.CertManWebhook { caBytes, err := createCerts(cfg) if err != nil {