Merge pull request #936 from kinvolk/invidian/simplify-assets-handling

pkg/assets: cleanup exported API

Author: invidian

pkg/assets/assets.go

@@ -19,13 +19,27 @@ package assets
 import (
 	"fmt"
 	"io"
-	"net/http"
 	"os"
-	"time"
 
-	"github.com/prometheus/alertmanager/pkg/modtimevfs"
-	"github.com/shurcooL/httpfs/union"
-	"github.com/shurcooL/vfsgen"
+	// Currently pkg/assets/assets_generate.go file is ignored from regular
+	// builds and it's only used when running 'go generate'.
+	//
+	// Because of that, external dependencies of assets_generate.go are not
+	// normally added to go.mod and go.sum files, unless you run 'go generate'.
+	// But then 'go mod tidy' removes them again.
+	//
+	// Those anonymous imports ensure, that those external dependencies
+	// will always be pulled into go.mod file.
+	//
+	// The alternative would be to refactor the function to actually use the
+	// dependencies here, but then we have an exported function, which
+	// shouldn't really be exported.
+	//
+	// Also see https://github.com/golang/go/issues/25922 and https://github.com/golang/go/issues/29516
+	// for more details about this issue.
+	_ "github.com/prometheus/alertmanager/pkg/modtimevfs"
+	_ "github.com/shurcooL/httpfs/union"
+	_ "github.com/shurcooL/vfsgen"
 )
 
 const (
@@ -37,46 +51,24 @@ const (
 	TerraformModulesSource = "/terraform-modules"
 )
 
-type WalkFunc func(fileName string, fileInfo os.FileInfo, r io.ReadSeeker, err error) error
+type walkFunc func(fileName string, fileInfo os.FileInfo, r io.ReadSeeker, err error) error
 
-type AssetsIface interface {
+type assetsIface interface {
 	// WalkFiles calls cb for every regular file within path.
 	//
 	// Usually, fileName passed to the cb will be relative to
 	// path. But in case of error, it is possible that it will
 	// not.be relative. Also, in case of error, fileInfo or r may
 	// be nil.
-	WalkFiles(path string, cb WalkFunc) error
+	WalkFiles(path string, cb walkFunc) error
 }
 
-var Assets AssetsIface
-
-func init() {
-	Assets = newEmbeddedAssets()
+func get() assetsIface {
 	if value, found := os.LookupEnv("LOKOCTL_USE_FS_ASSETS"); found {
-		Assets = newFsAssets(value)
+		return newFsAssets(value)
 	}
-}
 
-// Generate function wraps vfsgen.Generate function.
-// Additionally to vfsgen.Generate, it also takes map of directories,
-// where key represents path in the assets and a value represents path
-// to the assets directory in local filesystem (which should be relative).
-//
-// This function also resets modification time for every file, so creating a new copy
-// of code does not trigger changes in all asset files.
-func Generate(fileName string, packageName string, variableName string, dirs map[string]string) error {
-	ufs := make(map[string]http.FileSystem)
-	for assetsPath, fsPath := range dirs {
-		ufs[assetsPath] = http.Dir(fsPath)
-	}
-	u := union.New(ufs)
-	fs := modtimevfs.New(u, time.Unix(1, 0))
-	return vfsgen.Generate(fs, vfsgen.Options{
-		Filename:     fileName,
-		PackageName:  packageName,
-		VariableName: variableName,
-	})
+	return newEmbeddedAssets()
 }
 
 // Extract recursively extracts the assets at src into the directory dst. If dst doesn't exist, the
@@ -85,8 +77,8 @@ func Generate(fileName string, packageName string, variableName string, dirs map
 // The assets are read either from data embedded in the binary or from the filesystem, depending on
 // whether the LOKOCTL_USE_FS_ASSETS environment variable is set.
 func Extract(src, dst string) error {
-	walk := CopyingWalker(dst, 0755)
-	if err := Assets.WalkFiles(src, walk); err != nil {
+	walk := copyingWalker(dst, 0700)
+	if err := get().WalkFiles(src, walk); err != nil {
 		return fmt.Errorf("failed to walk assets: %v", err)
 	}
 

pkg/assets/assets_generate.go

@@ -18,19 +18,36 @@
 package main
 
 import (
-	"log"
+	"net/http"
+	"time"
+
+	// External dependencies should also be added to assets.go file.
+	"github.com/prometheus/alertmanager/pkg/modtimevfs"
+	"github.com/shurcooL/httpfs/union"
+	"github.com/shurcooL/vfsgen"
 
 	"github.com/kinvolk/lokomotive/pkg/assets"
 )
 
 func main() {
-	dirs := map[string]string{
-		assets.TerraformModulesSource: "../../assets/terraform-modules",
-		assets.ControlPlaneSource:     "../../assets/charts/control-plane",
-		assets.ComponentsSource:       "../../assets/charts/components",
+	directoriesToEmbed := map[string]http.FileSystem{
+		assets.TerraformModulesSource: http.Dir("../../assets/terraform-modules"),
+		assets.ControlPlaneSource:     http.Dir("../../assets/charts/control-plane"),
+		assets.ComponentsSource:       http.Dir("../../assets/charts/components"),
 	}
-	err := assets.Generate("generated_assets.go", "assets", "vfsgenAssets", dirs)
-	if err != nil {
-		log.Fatalln(err)
+
+	// Reset modification time on files, so after cloning the repository all
+	// assets don't get modified.
+	u := union.New(directoriesToEmbed)
+	fs := modtimevfs.New(u, time.Unix(1, 0))
+
+	options := vfsgen.Options{
+		Filename:     "generated_assets.go",
+		PackageName:  "assets",
+		VariableName: "vfsgenAssets",
+	}
+
+	if err := vfsgen.Generate(fs, options); err != nil {
+		panic(err)
 	}
 }

pkg/assets/embedded.go

@@ -29,15 +29,13 @@ type embeddedAssets struct {
 	fs http.FileSystem
 }
 
-var _ AssetsIface = &embeddedAssets{}
-
 func newEmbeddedAssets() *embeddedAssets {
 	return &embeddedAssets{
 		fs: vfsgenAssets,
 	}
 }
 
-func (a *embeddedAssets) WalkFiles(location string, cb WalkFunc) error {
+func (a *embeddedAssets) WalkFiles(location string, cb walkFunc) error {
 	return vfsutil.WalkFiles(a.fs, location, func(filePath string, fileInfo os.FileInfo, r io.ReadSeeker, err error) error {
 		if err != nil {
 			return cb(filePath, fileInfo, r, err)

pkg/assets/fs.go

@@ -26,8 +26,6 @@ type fsAssets struct {
 	assetsDir string
 }
 
-var _ AssetsIface = &fsAssets{}
-
 func newFsAssets(dir string) *fsAssets {
 	if dir == "" {
 		execDir, err := osext.ExecutableFolder()
@@ -41,7 +39,7 @@ func newFsAssets(dir string) *fsAssets {
 	}
 }
 
-func (a *fsAssets) WalkFiles(location string, cb WalkFunc) error {
+func (a *fsAssets) WalkFiles(location string, cb walkFunc) error {
 	relativeLocation := strings.TrimLeft(location, string(os.PathSeparator))
 	assetsLocation := filepath.Join(a.assetsDir, relativeLocation)
 	return filepath.Walk(assetsLocation, func(path string, info os.FileInfo, err error) error {

pkg/assets/walkers.go

@@ -17,13 +17,12 @@ package assets
 import (
 	"fmt"
 	"io"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 )
 
-// CopyingWalker returns a WalkFunc which writes the given file to disk.
-func CopyingWalker(path string, newDirPerms os.FileMode) WalkFunc {
+// copyingWalker returns a WalkFunc which writes the given file to disk.
+func copyingWalker(path string, newDirPerms os.FileMode) walkFunc {
 	return func(fileName string, fileInfo os.FileInfo, r io.ReadSeeker, err error) error {
 		if err != nil {
 			return fmt.Errorf("error while walking at %q: %w", fileName, err)
@@ -39,39 +38,6 @@ func CopyingWalker(path string, newDirPerms os.FileMode) WalkFunc {
 	}
 }
 
-// DumpingWalker returns a WalkFunc which sets the contents of the given file in a map.
-func DumpingWalker(contentsMap map[string]string, allowedExts ...string) WalkFunc {
-	var extsMap map[string]struct{}
-
-	if len(allowedExts) > 0 {
-		extsMap = make(map[string]struct{}, len(allowedExts))
-		for _, ext := range allowedExts {
-			extsMap[ext] = struct{}{}
-		}
-	}
-
-	return func(fileName string, fileInfo os.FileInfo, r io.ReadSeeker, err error) error {
-		if err != nil {
-			return fmt.Errorf("error while walking at %q: %w", fileName, err)
-		}
-
-		if extsMap != nil {
-			if _, ok := extsMap[filepath.Ext(fileName)]; !ok {
-				return nil
-			}
-		}
-
-		contents, err := ioutil.ReadAll(r)
-		if err != nil {
-			return fmt.Errorf("failed to read %q: %w", fileName, err)
-		}
-
-		contentsMap[fileName] = string(contents)
-
-		return nil
-	}
-}
-
 // writeFile writes data from given io.Reader to the file and makes sure, that
 // this is the only content stored in the file.
 func writeFile(p string, r io.Reader) error {

pkg/helm/charts.go

@@ -46,9 +46,7 @@ func ChartFromAssets(location string) (*chart.Chart, error) {
 	// isn't trivial since we call os.RemoveAll() in a defer statement.
 	defer os.RemoveAll(tmpDir) //nolint: errcheck
 
-	// Rendered files could contain secrets - allow r/w access to owner only.
-	walk := assets.CopyingWalker(tmpDir, 0700)
-	if err := assets.Assets.WalkFiles(location, walk); err != nil {
+	if err := assets.Extract(location, tmpDir); err != nil {
 		return nil, fmt.Errorf("traversing assets: %w", err)
 	}