[TT-1802] Clean up Stow locations when deleting middleware (#22)

* refactor(http_funcs): carve up test files ahead of adding coverage
* feat(configuration): add capability to refresh and reload config from disk
Resetting the configuration for mserv to zero and (re)reading afresh aids testability.

* ci(golangci-lint): remove as per warning; maligned has since moved into 'go vet'
* test(http_funcs): add coverage for DeleteMW handler
Added a new test, along with some helpers and mock implementations, to cover the DeleteMW. Note that
the test is currently failing as of this commit, so let's see about fixing that!

* refactor(api): set up format string const for container name pattern
* refactor(api): use package consts instead of hard-coding Kind strings
* fix(api): also remove stow container when deleting a bundle
There was previously no call made from the bundle delete handler for stow to remove its local/S3
container, so local files and S3 buckets would be left lying around after their corresponding
middleware definitions within MServ wer long gone.

fix TT-1802

* fix(api): work around shortcoming with stow's "local" Kind
The RemoveContainer implementation for "local" Kind in stow does not use the full path when
deleting, only a relative one. If WasabiAiR/stow#239 were to be
implemented/resolved then this block of code could be deleted.

* fix(api): clean up lint issue by catching/logging potential error from Close call
* fix(api): walk container and remove each item before removing container, for S3's sake
Via Stow, S3 buckets can only be removed once they no longer contain any objects.

fix TT-1802

* refactor(api): fix lint issues; stop shadowing error declarations
* refactor(api): move store.DeleteMW call to end of handler rather than beginning
We still do a GetMWByID call on the middleware in the store at the start of the handler, which would
error and return immediately if middleware with that ID did not exist. Given that succeeds though,
we proceed to clean out the stow container and remove that, which is much more error-prone given the
new changes in this feature branch. Assuming all of that succeeds, then we delete the middleware
from the store.

* feat(api): export format string for container names

.gitignore

@@ -14,7 +14,7 @@ mservctl
 build/bundle.zip
 bundle.zip
 client/client
-files/mserv-plugin*
+files/mserv-plugin-*
 hack
 mserv.json
 plugins

.golangci.yaml

@@ -49,9 +49,6 @@ linters-settings:
     line-length: 120
     tab-width: 2
 
-  maligned:
-    suggest-new: true
-
   nakedret:
     max-func-lines: 25
 
@@ -107,7 +104,6 @@ linters:
     - govet
     - ineffassign
     - lll
-    - maligned
     - nakedret
     - nestif
     - nlreturn

api/api.go

@@ -24,11 +24,15 @@ import (
 	"github.com/TykTechnologies/mserv/util/storage/errors"
 )
 
-var (
+const (
+	// FmtPluginContainer is a format string for the layout of the container names.
+	FmtPluginContainer = "mserv-plugin-%s"
+
 	moduleName = "mserv.api"
-	log        = logger.GetLogger(moduleName)
 )
 
+var log = logger.GetLogger(moduleName)
+
 func NewAPI(store storage.MservStore) *API {
 	return &API{store: store}
 }
@@ -57,6 +61,69 @@ func (a *API) HandleDeleteBundle(bundleName string) error {
 		return err
 	}
 
+	fStore, err := GetFileStore()
+	if err != nil {
+		log.WithError(err).Error("failed to get file handle")
+
+		return err
+	}
+
+	defer func() {
+		if errFC := fStore.Close(); errFC != nil {
+			log.WithError(errFC).Error("error while closing file store")
+		}
+	}()
+
+	pluginContainerID := fmt.Sprintf(FmtPluginContainer, bundleName)
+
+	fCont, err := fStore.Container(pluginContainerID)
+	if err != nil {
+		return fmt.Errorf("could not get container: %w", err)
+	}
+
+	if errWalk := stow.Walk(fCont, "", 100, func(i stow.Item, e error) error {
+		if e != nil {
+			return fmt.Errorf("error getting item while walking container: %w", e)
+		}
+
+		return fCont.RemoveItem(i.ID())
+	}); errWalk != nil {
+		return fmt.Errorf("error while walking container to delete contents: %w", errWalk)
+	}
+
+	// HACK: workaround for https://github.com/graymeta/stow/issues/239 - vvv
+	//
+	// (stow.Location).RemoveContainer doesn't currently take the full path into account for Kind "local".
+	// It merely calls "os.RemoveAll" with the _relative_ path, so we need to change to the parent path, and then defer
+	// changing back until after the misbehaving RemoveContainer call.
+	//
+	// Maybe swap out Stow for the Go CDK one day? https://gocloud.dev/howto/blob/
+
+	fsCfg := config.GetConf().Mserv.FileStore
+
+	if fsCfg.Kind == local.Kind {
+		prevWD, errWD := os.Getwd()
+		if errWD != nil {
+			return fmt.Errorf("could not get current working directory: %w", errWD)
+		}
+
+		if errCD := os.Chdir(fsCfg.Local.ConfigKeyPath); errCD != nil {
+			return fmt.Errorf("could not change current working directory: %w", errCD)
+		}
+
+		defer func() {
+			if errPD := os.Chdir(prevWD); errPD != nil {
+				log.WithError(errPD).WithField("dir", prevWD).Error("could not revert to previous working directory")
+			}
+		}()
+	}
+
+	// HACK: workaround for https://github.com/graymeta/stow/issues/239 - ^^^
+
+	if errRC := fStore.RemoveContainer(pluginContainerID); errRC != nil {
+		return fmt.Errorf("could not remove container '%s': %w", pluginContainerID, errRC)
+	}
+
 	return a.store.DeleteMW(mw.UID)
 }
 
@@ -112,10 +179,11 @@ func (a *API) HandleNewBundle(filePath string, apiID, bundleName string) (*stora
 	pluginPath := path.Join(bdl.Path, fName)
 
 	log.Info("storing bundle in asset repo")
-	pluginContainerID := "mserv-plugin-" + bundleName
+
+	pluginContainerID := fmt.Sprintf(FmtPluginContainer, bundleName)
 	fCont, getErr := fStore.Container(pluginContainerID)
 	if getErr != nil {
-		log.Warning("container not found, creating")
+		log.WithField("container-id", pluginContainerID).Warning("container not found, creating")
 		fCont, err = fStore.CreateContainer(pluginContainerID)
 		if err != nil {
 			return nil, fmt.Errorf("couldn't fetch container: %s, couldn't create container: %s", getErr.Error(), err.Error())
@@ -257,7 +325,8 @@ func (a *API) StoreBundleOnly(filePath string, apiID, bundleName string) (*stora
 	defer fStore.Close()
 
 	log.Info("file store handle opened, storing bundle in asset repo")
-	pluginContainerID := "mserv-plugin-" + bundleName
+
+	pluginContainerID := fmt.Sprintf(FmtPluginContainer, bundleName)
 	fCont, getErr := fStore.Container(pluginContainerID)
 	if getErr != nil {
 		log.WithField("container-id", pluginContainerID).Warning("container not found, creating")
@@ -396,21 +465,21 @@ func GetFileStore() (stow.Location, error) {
 	}
 
 	switch fsCfg.Kind {
-	case "local":
+	case local.Kind:
 		log.WithField("path", fsCfg.Local.ConfigKeyPath).Info("detected local store")
 
 		// Dialling stow/local will fail if the base directory doesn't already exist
 		if err := os.MkdirAll(fsCfg.Local.ConfigKeyPath, 0o750); err != nil && !os.IsExist(err) {
 			return nil, fmt.Errorf("%w: %s", ErrCreateLocal, fsCfg.Local.ConfigKeyPath)
 		}
 
-		return stow.Dial("local", stow.ConfigMap{
+		return stow.Dial(local.Kind, stow.ConfigMap{
 			local.ConfigKeyPath: fsCfg.Local.ConfigKeyPath,
 		})
-	case "s3":
+	case s3.Kind:
 		log.Info("detected s3 store")
 
-		return stow.Dial("s3", stow.ConfigMap{
+		return stow.Dial(s3.Kind, stow.ConfigMap{
 			s3.ConfigAccessKeyID: fsCfg.S3.ConfigAccessKeyID,
 			s3.ConfigRegion:      fsCfg.S3.ConfigRegion,
 			s3.ConfigSecretKey:   fsCfg.S3.ConfigSecretKey,

conf/conf.go

@@ -65,18 +65,32 @@ var (
 	log   = logger.GetLogger(moduleName)
 )
 
-// GetConf will get the config data for the MServ server
-var GetConf = func() *Config {
-	if sConf == nil {
-		sConf = &Config{}
+// Reload the configuration settings from environment and file, in that order.
+func Reload() {
+	sConf = &Config{}
+
+	if err := envconfig.Process(envPrefix, sConf); err != nil {
+		log.WithError(err).Fatal("failed to process config env vars")
+	}
 
-		if err := envconfig.Process(envPrefix, sConf); err != nil {
-			log.WithError(err).Fatal("failed to process config env vars")
-		}
+	// Make sure the config data we're about to use is fresh
+	//
+	// We should consider migrating the 'conf' and 'config' packages to something more capable, like Viper:
+	// https://github.com/spf13/viper#watching-and-re-reading-config-files
+	// https://pkg.go.dev/github.com/spf13/viper#OnConfigChange
+	conf.Flush()
 
-		if err := json.Unmarshal(conf.ReadConf(), sConf); err != nil {
-			log.WithError(err).Fatal("failed to unmarshal mserv driver config")
-		}
+	if err := json.Unmarshal(conf.ReadConf(), sConf); err != nil {
+		log.WithError(err).Fatal("failed to unmarshal mserv driver config")
+	}
+}
+
+// GetConf will get the configuration settings for the MServ server.
+// Subsequent calls will NOT refresh the settings if they have since changed.
+// For that to happen, call Reload first.
+var GetConf = func() *Config {
+	if sConf == nil {
+		Reload()
 	}
 
 	return sConf

http_funcs/api_handlers.go

@@ -182,8 +182,7 @@ func (h *HttpServ) DeleteMW(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	err := h.api.HandleDeleteBundle(id)
-	if err != nil {
+	if err := h.api.HandleDeleteBundle(id); err != nil {
 		h.HandleError(err, w, r)
 		return
 	}