Repository: rancher/fleet
Pull request #1662 (2023-07-21 14:09:15)

Enforce strict match on cluster name when fetching bundledeployments by cluster
When fetching bundle deployments in a specific cluster for a given bundle, Fleet must only return bundles which live in that cluster. This pull request prevents bundle deployments from other clusters from being returned, by enforcing a strict match on cluster name labels. This takes advantage of #1571.

Refers to #1245

## Test

To test this pull request, you can run the following commands:

```shell
go test ./internal/cmd/controller/target/
```

## Additional Information

### Trade-off

I did not manage to reproduce #1245 , but this should mitigate what we believe to be its root cause.


Patch:
diff --git a/internal/cmd/controller/mocks/bundle_deployment_cache_mock.go b/internal/cmd/controller/mocks/bundle_deployment_cache_mock.go
new file mode 100644
index 00000000..083d98a0
--- /dev/null
+++ b/internal/cmd/controller/mocks/bundle_deployment_cache_mock.go
@@ -0,0 +1,94 @@
+// Code generated by MockGen. DO NOT EDIT.
+// Source: github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1 (interfaces: BundleDeploymentCache)
+
+// Package mocks is a generated GoMock package.
+package mocks
+
+import (
+	reflect "reflect"
+
+	gomock "github.com/golang/mock/gomock"
+	v1alpha1 "github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
+	v1alpha10 "github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1"
+	labels "k8s.io/apimachinery/pkg/labels"
+)
+
+// MockBundleDeploymentCache is a mock of BundleDeploymentCache interface.
+type MockBundleDeploymentCache struct {
+	ctrl     *gomock.Controller
+	recorder *MockBundleDeploymentCacheMockRecorder
+}
+
+// MockBundleDeploymentCacheMockRecorder is the mock recorder for MockBundleDeploymentCache.
+type MockBundleDeploymentCacheMockRecorder struct {
+	mock *MockBundleDeploymentCache
+}
+
+// NewMockBundleDeploymentCache creates a new mock instance.
+func NewMockBundleDeploymentCache(ctrl *gomock.Controller) *MockBundleDeploymentCache {
+	mock := &MockBundleDeploymentCache{ctrl: ctrl}
+	mock.recorder = &MockBundleDeploymentCacheMockRecorder{mock}
+	return mock
+}
+
+// EXPECT returns an object that allows the caller to indicate expected use.
+func (m *MockBundleDeploymentCache) EXPECT() *MockBundleDeploymentCacheMockRecorder {
+	return m.recorder
+}
+
+// AddIndexer mocks base method.
+func (m *MockBundleDeploymentCache) AddIndexer(arg0 string, arg1 v1alpha10.BundleDeploymentIndexer) {
+	m.ctrl.T.Helper()
+	m.ctrl.Call(m, "AddIndexer", arg0, arg1)
+}
+
+// AddIndexer indicates an expected call of AddIndexer.
+func (mr *MockBundleDeploymentCacheMockRecorder) AddIndexer(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddIndexer", reflect.TypeOf((*MockBundleDeploymentCache)(nil).AddIndexer), arg0, arg1)
+}
+
+// Get mocks base method.
+func (m *MockBundleDeploymentCache) Get(arg0, arg1 string) (*v1alpha1.BundleDeployment, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Get", arg0, arg1)
+	ret0, _ := ret[0].(*v1alpha1.BundleDeployment)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// Get indicates an expected call of Get.
+func (mr *MockBundleDeploymentCacheMockRecorder) Get(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Get", reflect.TypeOf((*MockBundleDeploymentCache)(nil).Get), arg0, arg1)
+}
+
+// GetByIndex mocks base method.
+func (m *MockBundleDeploymentCache) GetByIndex(arg0, arg1 string) ([]*v1alpha1.BundleDeployment, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "GetByIndex", arg0, arg1)
+	ret0, _ := ret[0].([]*v1alpha1.BundleDeployment)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// GetByIndex indicates an expected call of GetByIndex.
+func (mr *MockBundleDeploymentCacheMockRecorder) GetByIndex(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetByIndex", reflect.TypeOf((*MockBundleDeploymentCache)(nil).GetByIndex), arg0, arg1)
+}
+
+// List mocks base method.
+func (m *MockBundleDeploymentCache) List(arg0 string, arg1 labels.Selector) ([]*v1alpha1.BundleDeployment, error) {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "List", arg0, arg1)
+	ret0, _ := ret[0].([]*v1alpha1.BundleDeployment)
+	ret1, _ := ret[1].(error)
+	return ret0, ret1
+}
+
+// List indicates an expected call of List.
+func (mr *MockBundleDeploymentCacheMockRecorder) List(arg0, arg1 interface{}) *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "List", reflect.TypeOf((*MockBundleDeploymentCache)(nil).List), arg0, arg1)
+}
diff --git a/internal/cmd/controller/target/target.go b/internal/cmd/controller/target/target.go
index 4dca013b..6bebffc3 100644
--- a/internal/cmd/controller/target/target.go
+++ b/internal/cmd/controller/target/target.go
@@ -25,7 +25,6 @@ import (
 	fleetcontrollers "github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1"
 
 	corecontrollers "github.com/rancher/wrangler/pkg/generated/controllers/core/v1"
-	"github.com/rancher/wrangler/pkg/name"
 	"github.com/rancher/wrangler/pkg/yaml"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -197,19 +196,14 @@ func (m *Manager) GetBundleDeploymentsForBundleInCluster(bundle *fleet.Bundle, c
 	bundleDeployments, err := m.bundleDeploymentCache.List("", labels.SelectorFromSet(labels.Set{
 		fleet.BundleLabel:          bundle.Name,
 		fleet.BundleNamespaceLabel: bundle.Namespace,
+		fleet.ClusterLabel:         cluster.Name,
 	}))
 
 	if err != nil {
 		return nil, err
 	}
-	nsPrefix := name.SafeConcatName("cluster", cluster.Namespace, cluster.Name)
-	for _, bd := range bundleDeployments {
-		if strings.HasPrefix(bd.Namespace, nsPrefix) {
-			result = append(result, bd)
-		}
-	}
 
-	return result, nil
+	return bundleDeployments, nil
 }
 
 // getNamespacesForBundle returns the namespaces that bundledeployments could


Test patch:
diff --git a/.github/workflows/e2e-ci.yml b/.github/workflows/e2e-ci.yml
index 1f54fba1..fa725db3 100644
--- a/.github/workflows/e2e-ci.yml
+++ b/.github/workflows/e2e-ci.yml
@@ -57,16 +57,16 @@ jobs:
 
           go build -o "bin/fleet-linux-$GOARCH" ./cmd/fleetcli
           go build -o "bin/fleetagent-linux-$GOARCH" ./cmd/fleetagent
-          cd e2e/testenv/infra
-          go build -o . ./...
+          # cd e2e/testenv/infra
+          # go build -o . ./...
       -
         name: Build Docker Images
         run: |
           docker build -f package/Dockerfile -t rancher/fleet:dev --build-arg="ARCH=$GOARCH" .
           docker build -f package/Dockerfile.agent -t rancher/fleet-agent:dev --build-arg="ARCH=$GOARCH" .
-          cd e2e/assets/gitrepo
-          # Buildkit needed here for proper here-document support
-          DOCKER_BUILDKIT=1 docker build -f Dockerfile.gitserver -t nginx-git:test --build-arg="passwd=$(openssl passwd foo)" .
+          # cd e2e/assets/gitrepo
+          # # Buildkit needed here for proper here-document support
+          # DOCKER_BUILDKIT=1 docker build -f Dockerfile.gitserver -t nginx-git:test --build-arg="passwd=$(openssl passwd foo)" .
       -
         name: Provision k3d Cluster
         uses: AbsaOSS/k3d-action@v2
@@ -82,7 +82,7 @@ jobs:
         name: Import Images Into k3d
         run: |
           k3d image import rancher/fleet:dev rancher/fleet-agent:dev
-          k3d image import nginx-git:test nginx-git:test
+          #k3d image import nginx-git:test nginx-git:test
       -
         name: Set Up Tmate Debug Session
         if: ${{ github.event_name == 'workflow_dispatch' && github.event.inputs.enable_tmate == 'true' }}
@@ -94,13 +94,13 @@ jobs:
         name: Deploy Fleet
         run: |
           ./.github/scripts/deploy-fleet.sh
-      -
-        name: Create Zot certificates for OCI tests
-        env:
-          FLEET_E2E_NS: fleet-local
-        run: |
-          # Generate cert and key for TLS
-          ./.github/scripts/create-zot-certs.sh "FleetCI-RootCA"
+      # -
+      #   name: Create Zot certificates for OCI tests
+      #   env:
+      #     FLEET_E2E_NS: fleet-local
+      #   run: |
+      #     # Generate cert and key for TLS
+      #     ./.github/scripts/create-zot-certs.sh "FleetCI-RootCA"
       -
         name: E2E Tests
         env:
@@ -112,12 +112,12 @@ jobs:
           CI_OCI_PASSWORD: "foo"
           HELM_PATH: /usr/local/bin/helm
         run: |
-          export CI_OCI_CERTS_DIR="$(git rev-parse --show-toplevel)/FleetCI-RootCA"
-          e2e/testenv/infra/infra setup
+          # export CI_OCI_CERTS_DIR="$(git rev-parse --show-toplevel)/FleetCI-RootCA"
+          # e2e/testenv/infra/infra setup
 
-          ginkgo e2e/single-cluster e2e/keep-resources
+          ginkgo --label-filter="!infra-setup" e2e/single-cluster e2e/keep-resources
 
-          e2e/testenv/infra/infra teardown
+          # e2e/testenv/infra/infra teardown
       -
         name: Acceptance Tests for Examples
         if: >
diff --git a/internal/cmd/controller/controllers/manageagent/manageagent_test.go b/internal/cmd/controller/controllers/manageagent/manageagent_test.go
index fb1c2934..be6dccd6 100644
--- a/internal/cmd/controller/controllers/manageagent/manageagent_test.go
+++ b/internal/cmd/controller/controllers/manageagent/manageagent_test.go
@@ -1,6 +1,6 @@
 package manageagent
 
-//go:generate mockgen --build_flags=--mod=mod -destination=../../../internal/mocks/namespace_mock.go -package=mocks github.com/rancher/wrangler/pkg/generated/controllers/core/v1 NamespaceController
+//go:generate mockgen --build_flags=--mod=mod -destination=../../mocks/namespace_mock.go -package=mocks github.com/rancher/wrangler/pkg/generated/controllers/core/v1 NamespaceController
 
 import (
 	"testing"
diff --git a/internal/cmd/controller/target/target_test.go b/internal/cmd/controller/target/target_test.go
index 7ab9b0ad..d0779f70 100644
--- a/internal/cmd/controller/target/target_test.go
+++ b/internal/cmd/controller/target/target_test.go
@@ -1,11 +1,18 @@
 package target
 
+//go:generate mockgen --build_flags=--mod=mod -destination=../mocks/bundle_deployment_cache_mock.go -package=mocks github.com/rancher/fleet/pkg/generated/controllers/fleet.cattle.io/v1alpha1 BundleDeploymentCache
+
 import (
 	"testing"
 
+	"github.com/golang/mock/gomock"
 	"github.com/pkg/errors"
 	"github.com/rancher/wrangler/pkg/yaml"
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 
+	"github.com/rancher/fleet/internal/cmd/controller/mocks"
 	"github.com/rancher/fleet/pkg/apis/fleet.cattle.io/v1alpha1"
 )
 
@@ -464,3 +471,84 @@ func TestDisablePreProcessFlagMissing(t *testing.T) {
 	}
 
 }
+
+func TestGetBundleDeploymentForBundleInCluster(t *testing.T) {
+	testCases := []struct {
+		name                       string
+		bundleName                 string
+		bundleNamespace            string
+		clusterName                string
+		listBundleDeploymentsError error
+		expectedBundleDeployments  []*v1alpha1.BundleDeployment
+		wantError                  bool
+	}{
+		{
+			name:            "returns listed bundle deployments",
+			bundleName:      "my-bundle-my-cluster",
+			bundleNamespace: "fleet-default",
+			clusterName:     "my-cluster",
+			expectedBundleDeployments: []*v1alpha1.BundleDeployment{
+				{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: "cluster-fleet-default-my-cluster-1df72965a9b5",
+						Labels: map[string]string{
+							"fleet.cattle.io/cluster": "my-cluster",
+						},
+					},
+				},
+			},
+		},
+		{
+			name:                       "returns error from bundle deployment cache listing",
+			bundleName:                 "my-bundle-my-cluster",
+			bundleNamespace:            "fleet-default",
+			listBundleDeploymentsError: errors.New("something happened"),
+			expectedBundleDeployments:  nil,
+			wantError:                  true,
+		},
+		{
+			name:                      "returns no bundle deployments when none are listed",
+			bundleName:                "my-bundle-my-cluster",
+			bundleNamespace:           "fleet-default",
+			clusterName:               "my-cluster",
+			expectedBundleDeployments: nil,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			ctrl := gomock.NewController(t)
+
+			mockBundleDeploymentCache := mocks.NewMockBundleDeploymentCache(ctrl)
+
+			bundle := v1alpha1.Bundle{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      tc.bundleName,
+					Namespace: tc.bundleNamespace,
+				},
+			}
+
+			cluster := v1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: tc.clusterName,
+				},
+			}
+
+			mockBundleDeploymentCache.EXPECT().List("", labels.SelectorFromSet(labels.Set{
+				v1alpha1.BundleLabel:          bundle.Name,
+				v1alpha1.BundleNamespaceLabel: bundle.Namespace,
+				v1alpha1.ClusterLabel:         cluster.Name,
+			})).Return(tc.expectedBundleDeployments, tc.listBundleDeploymentsError)
+
+			manager := New(nil, nil, nil, nil, nil, nil, mockBundleDeploymentCache)
+			result, err := manager.GetBundleDeploymentsForBundleInCluster(&bundle, &cluster)
+
+			if tc.wantError {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+			}
+			assert.Equal(t, tc.expectedBundleDeployments, result)
+		})
+	}
+}