cache the executor validation results (open-cluster-management-io#165)

* cache the executor validation results

Signed-off-by: zhujian <jiazhu@redhat.com>

* move executor cache controller to auth package

Signed-off-by: zhujian <jiazhu@redhat.com>

* add a binding resource executor mapper to process delete event

Signed-off-by: zhujian <jiazhu@redhat.com>

* initialize caches before starting the cache controller

Signed-off-by: zhujian <jiazhu@redhat.com>

* read enable executor caches from flag

Signed-off-by: zhujian <jiazhu@redhat.com>

* add unit tests for cache store

Signed-off-by: zhujian <jiazhu@redhat.com>

* add unit tests for cache validator

Signed-off-by: zhujian <jiazhu@redhat.com>

* add unit tests for cache controller

Signed-off-by: zhujian <jiazhu@redhat.com>

* read enable executor caches from feature gate

Signed-off-by: zhujian <jiazhu@redhat.com>

* add integration tests for cache controller

Signed-off-by: zhujian <jiazhu@redhat.com>

* add a description doc for the cache package

Signed-off-by: zhujian <jiazhu@redhat.com>

Signed-off-by: zhujian <jiazhu@redhat.com>

go.mod

@@ -21,7 +21,7 @@ require (
 	k8s.io/component-base v0.24.3
 	k8s.io/klog/v2 v2.70.1
 	k8s.io/utils v0.0.0-20220725171434-9bab9ef40391
-	open-cluster-management.io/api v0.9.1-0.20221031031432-8b08a2ec335b
+	open-cluster-management.io/api v0.9.1-0.20221114135427-b57d18bd356a
 	sigs.k8s.io/controller-runtime v0.12.3
 )
 

go.sum

@@ -1240,8 +1240,8 @@ modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=
 modernc.org/strutil v1.0.0/go.mod h1:lstksw84oURvj9y3tn8lGvRxyRC1S2+g5uuIzNfIOBs=
 modernc.org/xc v1.0.0/go.mod h1:mRNCo0bvLjGhHO9WsyuKVU4q0ceiDDDoEeWDJHrNx8I=
-open-cluster-management.io/api v0.9.1-0.20221031031432-8b08a2ec335b h1:ETkAxS9D/I3/DNYPO+FPVoz9BndQw2NJ1nQu6Aw3zXg=
-open-cluster-management.io/api v0.9.1-0.20221031031432-8b08a2ec335b/go.mod h1:+OEARSAl2jIhuLItUcS30UgLA3khmA9ihygLVxzEn+U=
+open-cluster-management.io/api v0.9.1-0.20221114135427-b57d18bd356a h1:dMhmCqGRxUWVF4yUegDtilfYv2Qu+4/ESc4iwGfORTA=
+open-cluster-management.io/api v0.9.1-0.20221114135427-b57d18bd356a/go.mod h1:9KkJPh/zpDevXj2P+zkvSVjC2pq2PQ1JDNLLEes8TEc=
 rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8=
 rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0=
 rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA=

pkg/features/feature.go

@@ -6,9 +6,16 @@ package features
 import (
 	"k8s.io/apimachinery/pkg/util/runtime"
 	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	"k8s.io/component-base/featuregate"
 	ocmfeature "open-cluster-management.io/api/feature"
 )
 
+var (
+	// DefaultSpokeMutableFeatureGate is made up of multiple mutable feature-gates for work agent.
+	DefaultSpokeMutableFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate()
+)
+
 func init() {
+	runtime.Must(DefaultSpokeMutableFeatureGate.Add(ocmfeature.DefaultSpokeWorkFeatureGates))
 	runtime.Must(utilfeature.DefaultMutableFeatureGate.Add(ocmfeature.DefaultHubWorkFeatureGates))
 }

pkg/helper/helper_test.go

@@ -3,6 +3,8 @@ package helper
 import (
 	"context"
 	"encoding/json"
+	"fmt"
+	"reflect"
 	"testing"
 	"time"
 
@@ -19,6 +21,8 @@ import (
 	clienttesting "k8s.io/client-go/testing"
 	fakeworkclient "open-cluster-management.io/api/client/work/clientset/versioned/fake"
 	workapiv1 "open-cluster-management.io/api/work/v1"
+
+	"open-cluster-management.io/work/pkg/spoke/spoketesting"
 )
 
 func newCondition(name, status, reason, message string, lastTransition *metav1.Time) metav1.Condition {
@@ -707,3 +711,85 @@ func TestOwnedByTheWork(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildResourceMeta(t *testing.T) {
+	restMapper := spoketesting.NewFakeRestMapper()
+
+	cases := []struct {
+		name         string
+		index        int
+		obj          runtime.Object
+		expectedErr  error
+		expectedGVR  schema.GroupVersionResource
+		expectedMeta workapiv1.ManifestResourceMeta
+	}{
+		{
+			name:        "object nil",
+			index:       0,
+			obj:         nil,
+			expectedErr: nil,
+			expectedGVR: schema.GroupVersionResource{},
+			expectedMeta: workapiv1.ManifestResourceMeta{
+				Ordinal: int32(0),
+			},
+		},
+		{
+			name:        "secret success",
+			index:       1,
+			obj:         spoketesting.NewUnstructured("v1", "Secret", "ns1", "test"),
+			expectedErr: nil,
+			expectedGVR: schema.GroupVersionResource{
+				Group:    "",
+				Version:  "v1",
+				Resource: "secrets",
+			},
+			expectedMeta: workapiv1.ManifestResourceMeta{
+				Ordinal:   int32(1),
+				Group:     "",
+				Version:   "v1",
+				Kind:      "Secret",
+				Resource:  "secrets",
+				Namespace: "ns1",
+				Name:      "test",
+			},
+		},
+		{
+			name:        "unknow object type",
+			index:       1,
+			obj:         spoketesting.NewUnstructured("test/v1", "NewObject", "ns1", "test"),
+			expectedErr: fmt.Errorf("the server doesn't have a resource type %q", "NewObject"),
+			expectedGVR: schema.GroupVersionResource{},
+			expectedMeta: workapiv1.ManifestResourceMeta{
+				Ordinal:   int32(1),
+				Group:     "test",
+				Version:   "v1",
+				Kind:      "NewObject",
+				Namespace: "ns1",
+				Name:      "test",
+			},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			meta, gvr, err := BuildResourceMeta(c.index, c.obj, restMapper)
+			if c.expectedErr == nil {
+				if err != nil {
+					t.Errorf("Case name: %s, expect error nil, but got %v", c.name, err)
+				}
+			} else if err == nil {
+				t.Errorf("Case name: %s, expect error %s, but got nil", c.name, c.expectedErr)
+			} else if c.expectedErr.Error() != err.Error() {
+				t.Errorf("Case name: %s, expect error %s, but got %s", c.name, c.expectedErr, err)
+			}
+
+			if !reflect.DeepEqual(c.expectedGVR, gvr) {
+				t.Errorf("Case name: %s, expect gvr %v, but got %v", c.name, c.expectedGVR, gvr)
+			}
+
+			if !reflect.DeepEqual(c.expectedMeta, meta) {
+				t.Errorf("Case name: %s, expect meta %v, but got %v", c.name, c.expectedMeta, meta)
+			}
+		})
+	}
+}

pkg/helper/helpers.go

@@ -5,6 +5,7 @@ import (
 	"crypto/sha256"
 	"encoding/json"
 	"fmt"
+	"reflect"
 	"strings"
 	"time"
 
@@ -463,3 +464,46 @@ func OwnedByTheWork(gvr schema.GroupVersionResource,
 
 	return true
 }
+
+// BuildResourceMeta builds manifest resource meta for the object
+func BuildResourceMeta(
+	index int,
+	object runtime.Object,
+	restMapper meta.RESTMapper) (workapiv1.ManifestResourceMeta, schema.GroupVersionResource, error) {
+	resourceMeta := workapiv1.ManifestResourceMeta{
+		Ordinal: int32(index),
+	}
+
+	if object == nil || reflect.ValueOf(object).IsNil() {
+		return resourceMeta, schema.GroupVersionResource{}, nil
+	}
+
+	// set gvk
+	gvk, err := GuessObjectGroupVersionKind(object)
+	if err != nil {
+		return resourceMeta, schema.GroupVersionResource{}, err
+	}
+	resourceMeta.Group = gvk.Group
+	resourceMeta.Version = gvk.Version
+	resourceMeta.Kind = gvk.Kind
+
+	// set namespace/name
+	if accessor, e := meta.Accessor(object); e != nil {
+		err = fmt.Errorf("cannot access metadata of %v: %w", object, e)
+	} else {
+		resourceMeta.Namespace = accessor.GetNamespace()
+		resourceMeta.Name = accessor.GetName()
+	}
+
+	// set resource
+	if restMapper == nil {
+		return resourceMeta, schema.GroupVersionResource{}, err
+	}
+	mapping, err := restMapper.RESTMapping(gvk.GroupKind(), gvk.Version)
+	if err != nil {
+		return resourceMeta, schema.GroupVersionResource{}, fmt.Errorf("the server doesn't have a resource type %q", gvk.Kind)
+	}
+
+	resourceMeta.Resource = mapping.Resource.Resource
+	return resourceMeta, mapping.Resource, err
+}

pkg/spoke/apply/server_side_apply_test.go

@@ -96,7 +96,7 @@ func TestServerSideApply(t *testing.T) {
 				return
 			}
 
-			var ssaConflict = &ServerSideApplyConflictError{}
+			var ssaConflict *ServerSideApplyConflictError
 			if !errors.As(err, &ssaConflict) {
 				t.Errorf("expect serverside apply conflict error, but got %v", err)
 			}

pkg/spoke/auth/auth.go → pkg/spoke/auth/basic/auth.go

@@ -1,4 +1,4 @@
-package auth
+package basic
 
 import (
 	"context"
@@ -18,15 +18,6 @@ import (
 	workapiv1 "open-cluster-management.io/api/work/v1"
 )
 
-// ExecutorValidator validates whether the executor has permission to perform the requests
-// to the local managed cluster
-type ExecutorValidator interface {
-	// Validate whether the work executor subject has permission to perform action on the specific manifest,
-	// if there is no permission will return a kubernetes forbidden error.
-	Validate(ctx context.Context, executor *workapiv1.ManifestWorkExecutor, gvr schema.GroupVersionResource,
-		namespace, name string, ownedByTheWork bool, obj *unstructured.Unstructured) error
-}
-
 type NotAllowedError struct {
 	Err         error
 	RequeueTime time.Duration
@@ -40,15 +31,16 @@ func (e *NotAllowedError) Error() string {
 	return err
 }
 
-func NewExecutorValidator(config *rest.Config, kubeClient kubernetes.Interface) ExecutorValidator {
-	return &sarValidator{
+// NewSARValidator creates a SARValidator
+func NewSARValidator(config *rest.Config, kubeClient kubernetes.Interface) *SarValidator {
+	return &SarValidator{
 		kubeClient:               kubeClient,
 		config:                   config,
 		newImpersonateClientFunc: defaultNewImpersonateClient,
 	}
 }
 
-type sarValidator struct {
+type SarValidator struct {
 	kubeClient               kubernetes.Interface
 	config                   *rest.Config
 	newImpersonateClientFunc newImpersonateClient
@@ -65,13 +57,30 @@ func defaultNewImpersonateClient(config *rest.Config, username string) (dynamic.
 	return dynamic.NewForConfig(&impersonatedConfig)
 }
 
-func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.ManifestWorkExecutor,
+// Validate checks whether the executor has permission to operate the specific gvr resource by
+// sending sar requests to the api server.
+func (v *SarValidator) Validate(ctx context.Context, executor *workapiv1.ManifestWorkExecutor,
 	gvr schema.GroupVersionResource, namespace, name string,
 	ownedByTheWork bool, obj *unstructured.Unstructured) error {
 	if executor == nil {
 		return nil
 	}
 
+	if err := v.ExecutorBasicCheck(executor); err != nil {
+		return err
+	}
+
+	if err := v.CheckSubjectAccessReviews(ctx, executor.Subject.ServiceAccount,
+		gvr, namespace, name, ownedByTheWork); err != nil {
+		return err
+	}
+
+	// subjectaccessreview can not check permission escalation, use an impersonation request to check again
+	return v.CheckEscalation(ctx, executor.Subject.ServiceAccount, gvr, namespace, name, obj)
+}
+
+// ExecutorBasicCheck do some basic checks for the executor
+func (v *SarValidator) ExecutorBasicCheck(executor *workapiv1.ManifestWorkExecutor) error {
 	if executor.Subject.Type != workapiv1.ExecutorSubjectTypeServiceAccount {
 		return fmt.Errorf("only support %s type for the executor", workapiv1.ExecutorSubjectTypeServiceAccount)
 	}
@@ -81,6 +90,13 @@ func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.Manifes
 		return fmt.Errorf("the executor service account is nil")
 	}
 
+	return nil
+}
+
+// CheckSubjectAccessReviews checks if the sa has permission to operate the gvr resource by subjectAccessReview requests
+func (v *SarValidator) CheckSubjectAccessReviews(ctx context.Context, sa *workapiv1.ManifestWorkSubjectServiceAccount,
+	gvr schema.GroupVersionResource, namespace, name string, ownedByTheWork bool) error {
+
 	verbs := []string{"create", "update", "patch", "get"}
 	if ownedByTheWork {
 		// if the resource to be applied is owned by the manifestwork, will check the delete permission in
@@ -112,21 +128,21 @@ func (v *sarValidator) Validate(ctx context.Context, executor *workapiv1.Manifes
 		}
 	}
 
-	if gvr.Group != "rbac.authorization.k8s.io" {
-		return nil
-	}
-	if gvr.Resource == "roles" || gvr.Resource == "rolebindings" ||
-		gvr.Resource == "clusterroles" || gvr.Resource == "clusterrolebindings" {
-		// subjectaccessreview can not check permission escalation, use an impersonation request to check again
-		return v.checkEscalation(ctx, sa, gvr, namespace, name, obj)
-	}
-
 	return nil
 }
 
-func (v *sarValidator) checkEscalation(ctx context.Context, sa *workapiv1.ManifestWorkSubjectServiceAccount,
+// CheckEscalation checks whether the sa is escalated to operate the gvr(RBAC) resources.
+func (v *SarValidator) CheckEscalation(ctx context.Context, sa *workapiv1.ManifestWorkSubjectServiceAccount,
 	gvr schema.GroupVersionResource, namespace, name string, obj *unstructured.Unstructured) error {
 
+	if gvr.Group != "rbac.authorization.k8s.io" {
+		return nil
+	}
+	if gvr.Resource != "roles" && gvr.Resource != "rolebindings" &&
+		gvr.Resource != "clusterroles" && gvr.Resource != "clusterrolebindings" {
+		return nil
+	}
+
 	dynamicClient, err := v.newImpersonateClientFunc(v.config, username(sa.Namespace, sa.Name))
 	if err != nil {
 		return err

pkg/spoke/auth/auth_test.go → pkg/spoke/auth/basic/auth_test.go

@@ -1,4 +1,4 @@
-package auth
+package basic
 
 import (
 	"context"
@@ -48,7 +48,7 @@ func TestValidate(t *testing.T) {
 			},
 			expect: fmt.Errorf("the executor service account is nil"),
 		},
-		"forbideen": {
+		"forbidden": {
 			executor: &workapiv1.ManifestWorkExecutor{
 				Subject: workapiv1.ManifestWorkExecutorSubject{
 					Type: workapiv1.ExecutorSubjectTypeServiceAccount,
@@ -102,7 +102,7 @@ func TestValidate(t *testing.T) {
 			return false, nil, nil
 		},
 	)
-	validator := NewExecutorValidator(nil, kubeClient)
+	validator := NewSARValidator(nil, kubeClient)
 	for testName, test := range tests {
 		t.Run(testName, func(t *testing.T) {
 			err := validator.Validate(context.TODO(), test.executor, gvr, test.namespace, test.name, true, nil)
@@ -126,7 +126,7 @@ func TestValidateEscalation(t *testing.T) {
 		obj       *unstructured.Unstructured
 		expect    error
 	}{
-		"forbideen": {
+		"forbidden": {
 			executor: &workapiv1.ManifestWorkExecutor{
 				Subject: workapiv1.ManifestWorkExecutorSubject{
 					Type: workapiv1.ExecutorSubjectTypeServiceAccount,
@@ -187,7 +187,7 @@ func TestValidateEscalation(t *testing.T) {
 			}
 			return false, nil, nil
 		})
-	validator := &sarValidator{
+	validator := &SarValidator{
 		kubeClient: kubeClient,
 		newImpersonateClientFunc: func(config *rest.Config, username string) (dynamic.Interface, error) {
 			return dynamicClient, nil