Merge pull request #740 from fluxcd/refactor-generator

Refactor: Extract generator to internal package

Author: stefanprodan

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)
 			}

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)
+}

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())
+}

controllers/kustomization_generator.go renamed to 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)
-}

controllers/kustomization_generator_test.go renamed to 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

controllers/testdata/different-filenames/Kustomization/Kustomization renamed to internal/generator/testdata/different-filenames/Kustomization/Kustomization

@@ -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,