From f1bcfa634cd88cb828efe0ab030f298daa0539ce Mon Sep 17 00:00:00 2001 From: YuChen Date: Wed, 27 Nov 2024 12:40:43 -0800 Subject: [PATCH 1/4] read catalogsource permission only for non-private ctrsource Signed-off-by: YuChen --- controllers/operator/manager.go | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/controllers/operator/manager.go b/controllers/operator/manager.go index d7a4adf8..6ecf4ae0 100644 --- a/controllers/operator/manager.go +++ b/controllers/operator/manager.go @@ -27,6 +27,7 @@ import ( operatorsv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1" "github.com/pkg/errors" "golang.org/x/mod/semver" + authorizationv1 "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -172,11 +173,19 @@ func (m *ODLMOperator) GetCatalogSourceAndChannelFromPackage(ctx context.Context if excludedCatalogSources != nil && util.Contains(excludedCatalogSources, pm.Status.CatalogSource) { continue } + + hasCatalogPermission := m.CheckResAuth(ctx, namespace, "operators.coreos.com", "catalogsources", "get") + if !hasCatalogPermission { + klog.V(2).Infof("No permission to get CatalogSource %s in the namespace %s", pm.Status.CatalogSource, pm.Status.CatalogSourceNamespace) + continue + } + // Fetch the CatalogSource if cluster permission allows catalogsource := &olmv1alpha1.CatalogSource{} if err := m.Reader.Get(ctx, types.NamespacedName{Name: pm.Status.CatalogSource, Namespace: pm.Status.CatalogSourceNamespace}, catalogsource); err != nil { klog.Warning(err) continue } + currentCatalog := CatalogSource{ Name: pm.Status.CatalogSource, Namespace: pm.Status.CatalogSourceNamespace, @@ -220,6 +229,28 @@ func (m *ODLMOperator) GetCatalogSourceAndChannelFromPackage(ctx context.Context } } +func (m *ODLMOperator) CheckResAuth(ctx context.Context, namespace, group, resource, verb string) bool { + sar := &authorizationv1.SubjectAccessReview{ + Spec: authorizationv1.SubjectAccessReviewSpec{ + ResourceAttributes: &authorizationv1.ResourceAttributes{ + Namespace: namespace, + Group: group, + Resource: resource, + Verb: verb, + }, + }, + } + if err := m.Create(ctx, sar); err != nil { + return false + } + + if !sar.Status.Allowed { + return false + } + + return true +} + func channelCheck(channelName string, channelList []operatorsv1.PackageChannel) bool { for _, channel := range channelList { if channelName == channel.Name { From c8a5f6bcf3da6ea2e3ff8271ec484d84f5af3e86 Mon Sep 17 00:00:00 2001 From: YuChen Date: Thu, 28 Nov 2024 14:57:41 -0800 Subject: [PATCH 2/4] add test case for checkResAuth function in manager.go Signed-off-by: YuChen --- controllers/operator/manager_test.go | 64 ++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/controllers/operator/manager_test.go b/controllers/operator/manager_test.go index 4d55b5e6..72f5ee31 100644 --- a/controllers/operator/manager_test.go +++ b/controllers/operator/manager_test.go @@ -18,13 +18,19 @@ package operator import ( "context" + "fmt" "reflect" "testing" olmv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" operatorsv1 "github.com/operator-framework/operator-lifecycle-manager/pkg/package-server/apis/operators/v1" + "github.com/stretchr/testify/mock" + authorizationv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/rest" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestChannelCheck(t *testing.T) { @@ -400,6 +406,64 @@ func TestGetCatalogSourceAndChannelFromPackage(t *testing.T) { } +type MockClient struct { + mock.Mock +} + +func (m *MockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { + args := m.Called(ctx, key, obj) + return args.Error(0) +} + +func (m *MockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + args := m.Called(ctx, list, opts) + return args.Error(0) +} + +func TestCheckResAuth(t *testing.T) { + ctx := context.TODO() + + mockClient := &MockClient{} + operator := &ODLMOperator{ + Client: fake.NewClientBuilder().Build(), + Reader: mockClient, // Using the same mock for Reader + Config: &rest.Config{}, + Recorder: record.NewFakeRecorder(10), + } + + namespace := "test-namespace" + group := "test-group" + resource := "test-resource" + verb := "get" + + // Test when SubjectAccessReview is allowed + mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + sar := args.Get(1).(*authorizationv1.SubjectAccessReview) + sar.Status.Allowed = true + }) + + if !operator.CheckResAuth(ctx, namespace, group, resource, verb) { + t.Errorf("Expected CheckResAuth to return true, but got false") + } + + // Test when SubjectAccessReview is not allowed + mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + sar := args.Get(1).(*authorizationv1.SubjectAccessReview) + sar.Status.Allowed = false + }) + + if operator.CheckResAuth(ctx, namespace, group, resource, verb) { + t.Errorf("Expected CheckResAuth to return false, but got true") + } + + // Test when Create returns an error + mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(fmt.Errorf("create error")) + + if operator.CheckResAuth(ctx, namespace, group, resource, verb) { + t.Errorf("Expected CheckResAuth to return false, but got true") + } +} + func assertCatalogSourceAndChannel(t *testing.T, catalogSourceName, expectedCatalogSourceName, catalogSourceNs, expectedCatalogSourceNs, availableChannel, expectedAvailableChannel string) { t.Helper() From 6c7c574d5595233e48ef4d71d1a6bf61535bd6a3 Mon Sep 17 00:00:00 2001 From: YuChen Date: Thu, 28 Nov 2024 14:58:47 -0800 Subject: [PATCH 3/4] add create mock test or ODLMoperator Signed-off-by: YuChen --- controllers/operator/manager_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/controllers/operator/manager_test.go b/controllers/operator/manager_test.go index 72f5ee31..b40b6a63 100644 --- a/controllers/operator/manager_test.go +++ b/controllers/operator/manager_test.go @@ -224,6 +224,7 @@ func TestGetFirstAvailableSemverChannelFromCatalog(t *testing.T) { type MockReader struct { PackageManifestList *operatorsv1.PackageManifestList CatalogSourceList *olmv1alpha1.CatalogSourceList + CreatedObjects map[string]client.Object } func (m *MockReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { @@ -248,6 +249,20 @@ func (m *MockReader) List(ctx context.Context, list client.ObjectList, opts ...c return nil } +func (m *MockReader) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if m.CreatedObjects == nil { + m.CreatedObjects = make(map[string]client.Object) + } + + // Generate a unique key based on the object's name and namespace + key := fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()) + + // Store the object in the map + m.CreatedObjects[key] = obj + + return nil +} + func TestGetCatalogSourceAndChannelFromPackage(t *testing.T) { ctx := context.TODO() From 8a0208b549d18f904fb66bbb5b8a964f2dc9dfdd Mon Sep 17 00:00:00 2001 From: YuChen Date: Fri, 29 Nov 2024 07:23:39 -0800 Subject: [PATCH 4/4] update test case Signed-off-by: YuChen --- controllers/operator/manager_test.go | 51 ++++++++++------------------ 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/controllers/operator/manager_test.go b/controllers/operator/manager_test.go index b40b6a63..221469e3 100644 --- a/controllers/operator/manager_test.go +++ b/controllers/operator/manager_test.go @@ -27,10 +27,7 @@ import ( "github.com/stretchr/testify/mock" authorizationv1 "k8s.io/api/authorization/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestChannelCheck(t *testing.T) { @@ -224,7 +221,6 @@ func TestGetFirstAvailableSemverChannelFromCatalog(t *testing.T) { type MockReader struct { PackageManifestList *operatorsv1.PackageManifestList CatalogSourceList *olmv1alpha1.CatalogSourceList - CreatedObjects map[string]client.Object } func (m *MockReader) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { @@ -249,20 +245,6 @@ func (m *MockReader) List(ctx context.Context, list client.ObjectList, opts ...c return nil } -func (m *MockReader) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { - if m.CreatedObjects == nil { - m.CreatedObjects = make(map[string]client.Object) - } - - // Generate a unique key based on the object's name and namespace - key := fmt.Sprintf("%s/%s", obj.GetNamespace(), obj.GetName()) - - // Store the object in the map - m.CreatedObjects[key] = obj - - return nil -} - func TestGetCatalogSourceAndChannelFromPackage(t *testing.T) { ctx := context.TODO() @@ -323,10 +305,18 @@ func TestGetCatalogSourceAndChannelFromPackage(t *testing.T) { CatalogSourceList: CatalogSourceList, } + mockClient := &MockClient{} + operator := &ODLMOperator{ Reader: fakeReader, + Client: mockClient, } + mockClient.mock.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + sar := args.Get(1).(*authorizationv1.SubjectAccessReview) + sar.Status.Allowed = true + }) + registryNs := "registry-namespace" odlmCatalog := "odlm-catalog" odlmCatalogNs := "odlm-namespace" @@ -422,16 +412,12 @@ func TestGetCatalogSourceAndChannelFromPackage(t *testing.T) { } type MockClient struct { - mock.Mock -} - -func (m *MockClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error { - args := m.Called(ctx, key, obj) - return args.Error(0) + mock mock.Mock + client.Client } -func (m *MockClient) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - args := m.Called(ctx, list, opts) +func (m *MockClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + args := m.mock.Called(ctx, obj, opts) return args.Error(0) } @@ -440,10 +426,7 @@ func TestCheckResAuth(t *testing.T) { mockClient := &MockClient{} operator := &ODLMOperator{ - Client: fake.NewClientBuilder().Build(), - Reader: mockClient, // Using the same mock for Reader - Config: &rest.Config{}, - Recorder: record.NewFakeRecorder(10), + Client: mockClient, } namespace := "test-namespace" @@ -452,7 +435,7 @@ func TestCheckResAuth(t *testing.T) { verb := "get" // Test when SubjectAccessReview is allowed - mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + mockClient.mock.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { sar := args.Get(1).(*authorizationv1.SubjectAccessReview) sar.Status.Allowed = true }) @@ -462,7 +445,8 @@ func TestCheckResAuth(t *testing.T) { } // Test when SubjectAccessReview is not allowed - mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { + mockClient.mock.ExpectedCalls = nil + mockClient.mock.On("Create", ctx, mock.Anything, mock.Anything).Return(nil).Run(func(args mock.Arguments) { sar := args.Get(1).(*authorizationv1.SubjectAccessReview) sar.Status.Allowed = false }) @@ -472,7 +456,8 @@ func TestCheckResAuth(t *testing.T) { } // Test when Create returns an error - mockClient.On("Create", ctx, mock.Anything, mock.Anything).Return(fmt.Errorf("create error")) + mockClient.mock.ExpectedCalls = nil + mockClient.mock.On("Create", ctx, mock.Anything, mock.Anything).Return(fmt.Errorf("create error")) if operator.CheckResAuth(ctx, namespace, group, resource, verb) { t.Errorf("Expected CheckResAuth to return false, but got true")