Fix agent rollback does not delete created dirs

sdk/config_apply.go

@@ -7,6 +7,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"strings"
 
 	crossplane "github.com/nginxinc/nginx-go-crossplane"
 	log "github.com/sirupsen/logrus"
@@ -18,9 +19,10 @@ import (
 // of the current files, mark them off as they are getting applied, and delete any leftovers that's not in the incoming
 // apply payload.
 type ConfigApply struct {
-	writer    *zip.Writer
-	existing  map[string]struct{}
-	notExists map[string]struct{} // set of files that exists in the config provided payload, but not on disk
+	writer       *zip.Writer
+	existing     map[string]struct{}
+	notExists    map[string]struct{} // set of files that exists in the config provided payload, but not on disk
+	notExistDirs map[string]struct{} // set of directories that exists in the config provided payload, but not on disk
 }
 
 func NewConfigApply(
@@ -32,9 +34,10 @@ func NewConfigApply(
 		return nil, err
 	}
 	b := &ConfigApply{
-		writer:    w,
-		existing:  make(map[string]struct{}),
-		notExists: make(map[string]struct{}),
+		writer:       w,
+		existing:     make(map[string]struct{}),
+		notExists:    make(map[string]struct{}),
+		notExistDirs: make(map[string]struct{}),
 	}
 	if confFile != "" {
 		return b, b.mapCurrentFiles(confFile, allowedDirectories)
@@ -64,6 +67,13 @@ func (b *ConfigApply) Rollback(cause error) error {
 		}
 	}
 
+	for fullPath := range b.notExistDirs {
+		err = os.RemoveAll(fullPath)
+		if err != nil {
+			log.Warnf("error during rollback (remove dir) for %s: %s", fullPath, err)
+		}
+	}
+
 	r.RangeFileReaders(func(innerErr error, path string, mode os.FileMode, r io.Reader) bool {
 		if innerErr != nil {
 			log.Warnf("error during rollback for %s: %s", path, innerErr)
@@ -109,16 +119,43 @@ func (b *ConfigApply) Complete() error {
 func (b *ConfigApply) MarkAndSave(fullPath string) error {
 	// delete from existing list, so we don't delete them during Complete
 	delete(b.existing, fullPath)
+
 	p, err := os.Stat(fullPath)
 	if err != nil {
 		if errors.Is(err, os.ErrNotExist) {
 			b.notExists[fullPath] = struct{}{}
 			log.Debugf("backup: %s does not exist", fullPath)
+
+			for dir := range b.notExistDirs {
+				if strings.HasPrefix(fullPath, dir) {
+					return nil
+				}
+			}
+
+			paths := strings.Split(fullPath, "/")
+			for i := 2; i < len(paths); i++ {
+				dirPath := strings.Join(paths[0:i], "/")
+
+				_, err := os.Stat(dirPath)
+				if err != nil {
+					if errors.Is(err, os.ErrNotExist) {
+						b.notExistDirs[dirPath] = struct{}{}
+						log.Debugf("backup: dir %s does not exist", dirPath)
+						return nil
+					}
+
+					log.Warnf("backup: dir %s error: %s", dirPath, err)
+					return err
+				}
+			}
+
 			return nil
 		}
+
 		log.Warnf("backup: %s error: %s", fullPath, err)
 		return err
 	}
+
 	r, err := os.Open(fullPath)
 	if err != nil {
 		log.Warnf("backup: %s open error: %s", fullPath, err)
@@ -210,3 +247,7 @@ func (b *ConfigApply) GetExisting() map[string]struct{} {
 func (b *ConfigApply) GetNotExists() map[string]struct{} {
 	return b.notExists
 }
+
+func (b *ConfigApply) GetNotExistDirs() map[string]struct{} {
+	return b.notExistDirs
+}

sdk/config_apply_test.go

@@ -10,6 +10,7 @@ import (
 
 	"github.com/nginx/agent/sdk/v2/zip"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 var (
@@ -65,27 +66,27 @@ var (
 func TestNewConfigApply(t *testing.T) {
 	tmpDir := t.TempDir()
 	rootDirectory := path.Join(tmpDir, "root/")
-	assert.NoError(t, os.Mkdir(rootDirectory, os.ModePerm))
+	require.NoError(t, os.Mkdir(rootDirectory, os.ModePerm))
 
 	rootFile1 := path.Join(rootDirectory, "root1.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile1, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile1, []byte{}, 0644))
 
 	rootFile2 := path.Join(rootDirectory, "root2.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile2, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile2, []byte{}, 0644))
 
 	rootFile3 := path.Join(rootDirectory, "root3.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile3, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile3, []byte{}, 0644))
 
 	emptyConfFile := path.Join(tmpDir, "empty_nginx.conf")
-	assert.NoError(t, ioutil.WriteFile(emptyConfFile, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(emptyConfFile, []byte{}, 0644))
 
 	defaultConfFile := path.Join(tmpDir, "default_nginx.conf")
 	defaultConfFileContent := fmt.Sprintf(defaultConfFileContentsString, rootDirectory, rootDirectory)
-	assert.NoError(t, ioutil.WriteFile(defaultConfFile, []byte(defaultConfFileContent), 0644))
+	require.NoError(t, ioutil.WriteFile(defaultConfFile, []byte(defaultConfFileContent), 0644))
 
 	confFile := path.Join(tmpDir, "nginx.conf")
 	confFileContent := fmt.Sprintf(confFileContentsString, defaultConfFile)
-	assert.NoError(t, ioutil.WriteFile(confFile, []byte(confFileContent), 0644))
+	require.NoError(t, ioutil.WriteFile(confFile, []byte(confFileContent), 0644))
 
 	testCases := []struct {
 		name                string
@@ -108,7 +109,8 @@ func TestNewConfigApply(t *testing.T) {
 					rootFile2:       {},
 					rootFile3:       {},
 				},
-				notExists: map[string]struct{}{},
+				notExists:    map[string]struct{}{},
+				notExistDirs: map[string]struct{}{},
 			},
 			expectError: false,
 		},
@@ -117,8 +119,9 @@ func TestNewConfigApply(t *testing.T) {
 			confFile:           "",
 			allowedDirectories: map[string]struct{}{},
 			expectedConfigApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{},
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{},
+				notExistDirs: map[string]struct{}{},
 			},
 			expectError: false,
 		},
@@ -127,8 +130,9 @@ func TestNewConfigApply(t *testing.T) {
 			confFile:           emptyConfFile,
 			allowedDirectories: map[string]struct{}{},
 			expectedConfigApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{},
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{},
+				notExistDirs: map[string]struct{}{},
 			},
 			expectError: false,
 		},
@@ -137,8 +141,9 @@ func TestNewConfigApply(t *testing.T) {
 			confFile:           "/tmp/unknown.conf",
 			allowedDirectories: map[string]struct{}{},
 			expectedConfigApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{},
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{},
+				notExistDirs: map[string]struct{}{},
 			},
 			expectError: true,
 		},
@@ -149,6 +154,7 @@ func TestNewConfigApply(t *testing.T) {
 			configApply, err := NewConfigApply(tc.confFile, tc.allowedDirectories)
 			assert.Equal(t, tc.expectedConfigApply.existing, configApply.GetExisting())
 			assert.Equal(t, tc.expectedConfigApply.notExists, configApply.GetNotExists())
+			assert.Equal(t, tc.expectedConfigApply.notExistDirs, configApply.GetNotExistDirs())
 			if tc.expectError {
 				assert.NotNil(t, err)
 			} else {
@@ -162,75 +168,96 @@ func TestConfigApplyMarkAndSave(t *testing.T) {
 	tmpDir := t.TempDir()
 	unknownFile := path.Join(tmpDir, "unknown.conf")
 	knownFile := path.Join(tmpDir, "known.conf")
-	assert.NoError(t, ioutil.WriteFile(knownFile, []byte{}, 0644))
+	unknownFileUnknownDir := path.Join(tmpDir, "/unknown/unknown.conf")
+	unknownFileUnknownNestedDirs := path.Join(tmpDir, "/unknown/nested/unknown.conf")
+
+	require.NoError(t, ioutil.WriteFile(knownFile, []byte{}, 0644))
 
 	writer, err := zip.NewWriter("/")
-	assert.Nil(t, err)
+	require.NoError(t, err)
 
 	testCases := []struct {
 		name                string
 		file                string
-		configApply         *ConfigApply
 		expectedConfigApply *ConfigApply
 	}{
 		{
 			name: "file doesn't exist",
 			file: unknownFile,
-			configApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{},
-			},
 			expectedConfigApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{unknownFile: {}},
-				writer:    writer,
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{unknownFile: {}},
+				notExistDirs: map[string]struct{}{},
+				writer:       writer,
 			},
 		},
 		{
 			name: "file exists",
 			file: knownFile,
-			configApply: &ConfigApply{
-				existing:  map[string]struct{}{knownFile: {}},
-				notExists: map[string]struct{}{},
-				writer:    writer,
+			expectedConfigApply: &ConfigApply{
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{},
+				notExistDirs: map[string]struct{}{},
+			},
+		},
+		{
+			name: "file doesn't exist and dir doesn't exist",
+			file: unknownFileUnknownDir,
+			expectedConfigApply: &ConfigApply{
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{unknownFileUnknownDir: {}},
+				notExistDirs: map[string]struct{}{path.Dir(unknownFileUnknownDir): {}},
 			},
+		},
+		{
+			name: "file doesn't exist and nested new dirs don't exist",
+			file: unknownFileUnknownNestedDirs,
 			expectedConfigApply: &ConfigApply{
-				existing:  map[string]struct{}{},
-				notExists: map[string]struct{}{},
+				existing:     map[string]struct{}{},
+				notExists:    map[string]struct{}{unknownFileUnknownNestedDirs: {}},
+				notExistDirs: map[string]struct{}{path.Dir(unknownFileUnknownDir): {}},
 			},
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			assert.NoError(t, tc.configApply.MarkAndSave(tc.file))
-			assert.Equal(t, tc.expectedConfigApply.existing, tc.configApply.GetExisting())
-			assert.Equal(t, tc.expectedConfigApply.notExists, tc.configApply.GetNotExists())
+			configApply := &ConfigApply{
+				existing:     make(map[string]struct{}),
+				notExists:    make(map[string]struct{}),
+				notExistDirs: make(map[string]struct{}),
+				writer:       writer,
+			}
+
+			assert.NoError(t, configApply.MarkAndSave(tc.file))
+			assert.Equal(t, tc.expectedConfigApply.existing, configApply.GetExisting())
+			assert.Equal(t, tc.expectedConfigApply.notExists, configApply.GetNotExists())
+			assert.Equal(t, tc.expectedConfigApply.notExistDirs, configApply.GetNotExistDirs())
 		})
 	}
 }
 
 func TestConfigApplyCompleteAndRollback(t *testing.T) {
 	tmpDir := t.TempDir()
 	rootDirectory := path.Join(tmpDir, "root/")
-	assert.NoError(t, os.Mkdir(rootDirectory, os.ModePerm))
+	require.NoError(t, os.Mkdir(rootDirectory, os.ModePerm))
 
 	rootFile1 := path.Join(rootDirectory, "root1.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile1, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile1, []byte{}, 0644))
 
 	rootFile2 := path.Join(rootDirectory, "root2.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile2, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile2, []byte{}, 0644))
 
 	rootFile3 := path.Join(rootDirectory, "root3.html")
-	assert.NoError(t, ioutil.WriteFile(rootFile3, []byte{}, 0644))
+	require.NoError(t, ioutil.WriteFile(rootFile3, []byte{}, 0644))
 
 	defaultConfFile := path.Join(tmpDir, "default_nginx.conf")
 	defaultConfFileContent := fmt.Sprintf(defaultConfFileContentsString, rootDirectory, rootDirectory)
-	assert.NoError(t, ioutil.WriteFile(defaultConfFile, []byte(defaultConfFileContent), 0644))
+	require.NoError(t, ioutil.WriteFile(defaultConfFile, []byte(defaultConfFileContent), 0644))
 
 	confFile := path.Join(tmpDir, "nginx.conf")
 	confFileContent := fmt.Sprintf(confFileContentsString, defaultConfFile)
-	assert.NoError(t, ioutil.WriteFile(confFile, []byte(confFileContent), 0644))
+	require.NoError(t, ioutil.WriteFile(confFile, []byte(confFileContent), 0644))
 
 	allowedDirectories := map[string]struct{}{tmpDir: {}}
 

sdk/zip/zipped_file_test.go

@@ -36,7 +36,8 @@ func TestWriter(t *testing.T) {
 			// should not be able to double flush
 			prefix: "/root", preProtoCallback: func(f *Writer) {
 				_, _ = f.Proto()
-			}, protoErr: true,
+			},
+			protoErr: true,
 		},
 		{
 			prefix: "/root",

src/core/environment.go

@@ -162,10 +162,12 @@ func (env *EnvironmentType) WriteFiles(backup ConfigApplyMarker, files []*proto.
 }
 
 func (env *EnvironmentType) IsContainer() bool {
-	const dockerEnv = "/.dockerenv"
-	const containerEnv = "/run/.containerenv"
-	const selfCgroup = "/proc/self/cgroup"
-	const k8sServiceAcct = "/var/run/secrets/kubernetes.io/serviceaccount"
+	const (
+		dockerEnv      = "/.dockerenv"
+		containerEnv   = "/run/.containerenv"
+		selfCgroup     = "/proc/self/cgroup"
+		k8sServiceAcct = "/var/run/secrets/kubernetes.io/serviceaccount"
+	)
 
 	for _, filename := range []string{dockerEnv, containerEnv, k8sServiceAcct} {
 		if _, err := os.Stat(filename); err == nil {