From 08bb2291feb575aeb327cb53922550dc3580f5c4 Mon Sep 17 00:00:00 2001 From: "Saumya Shovan Roy (Deep)" <12953177+rdeepc@users.noreply.github.com> Date: Tue, 16 May 2023 02:59:26 -0400 Subject: [PATCH] feat: add controller class on VaultDynamicSecret resources (#2287) * feat: add generator for vaultdynamicsecret * Added controllerClass on VaultDynamicSecret * Added controllerClass on VaultDynamicSecret Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> * Fixed lint Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> * Fixed hack bash Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> * feat: Implemented generator controller class support - Controller class support in VaultDynamicSecret - Controller class support in Fake Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> * feat: Implemented Generator controller class check Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> * feat: Implemented Generator controller class check Signed-off-by: rdeepc * feat: Implemented Generator controller class check Signed-off-by: rdeepc * 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 --------- Signed-off-by: rdeepc <12953177+rdeepc@users.noreply.github.com> Signed-off-by: rdeepc Signed-off-by: Moritz Johner Co-authored-by: Frederic Mereu Co-authored-by: Moritz Johner --- .../v1alpha1/secretstore_types.go | 4 +-- .../v1beta1/secretstore_types.go | 4 +-- apis/generators/v1alpha1/generator_fake.go | 5 ++++ apis/generators/v1alpha1/generator_types.go | 21 +++++++++++++ apis/generators/v1alpha1/generator_vault.go | 5 ++++ .../v1alpha1/zz_generated.deepcopy.go | 16 ++++++++++ ...ternal-secrets.io_clustersecretstores.yaml | 8 ++--- .../external-secrets.io_secretstores.yaml | 8 ++--- .../generators.external-secrets.io_fakes.yaml | 5 ++++ ...ternal-secrets.io_vaultdynamicsecrets.yaml | 5 ++++ .../tests/__snapshot__/crds_test.yaml.snap | 4 +-- deploy/crds/bundle.yaml | 14 ++++++--- docs/api/spec.md | 12 ++++---- .../externalsecret_controller.go | 15 ++++++++++ .../externalsecret_controller_secret.go | 11 +++++++ .../externalsecret_controller_test.go | 30 +++++++++++++++++++ 16 files changed, 143 insertions(+), 24 deletions(-) create mode 100644 apis/generators/v1alpha1/generator_types.go diff --git a/apis/externalsecrets/v1alpha1/secretstore_types.go b/apis/externalsecrets/v1alpha1/secretstore_types.go index b4b66ef5c4d..ad47ce489b9 100644 --- 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 index e924f48edf2..8a2f835cafa 100644 --- 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 index 9a0ab9d7e74..9b93214195a 100644 --- 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 new file mode 100644 index 00000000000..6d7ac9d4242 --- /dev/null +++ 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 index 5005b30db5b..4e976f8c3d2 100644 --- 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 index 03d621373db..62d9d463771 100644 --- a/apis/generators/v1alpha1/zz_generated.deepcopy.go +++ b/apis/generators/v1alpha1/zz_generated.deepcopy.go @@ -265,6 +265,22 @@ func (in *AzureACRWorkloadIdentityAuth) DeepCopy() *AzureACRWorkloadIdentityAuth return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ControllerClassResource) DeepCopyInto(out *ControllerClassResource) { + *out = *in + out.Spec = in.Spec +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ControllerClassResource. +func (in *ControllerClassResource) DeepCopy() *ControllerClassResource { + if in == nil { + return nil + } + out := new(ControllerClassResource) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ECRAuthorizationToken) DeepCopyInto(out *ECRAuthorizationToken) { *out = *in diff --git a/config/crds/bases/external-secrets.io_clustersecretstores.yaml b/config/crds/bases/external-secrets.io_clustersecretstores.yaml index 80e612173aa..58e4e87c38a 100644 --- 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 index 0813f0ccef7..a096a4e31bf 100644 --- 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 index 09136b0a0e6..920dfdcddc7 100644 --- 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 index a58b89292e5..d99a819c0d7 100644 --- 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 index ad6e6291e50..53ca18a0de0 100644 --- 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 index 0760b426695..adaa5e611f6 100644 --- 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 index 05418152e06..afb5d2578ed 100644 --- a/docs/api/spec.md +++ b/docs/api/spec.md @@ -1470,8 +1470,8 @@ string (Optional) -

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

@@ -4211,8 +4211,8 @@ string (Optional) -

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

@@ -4722,8 +4722,8 @@ string (Optional) -

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

diff --git a/pkg/controllers/externalsecret/externalsecret_controller.go b/pkg/controllers/externalsecret/externalsecret_controller.go index 8d6535196bf..76c704df1a0 100644 --- 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 index 9a36a35a2a4..a7ff7313ab3 100644 --- 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 index e614ede9607..d97ea0ecd53 100644 --- 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),