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

@@ -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]+/

checks/metadataAndNameMismatched.yaml

@@ -1,5 +1,6 @@
 successMessage: Label app.kubernetes.io/name matches metadata.name
 failureMessage: Label app.kubernetes.io/name must match metadata.name
+category: Reliability
 target: Controller
 schema:
   '$schema': http://json-schema.org/draft-07/schema

checks/priorityClassNotSet.yaml

@@ -1,6 +1,6 @@
 successMessage: Priority class has been set
 failureMessage: Priority class should be set
-category: Security
+category: Reliability
 target: PodSpec
 schema:
   '$schema': http://json-schema.org/draft-07/schema

cmd/polaris/audit.go

@@ -173,7 +173,7 @@ var auditCmd = &cobra.Command{
 				os.Exit(1)
 			}
 			logrus.Println("Success! You can see your results at:")
-			logrus.Printf("%s/orgs/%s/clusters/%s\n", insightsHost, auth.Organization, clusterName)
+			logrus.Printf("%s/orgs/%s/clusters/%s/action-items\n", insightsHost, auth.Organization, clusterName)
 		} else {
 			outputAudit(auditData, auditOutputFile, auditOutputURL, auditOutputFormat, useColor, onlyShowFailedTests)
 		}

cmd/polaris/webhook.go

@@ -53,6 +53,8 @@ var webhookCmd = &cobra.Command{
 			CertDir: certDir,
 			Port:    webhookPort,
 			WebhookServer: webhook.NewServer(webhook.Options{
+				CertDir:  certDir,
+				Port:     webhookPort,
 				CertName: "tls.crt",
 				KeyName:  "tls.key",
 			}),
@@ -73,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 {

docs/admission-controller.md

@@ -53,7 +53,7 @@ This means Polaris will remediate the issue it finds, rather than rejecting
 the deployment.
 
 To enable the mutating webhook, add `--set webhook.mutate=true` to your
-Helm instlallation command.
+Helm installation command.
 
 The following default checks currently have mutation support enabled:
 * `hostPIDSet`

docs/changelog.md

@@ -6,6 +6,10 @@ meta:
 
 ---
 
+## 8.1.1
+* Add category for `metadataAndNameMismatched`.
+* Fix category for `priorityClassNotSet`.
+
 ## 8.1.0
 * Add `insights-host` global flag to configure Fairwinds Insights host (defaults to `https://insights.fairwinds.com`).
 * Add new `auth` sub-commands be able to authenticate on Polaris using Fairwinds Insights credentials

docs/checks/reliability.md

@@ -16,8 +16,8 @@ key | default | description
 `pullPolicyNotAlways` | `warning` | Fails when an image pull policy is not `always`.
 `priorityClassNotSet` | `warning` | Fails when a priorityClassName is not set for a pod.
 `deploymentMissingReplicas` | `warning` | Fails when there is only one replica for a deployment.
-`missingPodDisruptionBudget` | `warning`
-`metadataAndNameMismatched` | `warning`
+`missingPodDisruptionBudget` | `warning` | Fails when PDB is missing.
+`metadataAndNameMismatched` | `warning` | Fails when label `app.kubernetes.io/name` and `metadata.name` mismatch
 `topologySpreadConstraint` | `warning` | Fails when there is no topology spread constraint on the pod
 
 ## Background
@@ -70,4 +70,4 @@ spec:
 - [Kubernetes Docs: Configure Liveness and Readiness Probes](https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-probes/)
 - [Utilizing Kubernetes Liveness and Readiness Probes to Automatically Recover From Failure](https://medium.com/spire-labs/utilizing-kubernetes-liveness-and-readiness-probes-to-automatically-recover-from-failure-2fe0314f2b2e)
 - [Kubernetes Liveness and Readiness Probes: How to Avoid Shooting Yourself in the Foot](https://blog.colinbreck.com/kubernetes-liveness-and-readiness-probes-how-to-avoid-shooting-yourself-in-the-foot/)
-- [Topology Spread Cosntraints](https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/)
+- [Topology Spread Constraints](https://kubernetes.io/docs/concepts/scheduling-eviction/topology-spread-constraints/)

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/gorilla/mux v1.8.0
 	github.com/pkg/errors v0.9.1
 	github.com/qri-io/jsonschema v0.1.2
-	github.com/sirupsen/logrus v1.9.2
+	github.com/sirupsen/logrus v1.9.3
 	github.com/spf13/cobra v1.7.0
 	github.com/stretchr/testify v1.8.4
 	github.com/thoas/go-funk v0.9.3

go.sum

@@ -334,8 +334,8 @@ github.com/sergi/go-diff v1.0.0 h1:Kpca3qRNrduNnOQeazBd0ysaKrUJiIuISHxogkT9RPQ=
 github.com/sergi/go-diff v1.0.0/go.mod h1:0CfEIISq7TuYL3j771MWULgwwjU+GofnZX9QAmXWZgo=
 github.com/shurcooL/sanitized_anchor_name v1.0.0/go.mod h1:1NzhyTcUVG4SuEtjjoZeVRXNmyL/1OwPU0+IJeTBvfc=
 github.com/sirupsen/logrus v1.8.1/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic61uBYv0=
-github.com/sirupsen/logrus v1.9.2 h1:oxx1eChJGI6Uks2ZC4W1zpLlVgqB8ner4EuQwV4Ik1Y=
-github.com/sirupsen/logrus v1.9.2/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
+github.com/sirupsen/logrus v1.9.3 h1:dueUQJ1C2q9oE3F7wvmSGAaVtTmUizReu6fjN8uqzbQ=
+github.com/sirupsen/logrus v1.9.3/go.mod h1:naHLuLoDiP4jHNo9R0sCBMtWGeIprob74mVsIT4qYEQ=
 github.com/smartystreets/assertions v0.0.0-20180927180507-b2de0cb4f26d/go.mod h1:OnSkiWE9lh6wB0YB77sQom3nweQdgAjqCqsofrRNTgc=
 github.com/smartystreets/goconvey v1.6.4/go.mod h1:syvi0/a8iFYH4r/RixwvyeAJjdLS9QV7WQ/tjFTllLA=
 github.com/spaolacci/murmur3 v0.0.0-20180118202830-f09979ecbc72/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=

pkg/auth/login.go

@@ -85,7 +85,7 @@ func HandleLogin(insightsHost string) error {
 
 	var user, token, organization string
 	if answer == loginUsingBrowser {
-		listener, err := net.Listen("tcp", ":0")
+		listener, err := net.Listen("tcp", "localhost:0")
 		if err != nil {
 			panic(err)
 		}

pkg/config/checks.go

@@ -22,7 +22,6 @@ import (
 var (
 	// BuiltInChecks contains the checks that come pre-installed w/ Polaris
 	BuiltInChecks = map[string]SchemaCheck{}
-	schemaBox     = (*packr.Box)(nil)
 	// We explicitly set the order to avoid thrash in the
 	// tests as we migrate toward JSON schema
 	checkOrder = []string{
@@ -72,7 +71,7 @@ var (
 )
 
 func init() {
-	schemaBox = packr.New("Schemas", "../../checks")
+	schemaBox := packr.New("Schemas", "../../checks")
 	for _, checkID := range checkOrder {
 		contents, err := schemaBox.Find(checkID + ".yaml")
 		if err != nil {

pkg/config/checks_test.go

@@ -0,0 +1,16 @@
+package config
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRequiredFieldsOnBuiltInChecks(t *testing.T) {
+	for _, v := range BuiltInChecks {
+		assert.NotEmpty(t, v.SuccessMessage)
+		assert.NotEmpty(t, v.FailureMessage)
+		assert.NotEmpty(t, v.Category)
+		assert.NotEmpty(t, v.Target)
+	}
+}

pkg/kube/resources_test.go

@@ -16,8 +16,8 @@ package kube
 
 import (
 	"bytes"
-	"fmt"
 	"context"
+	"fmt"
 	"os"
 	"testing"
 	"time"

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...)
 }

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)

test/mutation_test.go

@@ -19,7 +19,7 @@ checks:
   hostNetworkSet: danger
   hostPortSet: warning
   deploymentMissingReplicas: warning
-  priorityClassNotSet: ignore
+  priorityClassNotSet: warning
   runAsRootAllowed: danger
   cpuRequestsMissing: warning
   cpuLimitsMissing: warning

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
+              - ALL

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

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