From 4ca4c8f0f536fd5055a9cfa65144bc306ddcbde2 Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Thu, 22 Jun 2023 13:22:19 -0400 Subject: [PATCH] Fix nil pointer issue with webhook (#966) * update * update go mod * tidy * revert go mod * fix port * move pod test case * downgrade controller-runtime * revert updates * fix nil pointer * add logs * fix var * remove test requirement * fix decoder * fix mutate * fix test case * fix logs * fmt * fix owned pods in mutate * fix test * add logs * add mutations to tests * convert to json for patch * fix up tests * remove nil check * fix logs * add logs * add env vars to webhook tests --- .circleci/config.yml | 3 +- cmd/polaris/webhook.go | 7 +-- pkg/kube/resources_test.go | 2 +- pkg/webhook/mutate.go | 36 +++++++++++++--- pkg/webhook/webhook.go | 43 ++++++++++--------- .../{pod.yaml => failing_test.pod.yaml} | 0 .../passing_test.deployment.yaml | 3 +- test/webhook_cases/passing_test.pod.yaml | 16 +++++++ test/webhook_test.sh | 8 ++-- 9 files changed, 82 insertions(+), 36 deletions(-) rename test/webhook_cases/{pod.yaml => failing_test.pod.yaml} (100%) create mode 100644 test/webhook_cases/passing_test.pod.yaml diff --git a/.circleci/config.yml b/.circleci/config.yml index eb70d11e..0884f0f3 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -114,6 +114,7 @@ jobs: executor: vm steps: - checkout + - *set_environment_variables - *install_k8s - *test_k8s @@ -161,8 +162,6 @@ workflows: only: /.*/ - build_and_push: context: org-global - requires: - - test filters: branches: ignore: /pull\/[0-9]+/ diff --git a/cmd/polaris/webhook.go b/cmd/polaris/webhook.go index 8a0b55a0..167e46cd 100644 --- a/cmd/polaris/webhook.go +++ b/cmd/polaris/webhook.go @@ -53,7 +53,8 @@ var webhookCmd = &cobra.Command{ CertDir: certDir, Port: webhookPort, WebhookServer: webhook.NewServer(webhook.Options{ - CertDir: certDir, + CertDir: certDir, + Port: webhookPort, CertName: "tls.crt", KeyName: "tls.key", }), @@ -74,10 +75,10 @@ var webhookCmd = &cobra.Command{ } if enableValidations { - fwebhook.NewValidateWebhook(mgr, fwebhook.Validator{Config: config, Client: mgr.GetClient()}) + fwebhook.NewValidateWebhook(mgr, config) } if enableMutations { - fwebhook.NewMutateWebhook(mgr, fwebhook.Mutator{Config: config, Client: mgr.GetClient()}) + fwebhook.NewMutateWebhook(mgr, config) } logrus.Infof("Polaris webhook server listening on port %d", webhookPort) if err := mgr.Start(signals.SetupSignalHandler()); err != nil { diff --git a/pkg/kube/resources_test.go b/pkg/kube/resources_test.go index 44b8c713..b8cdd6d1 100644 --- a/pkg/kube/resources_test.go +++ b/pkg/kube/resources_test.go @@ -16,8 +16,8 @@ package kube import ( "bytes" - "fmt" "context" + "fmt" "os" "testing" "time" diff --git a/pkg/webhook/mutate.go b/pkg/webhook/mutate.go index 40e50409..a133eb25 100644 --- a/pkg/webhook/mutate.go +++ b/pkg/webhook/mutate.go @@ -21,6 +21,7 @@ import ( "github.com/fairwindsops/polaris/pkg/mutation" "github.com/sirupsen/logrus" "gomodules.xyz/jsonpatch/v2" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -35,41 +36,66 @@ type Mutator struct { decoder *admission.Decoder } -var _ admission.Handler = &Mutator{} - // NewMutateWebhook creates a mutating admission webhook for the apiType. -func NewMutateWebhook(mgr manager.Manager, mutator Mutator) { +func NewMutateWebhook(mgr manager.Manager, c config.Configuration) { path := "/mutate" + mutator := Mutator{ + Client: mgr.GetClient(), + decoder: admission.NewDecoder(runtime.NewScheme()), + Config: c, + } mgr.GetWebhookServer().Register(path, &webhook.Admission{Handler: &mutator}) } func (m *Mutator) mutate(req admission.Request) ([]jsonpatch.Operation, error) { results, kubeResources, err := GetValidatedResults(req.AdmissionRequest.Kind.Kind, m.decoder, req, m.Config) if err != nil { + logrus.Errorf("Error while validating resource: %v", err) return nil, err } + if results == nil { + logrus.Infof("Not mutating owned pod") + return nil, nil + } patches := mutation.GetMutationsFromResult(results) originalYaml, err := yaml.JSONToYAML(kubeResources.OriginalObjectJSON) if err != nil { + logrus.Errorf("Failed to convert JSON to YAML: %v", err) return nil, err } mutatedYamlStr, err := mutation.ApplyAllMutations(string(originalYaml), patches) if err != nil { + logrus.Errorf("Failed to apply mutations: %v", err) return nil, err } - return jsonpatch.CreatePatch(originalYaml, []byte(mutatedYamlStr)) + + mutatedJson, err := yaml.YAMLToJSON([]byte(mutatedYamlStr)) + if err != nil { + logrus.Errorf("Failed to convert YAML to JSON: %v", err) + return nil, err + } + + ops, err := jsonpatch.CreatePatch(kubeResources.OriginalObjectJSON, mutatedJson) + if err != nil { + logrus.Errorf("Failed to create patch from mutation: %v", err) + return nil, err + } + return ops, nil } // Handle for Validator to run validation checks. func (m *Mutator) Handle(ctx context.Context, req admission.Request) admission.Response { - logrus.Info("Starting request") + logrus.Info("Starting mutation request") patches, err := m.mutate(req) if err != nil { + logrus.Errorf("Error while getting mutations: %v", err) return admission.Errored(403, err) } if patches == nil { + logrus.Infof("No patches generated") return admission.Allowed("Allowed") } + logrus.Infof("Generated %d patches", len(patches)) return admission.Patched("", patches...) } diff --git a/pkg/webhook/webhook.go b/pkg/webhook/webhook.go index 66e3f246..631de110 100644 --- a/pkg/webhook/webhook.go +++ b/pkg/webhook/webhook.go @@ -25,6 +25,7 @@ import ( "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/webhook" @@ -38,19 +39,14 @@ type Validator struct { Config config.Configuration } -// InjectDecoder injects the decoder. -func (v *Validator) InjectDecoder(d *admission.Decoder) error { - logrus.Info("Injecting decoder") - v.decoder = d - return nil -} - -var _ admission.Handler = &Validator{} - // NewValidateWebhook creates a validating admission webhook for the apiType. -func NewValidateWebhook(mgr manager.Manager, validator Validator) { +func NewValidateWebhook(mgr manager.Manager, c config.Configuration) { path := "/validate" - + validator := Validator{ + Client: mgr.GetClient(), + decoder: admission.NewDecoder(runtime.NewScheme()), + Config: c, + } mgr.GetWebhookServer().Register(path, &webhook.Admission{Handler: &validator}) } @@ -60,35 +56,40 @@ func (v *Validator) handleInternal(req admission.Request) (*validator.Result, ku // GetValidatedResults returns the validated results. func GetValidatedResults(kind string, decoder *admission.Decoder, req admission.Request, config config.Configuration) (*validator.Result, kube.GenericResource, error) { - var controller kube.GenericResource + var resource kube.GenericResource var err error if kind == "Pod" { + if decoder == nil { + panic("Decoder is nil!") + } pod := corev1.Pod{} err := decoder.Decode(req, &pod) if err != nil { - return nil, controller, err + logrus.Errorf("Failed to decode pod: %v", err) + return nil, resource, err } if len(pod.ObjectMeta.OwnerReferences) > 0 { logrus.Infof("Allowing owned pod %s/%s to pass through webhook", pod.ObjectMeta.Namespace, pod.ObjectMeta.Name) - return nil, controller, nil + return nil, resource, nil } - controller, err = kube.NewGenericResourceFromPod(pod, pod) + resource, err = kube.NewGenericResourceFromPod(pod, pod) } else { - controller, err = kube.NewGenericResourceFromBytes(req.Object.Raw) + resource, err = kube.NewGenericResourceFromBytes(req.Object.Raw) } if err != nil { - return nil, controller, err + logrus.Errorf("Failed to create resource: %v", err) + return nil, resource, err } - controllerResult, err := validator.ApplyAllSchemaChecks(&config, nil, controller) + resourceResult, err := validator.ApplyAllSchemaChecks(&config, nil, resource) if err != nil { - return nil, controller, err + return nil, resource, err } - return &controllerResult, controller, nil + return &resourceResult, resource, nil } // Handle for Validator to run validation checks. func (v *Validator) Handle(ctx context.Context, req admission.Request) admission.Response { - logrus.Info("Starting request") + logrus.Info("Starting admission request") result, _, err := v.handleInternal(req) if err != nil { logrus.Errorf("Error validating request: %v", err) diff --git a/test/webhook_cases/pod.yaml b/test/webhook_cases/failing_test.pod.yaml similarity index 100% rename from test/webhook_cases/pod.yaml rename to test/webhook_cases/failing_test.pod.yaml diff --git a/test/webhook_cases/passing_test.deployment.yaml b/test/webhook_cases/passing_test.deployment.yaml index d6d66f75..afeb0b87 100644 --- a/test/webhook_cases/passing_test.deployment.yaml +++ b/test/webhook_cases/passing_test.deployment.yaml @@ -17,6 +17,7 @@ spec: containers: - name: nginx image: nginx:1.7.9 + imagePullPolicy: IfNotPresent ports: - containerPort: 80 securityContext: @@ -26,4 +27,4 @@ spec: runAsNonRoot: true capabilities: drop: - - ALL \ No newline at end of file + - ALL diff --git a/test/webhook_cases/passing_test.pod.yaml b/test/webhook_cases/passing_test.pod.yaml new file mode 100644 index 00000000..440c58cc --- /dev/null +++ b/test/webhook_cases/passing_test.pod.yaml @@ -0,0 +1,16 @@ +apiVersion: v1 +kind: Pod +metadata: + name: nginx-2 +spec: + containers: + - name: nginx + image: nginx:1.7.9 + securityContext: + allowPrivilegeEscalation: false + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + capabilities: + drop: + - ALL diff --git a/test/webhook_test.sh b/test/webhook_test.sh index 22fb62a1..0e99f91f 100755 --- a/test/webhook_test.sh +++ b/test/webhook_test.sh @@ -60,7 +60,7 @@ function clean_up() { echo "Uninstalling webhook and webhook config" kubectl delete validatingwebhookconfigurations polaris-webhook --wait=false || true kubectl delete validatingwebhookconfigurations polaris-validate-webhook --wait=false || true - kubectl delete validatingwebhookconfigurations polaris-mutate-webhook --wait=false || true + kubectl delete mutatingwebhookconfigurations polaris-mutate-webhook --wait=false || true kubectl -n polaris delete deploy -l app=polaris --wait=false || true echo -e "\n\nDone cleaning up\n\n" } @@ -82,11 +82,12 @@ kubectl create ns tests echo "Installing a bad deployment" kubectl apply -n scale-test -f ./test/webhook_cases/failing_test.deployment.yaml -echo "Installing the webhook" +echo "Installing the webhook at version $CI_SHA1" helm repo add fairwinds-stable https://charts.fairwinds.com/stable helm install polaris fairwinds-stable/polaris --namespace polaris --create-namespace \ --set dashboard.enable=false \ --set webhook.enable=true \ + --set webhook.mutate=true \ --set image.tag=$CI_SHA1 echo "Waiting for the webhook to come online" @@ -105,6 +106,7 @@ for filename in test/webhook_cases/passing_test.*.yaml; do if ! kubectl apply -n tests -f $filename; then ALL_TESTS_PASSED=0 echo -e "${RED}****Test Failed: Polaris prevented a resource with no configuration issues****${NC}" + kubectl logs -n polaris deploy/polaris-webhook else echo -e "${GREEN}****Test Passed: Polaris correctly allowed this resource****${NC}" fi @@ -118,7 +120,7 @@ for filename in test/webhook_cases/failing_test.*.yaml; do if kubectl apply -n tests -f $filename; then ALL_TESTS_PASSED=0 echo -e "${RED}****Test Failed: Polaris should have prevented this resource due to configuration issues.****${NC}" - kubectl logs -n polaris $(kubectl get po -oname -n polaris | grep webhook) + kubectl logs -n polaris deploy/polaris-webhook else echo -e "${GREEN}****Test Passed: Polaris correctly prevented this resource****${NC}" fi