From 731188e45b26eacccbd090021e86d042c6279b3a Mon Sep 17 00:00:00 2001 From: Stefan Prodan Date: Fri, 7 Oct 2022 12:45:44 +0300 Subject: [PATCH] Refactor: Extract generator to internal package Signed-off-by: Stefan Prodan --- controllers/kustomization_controller.go | 7 +- internal/generator/build.go | 75 +++++++++++++++++++ internal/generator/build_test.go | 59 +++++++++++++++ .../generator/generator.go | 59 +-------------- .../generator/generator_test.go | 40 +--------- .../Kustomization/Kustomization | 0 .../Kustomization/deployment.yaml | 0 .../different-filenames/yaml/deployment.yaml | 0 .../yaml/kustomization.yaml | 0 .../different-filenames/yml/deployment.yaml | 0 .../different-filenames/yml/kustomization.yml | 0 .../testdata/panic/kustomization.yaml | 0 .../generator}/testdata/panic/secret.age.yaml | 0 .../relbase/clusters/base/configmap.yaml | 0 .../relbase/clusters/base/kustomization.yaml | 0 .../testdata/relbase/clusters/patches.yaml | 0 .../staging/flux-system/kustomization.yaml | 0 .../clusters/staging/flux-system/secret.yaml | 0 .../testdata/remote/kustomization.yaml | 0 .../generator/varsub.go | 22 +++++- 20 files changed, 163 insertions(+), 99 deletions(-) create mode 100644 internal/generator/build.go create mode 100644 internal/generator/build_test.go rename controllers/kustomization_generator.go => internal/generator/generator.go (78%) rename controllers/kustomization_generator_test.go => internal/generator/generator_test.go (68%) rename {controllers => internal/generator}/testdata/different-filenames/Kustomization/Kustomization (100%) rename {controllers => internal/generator}/testdata/different-filenames/Kustomization/deployment.yaml (100%) rename {controllers => internal/generator}/testdata/different-filenames/yaml/deployment.yaml (100%) rename {controllers => internal/generator}/testdata/different-filenames/yaml/kustomization.yaml (100%) rename {controllers => internal/generator}/testdata/different-filenames/yml/deployment.yaml (100%) rename {controllers => internal/generator}/testdata/different-filenames/yml/kustomization.yml (100%) rename {controllers => internal/generator}/testdata/panic/kustomization.yaml (100%) rename {controllers => internal/generator}/testdata/panic/secret.age.yaml (100%) rename {controllers => internal/generator}/testdata/relbase/clusters/base/configmap.yaml (100%) rename {controllers => internal/generator}/testdata/relbase/clusters/base/kustomization.yaml (100%) rename {controllers => internal/generator}/testdata/relbase/clusters/patches.yaml (100%) rename {controllers => internal/generator}/testdata/relbase/clusters/staging/flux-system/kustomization.yaml (100%) rename {controllers => internal/generator}/testdata/relbase/clusters/staging/flux-system/secret.yaml (100%) rename {controllers => internal/generator}/testdata/remote/kustomization.yaml (100%) rename controllers/kustomization_varsub.go => internal/generator/varsub.go (82%) diff --git a/controllers/kustomization_controller.go b/controllers/kustomization_controller.go index efd18a39..1a9675be 100644 --- a/controllers/kustomization_controller.go +++ b/controllers/kustomization_controller.go @@ -60,6 +60,7 @@ import ( kustomizev1 "github.com/fluxcd/kustomize-controller/api/v1beta2" "github.com/fluxcd/kustomize-controller/internal/decryptor" + "github.com/fluxcd/kustomize-controller/internal/generator" ) // +kubebuilder:rbac:groups=kustomize.toolkit.fluxcd.io,resources=kustomizations,verbs=get;list;watch;create;update;patch;delete @@ -593,7 +594,7 @@ func (r *KustomizationReconciler) getSource(ctx context.Context, kustomization k } func (r *KustomizationReconciler) generate(kustomization kustomizev1.Kustomization, workDir string, dirPath string) error { - _, err := NewGenerator(workDir, kustomization).WriteFile(dirPath) + _, err := generator.NewGenerator(workDir, kustomization).WriteFile(dirPath) return err } @@ -614,7 +615,7 @@ func (r *KustomizationReconciler) build(ctx context.Context, workDir string, kus return nil, fmt.Errorf("error decrypting env sources: %w", err) } - m, err := secureBuildKustomization(workDir, dirPath, !r.NoRemoteBases) + m, err := generator.Build(workDir, dirPath, !r.NoRemoteBases) if err != nil { return nil, fmt.Errorf("kustomize build failed: %w", err) } @@ -642,7 +643,7 @@ func (r *KustomizationReconciler) build(ctx context.Context, workDir string, kus // run variable substitutions if kustomization.Spec.PostBuild != nil { - outRes, err := substituteVariables(ctx, r.Client, kustomization, res) + outRes, err := generator.SubstituteVariables(ctx, r.Client, kustomization, res) if err != nil { return nil, fmt.Errorf("var substitution failed for '%s': %w", res.GetName(), err) } diff --git a/internal/generator/build.go b/internal/generator/build.go new file mode 100644 index 00000000..83ae4d61 --- /dev/null +++ b/internal/generator/build.go @@ -0,0 +1,75 @@ +/* +Copyright 2020 The Flux authors + +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 generator + +import ( + "fmt" + "sync" + + securefs "github.com/fluxcd/pkg/kustomize/filesys" + "sigs.k8s.io/kustomize/api/krusty" + "sigs.k8s.io/kustomize/api/resmap" + kustypes "sigs.k8s.io/kustomize/api/types" + "sigs.k8s.io/kustomize/kyaml/filesys" +) + +// buildMutex protects against kustomize concurrent map read/write panic +var buildMutex sync.Mutex + +// Build wraps krusty.MakeKustomizer with the following settings: +// - secure on-disk FS denying operations outside root +// - load files from outside the kustomization dir path +// (but not outside root) +// - disable plugins except for the builtin ones +func Build(root, dirPath string, allowRemoteBases bool) (_ resmap.ResMap, err error) { + var fs filesys.FileSystem + + // Create secure FS for root with or without remote base support + if allowRemoteBases { + fs, err = securefs.MakeFsOnDiskSecureBuild(root) + if err != nil { + return nil, err + } + } else { + fs, err = securefs.MakeFsOnDiskSecure(root) + if err != nil { + return nil, err + } + } + + // Temporary workaround for concurrent map read and map write bug + // https://github.com/kubernetes-sigs/kustomize/issues/3659 + buildMutex.Lock() + defer buildMutex.Unlock() + + // Kustomize tends to panic in unpredicted ways due to (accidental) + // invalid object data; recover when this happens to ensure continuity of + // operations + defer func() { + if r := recover(); r != nil { + err = fmt.Errorf("recovered from kustomize build panic: %v", r) + } + }() + + buildOptions := &krusty.Options{ + LoadRestrictions: kustypes.LoadRestrictionsNone, + PluginConfig: kustypes.DisabledPluginConfig(), + } + + k := krusty.MakeKustomizer(buildOptions) + return k.Run(fs, dirPath) +} diff --git a/internal/generator/build_test.go b/internal/generator/build_test.go new file mode 100644 index 00000000..d40af7c6 --- /dev/null +++ b/internal/generator/build_test.go @@ -0,0 +1,59 @@ +/* +Copyright 2022 The Flux authors + +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 generator + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func Test_Buid(t *testing.T) { + t.Run("remote build", func(t *testing.T) { + g := NewWithT(t) + + _, err := Build("testdata/remote", "testdata/remote", true) + g.Expect(err).ToNot(HaveOccurred()) + }) + + t.Run("no remote build", func(t *testing.T) { + g := NewWithT(t) + + _, err := Build("testdata/remote", "testdata/remote", false) + g.Expect(err).To(HaveOccurred()) + }) +} + +func Test_Buid_panic(t *testing.T) { + t.Run("build panic", func(t *testing.T) { + g := NewWithT(t) + + _, err := Build("testdata/panic", "testdata/panic", false) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("recovered from kustomize build panic")) + // Run again to ensure the lock is released + _, err = Build("testdata/panic", "testdata/panic", false) + g.Expect(err).To(HaveOccurred()) + }) +} + +func Test_Buid_rel_basedir(t *testing.T) { + g := NewWithT(t) + + _, err := Build("testdata/relbase", "testdata/relbase/clusters/staging/flux-system", false) + g.Expect(err).ToNot(HaveOccurred()) +} diff --git a/controllers/kustomization_generator.go b/internal/generator/generator.go similarity index 78% rename from controllers/kustomization_generator.go rename to internal/generator/generator.go index bae39885..9404c375 100644 --- a/controllers/kustomization_generator.go +++ b/internal/generator/generator.go @@ -14,28 +14,24 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controllers +package generator import ( "encoding/json" "fmt" "os" "path/filepath" + "strings" - "sync" + securefs "github.com/fluxcd/pkg/kustomize/filesys" "sigs.k8s.io/kustomize/api/konfig" - "sigs.k8s.io/kustomize/api/krusty" "sigs.k8s.io/kustomize/api/provider" - "sigs.k8s.io/kustomize/api/resmap" kustypes "sigs.k8s.io/kustomize/api/types" - "sigs.k8s.io/kustomize/kyaml/filesys" "sigs.k8s.io/yaml" - "github.com/fluxcd/pkg/apis/kustomize" - securefs "github.com/fluxcd/pkg/kustomize/filesys" - kustomizev1 "github.com/fluxcd/kustomize-controller/api/v1beta2" + "github.com/fluxcd/pkg/apis/kustomize" ) type KustomizeGenerator struct { @@ -238,50 +234,3 @@ func adaptSelector(selector *kustomize.Selector) (output *kustypes.Selector) { } return } - -// TODO: remove mutex when kustomize fixes the concurrent map read/write panic -var kustomizeBuildMutex sync.Mutex - -// secureBuildKustomization wraps krusty.MakeKustomizer with the following settings: -// - secure on-disk FS denying operations outside root -// - load files from outside the kustomization dir path -// (but not outside root) -// - disable plugins except for the builtin ones -func secureBuildKustomization(root, dirPath string, allowRemoteBases bool) (_ resmap.ResMap, err error) { - var fs filesys.FileSystem - - // Create secure FS for root with or without remote base support - if allowRemoteBases { - fs, err = securefs.MakeFsOnDiskSecureBuild(root) - if err != nil { - return nil, err - } - } else { - fs, err = securefs.MakeFsOnDiskSecure(root) - if err != nil { - return nil, err - } - } - - // Temporary workaround for concurrent map read and map write bug - // https://github.com/kubernetes-sigs/kustomize/issues/3659 - kustomizeBuildMutex.Lock() - defer kustomizeBuildMutex.Unlock() - - // Kustomize tends to panic in unpredicted ways due to (accidental) - // invalid object data; recover when this happens to ensure continuity of - // operations - defer func() { - if r := recover(); r != nil { - err = fmt.Errorf("recovered from kustomize build panic: %v", r) - } - }() - - buildOptions := &krusty.Options{ - LoadRestrictions: kustypes.LoadRestrictionsNone, - PluginConfig: kustypes.DisabledPluginConfig(), - } - - k := krusty.MakeKustomizer(buildOptions) - return k.Run(fs, dirPath) -} diff --git a/controllers/kustomization_generator_test.go b/internal/generator/generator_test.go similarity index 68% rename from controllers/kustomization_generator_test.go rename to internal/generator/generator_test.go index a251e4cd..e3a605b0 100644 --- a/controllers/kustomization_generator_test.go +++ b/internal/generator/generator_test.go @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package controllers +package generator import ( "os" @@ -30,43 +30,7 @@ import ( . "github.com/onsi/gomega" ) -func Test_secureBuildKustomization(t *testing.T) { - t.Run("remote build", func(t *testing.T) { - g := NewWithT(t) - - _, err := secureBuildKustomization("testdata/remote", "testdata/remote", true) - g.Expect(err).ToNot(HaveOccurred()) - }) - - t.Run("no remote build", func(t *testing.T) { - g := NewWithT(t) - - _, err := secureBuildKustomization("testdata/remote", "testdata/remote", false) - g.Expect(err).To(HaveOccurred()) - }) -} - -func Test_secureBuildKustomization_panic(t *testing.T) { - t.Run("build panic", func(t *testing.T) { - g := NewWithT(t) - - _, err := secureBuildKustomization("testdata/panic", "testdata/panic", false) - g.Expect(err).To(HaveOccurred()) - g.Expect(err.Error()).To(ContainSubstring("recovered from kustomize build panic")) - // Run again to ensure the lock is released - _, err = secureBuildKustomization("testdata/panic", "testdata/panic", false) - g.Expect(err).To(HaveOccurred()) - }) -} - -func Test_secureBuildKustomization_rel_basedir(t *testing.T) { - g := NewWithT(t) - - _, err := secureBuildKustomization("testdata/relbase", "testdata/relbase/clusters/staging/flux-system", false) - g.Expect(err).ToNot(HaveOccurred()) -} - -func TestGeneratorWriteFile(t *testing.T) { +func TestGenerator_WriteFile(t *testing.T) { tests := []struct { name string dir string diff --git a/controllers/testdata/different-filenames/Kustomization/Kustomization b/internal/generator/testdata/different-filenames/Kustomization/Kustomization similarity index 100% rename from controllers/testdata/different-filenames/Kustomization/Kustomization rename to internal/generator/testdata/different-filenames/Kustomization/Kustomization diff --git a/controllers/testdata/different-filenames/Kustomization/deployment.yaml b/internal/generator/testdata/different-filenames/Kustomization/deployment.yaml similarity index 100% rename from controllers/testdata/different-filenames/Kustomization/deployment.yaml rename to internal/generator/testdata/different-filenames/Kustomization/deployment.yaml diff --git a/controllers/testdata/different-filenames/yaml/deployment.yaml b/internal/generator/testdata/different-filenames/yaml/deployment.yaml similarity index 100% rename from controllers/testdata/different-filenames/yaml/deployment.yaml rename to internal/generator/testdata/different-filenames/yaml/deployment.yaml diff --git a/controllers/testdata/different-filenames/yaml/kustomization.yaml b/internal/generator/testdata/different-filenames/yaml/kustomization.yaml similarity index 100% rename from controllers/testdata/different-filenames/yaml/kustomization.yaml rename to internal/generator/testdata/different-filenames/yaml/kustomization.yaml diff --git a/controllers/testdata/different-filenames/yml/deployment.yaml b/internal/generator/testdata/different-filenames/yml/deployment.yaml similarity index 100% rename from controllers/testdata/different-filenames/yml/deployment.yaml rename to internal/generator/testdata/different-filenames/yml/deployment.yaml diff --git a/controllers/testdata/different-filenames/yml/kustomization.yml b/internal/generator/testdata/different-filenames/yml/kustomization.yml similarity index 100% rename from controllers/testdata/different-filenames/yml/kustomization.yml rename to internal/generator/testdata/different-filenames/yml/kustomization.yml diff --git a/controllers/testdata/panic/kustomization.yaml b/internal/generator/testdata/panic/kustomization.yaml similarity index 100% rename from controllers/testdata/panic/kustomization.yaml rename to internal/generator/testdata/panic/kustomization.yaml diff --git a/controllers/testdata/panic/secret.age.yaml b/internal/generator/testdata/panic/secret.age.yaml similarity index 100% rename from controllers/testdata/panic/secret.age.yaml rename to internal/generator/testdata/panic/secret.age.yaml diff --git a/controllers/testdata/relbase/clusters/base/configmap.yaml b/internal/generator/testdata/relbase/clusters/base/configmap.yaml similarity index 100% rename from controllers/testdata/relbase/clusters/base/configmap.yaml rename to internal/generator/testdata/relbase/clusters/base/configmap.yaml diff --git a/controllers/testdata/relbase/clusters/base/kustomization.yaml b/internal/generator/testdata/relbase/clusters/base/kustomization.yaml similarity index 100% rename from controllers/testdata/relbase/clusters/base/kustomization.yaml rename to internal/generator/testdata/relbase/clusters/base/kustomization.yaml diff --git a/controllers/testdata/relbase/clusters/patches.yaml b/internal/generator/testdata/relbase/clusters/patches.yaml similarity index 100% rename from controllers/testdata/relbase/clusters/patches.yaml rename to internal/generator/testdata/relbase/clusters/patches.yaml diff --git a/controllers/testdata/relbase/clusters/staging/flux-system/kustomization.yaml b/internal/generator/testdata/relbase/clusters/staging/flux-system/kustomization.yaml similarity index 100% rename from controllers/testdata/relbase/clusters/staging/flux-system/kustomization.yaml rename to internal/generator/testdata/relbase/clusters/staging/flux-system/kustomization.yaml diff --git a/controllers/testdata/relbase/clusters/staging/flux-system/secret.yaml b/internal/generator/testdata/relbase/clusters/staging/flux-system/secret.yaml similarity index 100% rename from controllers/testdata/relbase/clusters/staging/flux-system/secret.yaml rename to internal/generator/testdata/relbase/clusters/staging/flux-system/secret.yaml diff --git a/controllers/testdata/remote/kustomization.yaml b/internal/generator/testdata/remote/kustomization.yaml similarity index 100% rename from controllers/testdata/remote/kustomization.yaml rename to internal/generator/testdata/remote/kustomization.yaml diff --git a/controllers/kustomization_varsub.go b/internal/generator/varsub.go similarity index 82% rename from controllers/kustomization_varsub.go rename to internal/generator/varsub.go index 484eb24d..3168e397 100644 --- a/controllers/kustomization_varsub.go +++ b/internal/generator/varsub.go @@ -1,4 +1,20 @@ -package controllers +/* +Copyright 2021 The Flux authors + +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 generator import ( "context" @@ -21,10 +37,10 @@ import ( // the var names before substitution const varsubRegex = "^[_[:alpha:]][_[:alpha:][:digit:]]*$" -// substituteVariables replaces the vars with their values in the specified resource. +// SubstituteVariables replaces the vars with their values in the specified resource. // If a resource is labeled or annotated with // 'kustomize.toolkit.fluxcd.io/substitute: disabled' the substitution is skipped. -func substituteVariables( +func SubstituteVariables( ctx context.Context, kubeClient client.Client, kustomization kustomizev1.Kustomization,