Merge pull request #845 from tkashem/audit-config

Audit policy config observer

Author: openshift-merge-robot

go.mod

@@ -36,7 +36,7 @@ require (
 	github.com/opencontainers/go-digest v1.0.0-rc1
 	github.com/opencontainers/image-spec v1.0.1 // indirect
 	github.com/opencontainers/runc v0.0.0-20191031171055-b133feaeeb2e // indirect
-	github.com/openshift/api v0.0.0-20200722170803-0ba2c3658da6
+	github.com/openshift/api v0.0.0-20200723134351-89de68875e7c
 	github.com/openshift/build-machinery-go v0.0.0-20200713135615-1f43d26dccc7
 	github.com/openshift/client-go v0.0.0-20200722173614-5a1b0aaeff15
 	github.com/pkg/errors v0.9.1

go.sum

@@ -396,6 +396,8 @@ github.com/opencontainers/runc v0.0.0-20191031171055-b133feaeeb2e h1:NKMVwQeEqNO
 github.com/opencontainers/runc v0.0.0-20191031171055-b133feaeeb2e/go.mod h1:qT5XzbpPznkRYVz/mWwUaVBUv2rmF59PVA73FjuZG0U=
 github.com/openshift/api v0.0.0-20200722170803-0ba2c3658da6 h1:h2zOwAA/Zg7mc9d16q6W7/mcJppctFyqvHeE6vz1Qys=
 github.com/openshift/api v0.0.0-20200722170803-0ba2c3658da6/go.mod h1:IXsT3F4NjLtRzfnQvwU+g/oPWpoNsVV5vd5aaOMO8eU=
+github.com/openshift/api v0.0.0-20200723134351-89de68875e7c h1:qsj/GaQ1sdT584yIcGmqqRpR5xtX5jTw5Gis3/09YI4=
+github.com/openshift/api v0.0.0-20200723134351-89de68875e7c/go.mod h1:IXsT3F4NjLtRzfnQvwU+g/oPWpoNsVV5vd5aaOMO8eU=
 github.com/openshift/build-machinery-go v0.0.0-20200713135615-1f43d26dccc7 h1:iP7TOaN+tEVNUQ0ODEbN1ukjLz918lsIt7Czf8giWlM=
 github.com/openshift/build-machinery-go v0.0.0-20200713135615-1f43d26dccc7/go.mod h1:b1BuldmJlbA/xYtdZvKi+7j5YGB44qJUJDZ9zwiNCfE=
 github.com/openshift/client-go v0.0.0-20200722173614-5a1b0aaeff15 h1:b2QkHrmaYtY6kzy2VrYLc+KBmCuTpJjgvBahPqpt6V0=

pkg/operator/configobserver/apiserver/observe_audit.go

@@ -0,0 +1,88 @@
+package apiserver
+
+import (
+	"fmt"
+
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/klog/v2"
+
+	"github.com/openshift/library-go/pkg/operator/configobserver"
+	"github.com/openshift/library-go/pkg/operator/events"
+)
+
+// AuditPolicyPathGetterFunc allows the observer to be agnostic of the source of audit profile(s).
+// The function returns the path to the audit policy file (associated with the
+// given profile) in the static manifest folder.
+type AuditPolicyPathGetterFunc func(profile string) (string, error)
+
+// NewAuditObserver returns an ObserveConfigFunc that observes the audit field of the APIServer resource
+// and sets the apiServerArguments:audit-policy-file field for the apiserver appropriately.
+func NewAuditObserver(pathGetter AuditPolicyPathGetterFunc) configobserver.ObserveConfigFunc {
+	var (
+		apiServerArgumentsAuditPath = []string{"apiServerArguments", "audit-policy-file"}
+	)
+
+	return func(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (observed map[string]interface{}, _ []error) {
+		defer func() {
+			observed = configobserver.Pruned(observed, apiServerArgumentsAuditPath)
+		}()
+
+		errs := []error{}
+
+		// if the function encounters an error it returns existing/current config, which means that
+		// some other entity (default config in bindata ) must ensure to default the configuration.
+		// otherwise, the apiserver won't have a path to audit policy file and it will fail to start.
+		listers := genericListers.(APIServerLister)
+		apiServer, err := listers.APIServerLister().Get("cluster")
+		if err != nil {
+			if k8serrors.IsNotFound(err) {
+				klog.Warningf("apiserver.config.openshift.io/cluster: not found")
+
+				return existingConfig, errs
+			}
+
+			return existingConfig, append(errs, err)
+		}
+
+		desiredProfile := string(apiServer.Spec.Audit.Profile)
+		if len(desiredProfile) == 0 {
+			// The specified Profile is empty, so let the defaulting layer choose a default for us.
+			return map[string]interface{}{}, errs
+		}
+
+		desiredAuditPolicyPath, err := pathGetter(desiredProfile)
+		if err != nil {
+			return existingConfig, append(errs, fmt.Errorf("audit profile is not valid name=%s", desiredProfile))
+		}
+
+		currentAuditPolicyPath, err := getCurrentPolicyPath(existingConfig, apiServerArgumentsAuditPath...)
+		if err != nil {
+			return existingConfig, append(errs, fmt.Errorf("audit profile is not valid name=%s", desiredProfile))
+		}
+		if desiredAuditPolicyPath == currentAuditPolicyPath {
+			return existingConfig, errs
+		}
+
+		// we have a change of audit policy here!
+		observedConfig := map[string]interface{}{}
+		if err := unstructured.SetNestedStringSlice(observedConfig, []string{desiredAuditPolicyPath}, apiServerArgumentsAuditPath...); err != nil {
+			return existingConfig, append(errs, fmt.Errorf("failed to set desired audit profile in observed config name=%s", desiredProfile))
+		}
+
+		recorder.Eventf("ObserveAPIServerArgumentsAudit", "audit policy has been set to profile=%s", desiredProfile)
+		return observedConfig, errs
+	}
+}
+
+func getCurrentPolicyPath(existing map[string]interface{}, fields ...string) (string, error) {
+	current, _, err := unstructured.NestedStringSlice(existing, fields...)
+	if err != nil {
+		return "", err
+	}
+	if len(current) == 0 {
+		return "", nil
+	}
+
+	return current[0], nil
+}

pkg/operator/configobserver/apiserver/observe_audit_test.go

@@ -0,0 +1,218 @@
+package apiserver
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/client-go/tools/cache"
+
+	configv1 "github.com/openshift/api/config/v1"
+	configlistersv1 "github.com/openshift/client-go/config/listers/config/v1"
+	"github.com/openshift/library-go/pkg/operator/events"
+)
+
+var (
+	auditPolicyFilePath = []string{"apiServerArguments", "audit-policy-file"}
+)
+
+func TestAuditObserver(t *testing.T) {
+	tests := []struct {
+		name           string
+		existingConfig map[string]interface{}
+		desiredProfile *string
+		expectedPath   string
+		errExpected    bool
+	}{
+		{
+			name:           "WithCurrentAndDesiredBothEmpty",
+			existingConfig: map[string]interface{}{},
+			desiredProfile: stringPointer(""),
+			expectedPath:   "",
+		},
+		{
+			name: "WithCurrentSetAndDesiredEmpty",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+			},
+			desiredProfile: stringPointer(""),
+			expectedPath:   "",
+		},
+		{
+			name:           "WithCurrentEmpty",
+			existingConfig: map[string]interface{}{},
+			desiredProfile: stringPointer("WriteRequestBodies"),
+			expectedPath:   path("WriteRequestBodies"),
+		},
+		{
+			name: "WithCurrentAndDesiredAtDifferentValues",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+			},
+			desiredProfile: stringPointer("WriteRequestBodies"),
+			expectedPath:   path("WriteRequestBodies"),
+		},
+		{
+			// we expect the function to return just the keys it is responsible for.
+			name: "WithOtherKeysDropped",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+				"foo": []interface{}{
+					"should not be returned",
+				},
+			},
+			desiredProfile: stringPointer("WriteRequestBodies"),
+			expectedPath:   path("WriteRequestBodies"),
+		},
+		{
+			// if the user specifies an invalid audit profile we expect the current config to be set.
+			name: "WithCurrentSetAndDesiredInvalid",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+			},
+			desiredProfile: stringPointer("NotExist"),
+			expectedPath:   path("AllRequestBodies"),
+			errExpected:    true,
+		},
+		{
+			name:           "WithCurrentEmptyAndDesiredInvalid",
+			existingConfig: map[string]interface{}{},
+			desiredProfile: stringPointer("NotExist"),
+			expectedPath:   "",
+			errExpected:    true,
+		},
+		{
+			name: "WithCurrentAndDesiredBothSame",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+			},
+			desiredProfile: stringPointer("AllRequestBodies"),
+			expectedPath:   path("AllRequestBodies"),
+		},
+		{
+			name: "WithCurrentSetAndAPIServerResourceMissing",
+			existingConfig: map[string]interface{}{
+				"apiServerArguments": map[string]interface{}{
+					"audit-policy-file": []interface{}{
+						path("AllRequestBodies"),
+					},
+				},
+			},
+			expectedPath: path("AllRequestBodies"),
+		},
+		{
+			name:           "WithCurrentNotSetAndAPIServerResourceMissing",
+			existingConfig: map[string]interface{}{},
+			expectedPath:   "",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{})
+			if test.desiredProfile != nil {
+				if err := indexer.Add(&configv1.APIServer{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "cluster",
+					},
+					Spec: configv1.APIServerSpec{
+						Audit: configv1.Audit{
+							Profile: configv1.AuditProfileType(*test.desiredProfile),
+						},
+					},
+				}); err != nil {
+					t.Fatal(err)
+				}
+			}
+			listers := testLister{
+				apiLister: configlistersv1.NewAPIServerLister(indexer),
+			}
+
+			observer := NewAuditObserver(getter)
+			recorder := events.NewInMemoryRecorder(t.Name())
+			for i := 1; i <= 2; i++ {
+				gotConfig, errs := observer(listers, recorder, test.existingConfig)
+
+				if test.errExpected && len(errs) == 0 {
+					t.Errorf("expected errors, got %v", errs)
+				}
+				if !test.errExpected && len(errs) > 0 {
+					t.Errorf("expected no errors, got %v", errs)
+				}
+
+				gotPath := read(t, gotConfig)
+				if test.expectedPath != gotPath {
+					t.Errorf("audit path expected=%s got=%s", test.expectedPath, gotPath)
+				}
+
+				// put the observed config back into existingConfig.
+				if err := unstructured.SetNestedStringSlice(test.existingConfig, []string{gotPath}, auditPolicyFilePath...); err != nil {
+					t.Errorf("failed to put the observed config into the current conig -%s", err)
+				}
+			}
+		})
+	}
+}
+
+func path(profile string) string {
+	return fmt.Sprintf("%s/%s", "/etc/kubernetes/static-pod-resources/configmaps/kube-apiserver-audit-policies",
+		strings.ToLower(profile))
+}
+
+func getter(profile string) (string, error) {
+	if profile == "NotExist" {
+		return "", fmt.Errorf("invalid profile - name=%s", profile)
+	}
+
+	path := path(profile)
+	return path, nil
+}
+
+func stringPointer(s string) *string {
+	p := &s
+	return p
+}
+
+func read(t *testing.T, config map[string]interface{}) string {
+	// we expect only one key returned in the observed config.
+	if len(config) > 1 {
+		t.Fatal("expected observed config to have a single key 'apiServerArguments'")
+	}
+
+	current, found, err := unstructured.NestedStringSlice(config, auditPolicyFilePath...)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if !found {
+		return ""
+	}
+
+	if len(current) != 1 {
+		t.Fatal("expected config to have only audit policy path defined")
+	}
+
+	return current[0]
+}