feat: add controller class on VaultDynamicSecret resources (external-…

…secrets#2287) * feat: add generator for vaultdynamicsecret * Added controllerClass on VaultDynamicSecret * Added controllerClass on VaultDynamicSecret Signed-off-by: rdeepc <[email protected]> * Fixed lint Signed-off-by: rdeepc <[email protected]> * Fixed hack bash Signed-off-by: rdeepc <[email protected]> * feat: Implemented generator controller class support - Controller class support in VaultDynamicSecret - Controller class support in Fake Signed-off-by: rdeepc <[email protected]> * feat: Implemented Generator controller class check Signed-off-by: rdeepc <[email protected]> * feat: Implemented Generator controller class check Signed-off-by: rdeepc <[email protected]> * feat: Implemented Generator controller class check Signed-off-by: rdeepc <[email protected]> * feat: hoist controller class check to the top The generator controller class check should be at the very top of the reconcile function just like the other secretStore class check. Otherwise we would return an error and as a result set the status field on the es resource - which is undesirable. The controller should completely ignore the resource instead. Signed-off-by: Moritz Johner <[email protected]> --------- Signed-off-by: rdeepc <[email protected]> Signed-off-by: rdeepc <[email protected]> Signed-off-by: Moritz Johner <[email protected]> Co-authored-by: Frederic Mereu <[email protected]> Co-authored-by: Moritz Johner <[email protected]>
AmadeusITGroup · May 16, 2023 · 08bb229 · 08bb229
1 parent bbddc6f
commit 08bb229
Show file tree

Hide file tree

Showing 16 changed files with 143 additions and 24 deletions.
diff --git a/apis/externalsecrets/v1alpha1/secretstore_types.go b/apis/externalsecrets/v1alpha1/secretstore_types.go
@@ -21,8 +21,8 @@ import (
 
 // SecretStoreSpec defines the desired state of SecretStore.
 type SecretStoreSpec struct {
-	// Used to select the correct KES controller (think: ingress.ingressClassName)
-	// The KES controller is instantiated with a specific controller name and filters ES based on this property
+	// Used to select the correct ESO controller (think: ingress.ingressClassName)
+	// The ESO controller is instantiated with a specific controller name and filters ES based on this property
 	// +optional
 	Controller string `json:"controller"`
 

diff --git a/apis/externalsecrets/v1beta1/secretstore_types.go b/apis/externalsecrets/v1beta1/secretstore_types.go
@@ -21,8 +21,8 @@ import (
 
 // SecretStoreSpec defines the desired state of SecretStore.
 type SecretStoreSpec struct {
-	// Used to select the correct KES controller (think: ingress.ingressClassName)
-	// The KES controller is instantiated with a specific controller name and filters ES based on this property
+	// Used to select the correct ESO controller (think: ingress.ingressClassName)
+	// The ESO controller is instantiated with a specific controller name and filters ES based on this property
 	// +optional
 	Controller string `json:"controller"`
 

diff --git a/apis/generators/v1alpha1/generator_fake.go b/apis/generators/v1alpha1/generator_fake.go
@@ -20,6 +20,11 @@ import (
 
 // FakeSpec contains the static data.
 type FakeSpec struct {
+	// Used to select the correct ESO controller (think: ingress.ingressClassName)
+	// The ESO controller is instantiated with a specific controller name and filters VDS based on this property
+	// +optional
+	Controller string `json:"controller"`
+
 	// Data defines the static data returned
 	// by this generator.
 	Data map[string]string `json:"data,omitempty"`

diff --git a/apis/generators/v1alpha1/generator_types.go b/apis/generators/v1alpha1/generator_types.go
@@ -0,0 +1,21 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+type ControllerClassResource struct {
+	Spec struct {
+		ControllerClass string `json:"controller"`
+	} `json:"spec"`
+}
diff --git a/apis/generators/v1alpha1/generator_vault.go b/apis/generators/v1alpha1/generator_vault.go
@@ -22,6 +22,11 @@ import (
 )
 
 type VaultDynamicSecretSpec struct {
+	// Used to select the correct ESO controller (think: ingress.ingressClassName)
+	// The ESO controller is instantiated with a specific controller name and filters VDS based on this property
+	// +optional
+	Controller string `json:"controller"`
+
 	// Vault API method to use (GET/POST/other)
 	Method string `json:"method,omitempty"`
 

diff --git a/apis/generators/v1alpha1/zz_generated.deepcopy.go b/apis/generators/v1alpha1/zz_generated.deepcopy.go
diff --git a/config/crds/bases/external-secrets.io_clustersecretstores.yaml b/config/crds/bases/external-secrets.io_clustersecretstores.yaml
@@ -47,8 +47,8 @@ spec:
             description: SecretStoreSpec defines the desired state of SecretStore.
             properties:
               controller:
-                description: 'Used to select the correct KES controller (think: ingress.ingressClassName)
-                  The KES controller is instantiated with a specific controller name
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
                   and filters ES based on this property'
                 type: string
               provider:
@@ -1645,8 +1645,8 @@ spec:
                   type: object
                 type: array
               controller:
-                description: 'Used to select the correct KES controller (think: ingress.ingressClassName)
-                  The KES controller is instantiated with a specific controller name
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
                   and filters ES based on this property'
                 type: string
               provider:

diff --git a/config/crds/bases/external-secrets.io_secretstores.yaml b/config/crds/bases/external-secrets.io_secretstores.yaml
@@ -47,8 +47,8 @@ spec:
             description: SecretStoreSpec defines the desired state of SecretStore.
             properties:
               controller:
-                description: 'Used to select the correct KES controller (think: ingress.ingressClassName)
-                  The KES controller is instantiated with a specific controller name
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
                   and filters ES based on this property'
                 type: string
               provider:
@@ -1645,8 +1645,8 @@ spec:
                   type: object
                 type: array
               controller:
-                description: 'Used to select the correct KES controller (think: ingress.ingressClassName)
-                  The KES controller is instantiated with a specific controller name
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
                   and filters ES based on this property'
                 type: string
               provider:

diff --git a/config/crds/bases/generators.external-secrets.io_fakes.yaml b/config/crds/bases/generators.external-secrets.io_fakes.yaml
@@ -38,6 +38,11 @@ spec:
           spec:
             description: FakeSpec contains the static data.
             properties:
+              controller:
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
+                  and filters VDS based on this property'
+                type: string
               data:
                 additionalProperties:
                   type: string

diff --git a/config/crds/bases/generators.external-secrets.io_vaultdynamicsecrets.yaml b/config/crds/bases/generators.external-secrets.io_vaultdynamicsecrets.yaml
@@ -35,6 +35,11 @@ spec:
             type: object
           spec:
             properties:
+              controller:
+                description: 'Used to select the correct ESO controller (think: ingress.ingressClassName)
+                  The ESO controller is instantiated with a specific controller name
+                  and filters VDS based on this property'
+                type: string
               method:
                 description: Vault API method to use (GET/POST/other)
                 type: string

diff --git a/deploy/charts/external-secrets/tests/__snapshot__/crds_test.yaml.snap b/deploy/charts/external-secrets/tests/__snapshot__/crds_test.yaml.snap
@@ -54,7 +54,7 @@ should match snapshot of default values:
                   description: SecretStoreSpec defines the desired state of SecretStore.
                   properties:
                     controller:
-                      description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                      description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                       type: string
                     provider:
                       description: Used to configure the provider. Only one provider may be set
@@ -1216,7 +1216,7 @@ should match snapshot of default values:
                         type: object
                       type: array
                     controller:
-                      description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                      description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                       type: string
                     provider:
                       description: Used to configure the provider. Only one provider may be set

diff --git a/deploy/crds/bundle.yaml b/deploy/crds/bundle.yaml
@@ -498,7 +498,7 @@ spec:
               description: SecretStoreSpec defines the desired state of SecretStore.
               properties:
                 controller:
-                  description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                   type: string
                 provider:
                   description: Used to configure the provider. Only one provider may be set
@@ -1660,7 +1660,7 @@ spec:
                     type: object
                   type: array
                 controller:
-                  description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                   type: string
                 provider:
                   description: Used to configure the provider. Only one provider may be set
@@ -4052,7 +4052,7 @@ spec:
               description: SecretStoreSpec defines the desired state of SecretStore.
               properties:
                 controller:
-                  description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                   type: string
                 provider:
                   description: Used to configure the provider. Only one provider may be set
@@ -5214,7 +5214,7 @@ spec:
                     type: object
                   type: array
                 controller:
-                  description: 'Used to select the correct KES controller (think: ingress.ingressClassName) The KES controller is instantiated with a specific controller name and filters ES based on this property'
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters ES based on this property'
                   type: string
                 provider:
                   description: Used to configure the provider. Only one provider may be set
@@ -7031,6 +7031,9 @@ spec:
             spec:
               description: FakeSpec contains the static data.
               properties:
+                controller:
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters VDS based on this property'
+                  type: string
                 data:
                   additionalProperties:
                     type: string
@@ -7270,6 +7273,9 @@ spec:
               type: object
             spec:
               properties:
+                controller:
+                  description: 'Used to select the correct ESO controller (think: ingress.ingressClassName) The ESO controller is instantiated with a specific controller name and filters VDS based on this property'
+                  type: string
                 method:
                   description: Vault API method to use (GET/POST/other)
                   type: string

diff --git a/docs/api/spec.md b/docs/api/spec.md
@@ -1470,8 +1470,8 @@ string
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to select the correct KES controller (think: ingress.ingressClassName)
-The KES controller is instantiated with a specific controller name and filters ES based on this property</p>
+<p>Used to select the correct ESO controller (think: ingress.ingressClassName)
+The ESO controller is instantiated with a specific controller name and filters ES based on this property</p>
 </td>
 </tr>
 <tr>
@@ -4211,8 +4211,8 @@ string
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to select the correct KES controller (think: ingress.ingressClassName)
-The KES controller is instantiated with a specific controller name and filters ES based on this property</p>
+<p>Used to select the correct ESO controller (think: ingress.ingressClassName)
+The ESO controller is instantiated with a specific controller name and filters ES based on this property</p>
 </td>
 </tr>
 <tr>
@@ -4722,8 +4722,8 @@ string
 </td>
 <td>
 <em>(Optional)</em>
-<p>Used to select the correct KES controller (think: ingress.ingressClassName)
-The KES controller is instantiated with a specific controller name and filters ES based on this property</p>
+<p>Used to select the correct ESO controller (think: ingress.ingressClassName)
+The ESO controller is instantiated with a specific controller name and filters ES based on this property</p>
 </td>
 </tr>
 <tr>

diff --git a/pkg/controllers/externalsecret/externalsecret_controller.go b/pkg/controllers/externalsecret/externalsecret_controller.go
@@ -458,6 +458,21 @@ func shouldSkipUnmanagedStore(ctx context.Context, namespace string, r *Reconcil
 		if ref.SourceRef != nil && ref.SourceRef.SecretStoreRef != nil {
 			storeList = append(storeList, *ref.SourceRef.SecretStoreRef)
 		}
+
+		// verify that generator's controllerClass matches
+		if ref.SourceRef != nil && ref.SourceRef.GeneratorRef != nil {
+			genDef, err := r.getGeneratorDefinition(ctx, namespace, ref.SourceRef)
+			if err != nil {
+				return false, err
+			}
+			skipGenerator, err := shouldSkipGenerator(r, genDef)
+			if err != nil {
+				return false, err
+			}
+			if skipGenerator {
+				return true, nil
+			}
+		}
 	}
 
 	for _, ref := range storeList {

diff --git a/pkg/controllers/externalsecret/externalsecret_controller_secret.go b/pkg/controllers/externalsecret/externalsecret_controller_secret.go
@@ -16,6 +16,7 @@ package externalsecret
 
 import (
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 
@@ -229,3 +230,13 @@ func (r *Reconciler) handleFindAllSecrets(ctx context.Context, externalSecret *e
 	}
 	return secretMap, err
 }
+
+func shouldSkipGenerator(r *Reconciler, generatorDef *apiextensions.JSON) (bool, error) {
+	var genControllerClass genv1alpha1.ControllerClassResource
+	err := json.Unmarshal(generatorDef.Raw, &genControllerClass)
+	if err != nil {
+		return false, err
+	}
+	var controllerClass = genControllerClass.Spec.ControllerClass
+	return controllerClass != "" && controllerClass != r.ControllerClass, nil
+}
diff --git a/pkg/controllers/externalsecret/externalsecret_controller_test.go b/pkg/controllers/externalsecret/externalsecret_controller_test.go
@@ -16,6 +16,7 @@ package externalsecret
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"fmt"
 	"os"
 	"strconv"
@@ -26,6 +27,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	dto "github.com/prometheus/client_model/go"
 	v1 "k8s.io/api/core/v1"
+	apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -487,6 +489,33 @@ var _ = Describe("ExternalSecret controller", func() {
 		}
 	}
 
+	ignoreMismatchControllerForGeneratorRef := func(tc *testCase) {
+		const secretKey = "somekey"
+		const secretVal = "someValue"
+
+		fakeGenerator := &genv1alpha1.Fake{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "mytestfake2",
+				Namespace: ExternalSecretNamespace,
+			},
+			Spec: genv1alpha1.FakeSpec{
+				Data: map[string]string{
+					secretKey: secretVal,
+				},
+				Controller: "fakeControllerClass",
+			},
+		}
+
+		fakeGeneratorJSON, _ := json.Marshal(fakeGenerator)
+
+		Expect(shouldSkipGenerator(
+			&Reconciler{
+				ControllerClass: "default",
+			},
+			&apiextensions.JSON{Raw: fakeGeneratorJSON},
+		)).To(BeTrue())
+	}
+
 	syncWithMultipleSecretStores := func(tc *testCase) {
 		Expect(k8sClient.Create(context.Background(), &esv1beta1.SecretStore{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1970,6 +1999,7 @@ var _ = Describe("ExternalSecret controller", func() {
 		Entry("should not update unchanged secret using creationPolicy=Merge", mergeWithSecretNoChange),
 		Entry("should not delete pre-existing secret with creationPolicy=Orphan", createSecretPolicyOrphan),
 		Entry("should sync with generatorRef", syncWithGeneratorRef),
+		Entry("should not process generatorRef with mismatching controller field", ignoreMismatchControllerForGeneratorRef),
 		Entry("should sync with multiple secret stores via sourceRef", syncWithMultipleSecretStores),
 		Entry("should sync with template", syncWithTemplate),
 		Entry("should sync with template engine v2", syncWithTemplateV2),