veyron/services/mgmt/node/impl: use helper for deletions
When using the suidhelper to launch applications under a different
system account than the one used by the node manager, application
workspace and log state will cannot be deleted by the node manager.
Instead, use the suidhelper to delete state in this context.
Change-Id: I1ad307bbc5dbd2520ed28c65208e0650e6811958
diff --git a/services/mgmt/node/impl/app_invoker.go b/services/mgmt/node/impl/app_invoker.go
index 5832b36..5651b7b 100644
--- a/services/mgmt/node/impl/app_invoker.go
+++ b/services/mgmt/node/impl/app_invoker.go
@@ -300,7 +300,7 @@
installationID := generateID()
installationDir := filepath.Join(i.config.Root, applicationDirName(envelope.Title), installationDirName(installationID))
deferrer := func() {
- cleanupDir(installationDir)
+ cleanupDir(installationDir, "")
}
defer func() {
if deferrer != nil {
@@ -559,25 +559,26 @@
}
func (i *appInvoker) Start(call ipc.ServerContext) ([]string, error) {
+ helper := i.config.Helper
instanceDir, instanceID, err := i.newInstance()
if err != nil {
- cleanupDir(instanceDir)
+ cleanupDir(instanceDir, helper)
return nil, err
}
- systemName, err := systemAccountForHelper(i.config.Helper, call.RemoteBlessings().ForContext(call), i.uat)
+ systemName, err := systemAccountForHelper(helper, call.RemoteBlessings().ForContext(call), i.uat)
if err != nil {
- cleanupDir(instanceDir)
+ cleanupDir(instanceDir, helper)
return nil, err
}
if err := saveSystemNameForInstance(instanceDir, systemName); err != nil {
- cleanupDir(instanceDir)
+ cleanupDir(instanceDir, helper)
return nil, err
}
if err = i.run(instanceDir, systemName); err != nil {
- cleanupDir(instanceDir)
+ cleanupDir(instanceDir, helper)
return nil, err
}
return []string{instanceID}, nil
@@ -747,7 +748,7 @@
}
versionDir, err := newVersion(installationDir, newEnvelope, oldVersionDir)
if err != nil {
- cleanupDir(versionDir)
+ cleanupDir(versionDir, "")
return err
}
return nil
diff --git a/services/mgmt/node/impl/impl_helper_test.go b/services/mgmt/node/impl/impl_helper_test.go
new file mode 100644
index 0000000..1eaaeda
--- /dev/null
+++ b/services/mgmt/node/impl/impl_helper_test.go
@@ -0,0 +1,45 @@
+package impl_test
+
+// Separate from impl_test to avoid contributing further to impl_test bloat.
+// TODO(rjkroege): Move all helper-related tests to here.
+
+import (
+ "io/ioutil"
+ "os"
+ "path"
+ "testing"
+
+ "veyron.io/veyron/veyron/services/mgmt/node/impl"
+)
+
+func TestBaseCleanupDir(t *testing.T) {
+ dir, err := ioutil.TempDir("", "impl_helper_test")
+ if err != nil {
+ t.Fatalf("ioutil.TempDir() failed: %v", err)
+ }
+ defer os.RemoveAll(dir)
+
+ // Setup some files to delete.
+ helperTarget := path.Join(dir, "helper_target")
+ if err := os.MkdirAll(helperTarget, os.FileMode(0700)); err != nil {
+ t.Fatalf("os.MkdirAll(%s) failed: %v", helperTarget, err)
+ }
+
+ nohelperTarget := path.Join(dir, "nohelper_target")
+ if err := os.MkdirAll(nohelperTarget, os.FileMode(0700)); err != nil {
+ t.Fatalf("os.MkdirAll(%s) failed: %v", nohelperTarget, err)
+ }
+
+ // Setup a helper.
+ helper := generateSuidHelperScript(t, dir)
+
+ impl.WrapBaseCleanupDir(helperTarget, helper)
+ if _, err := os.Stat(helperTarget); err == nil || os.IsExist(err) {
+ t.Fatalf("%s should be missing but isn't", helperTarget)
+ }
+
+ impl.WrapBaseCleanupDir(nohelperTarget, "")
+ if _, err := os.Stat(nohelperTarget); err == nil || os.IsExist(err) {
+ t.Fatalf("%s should be missing but isn't", nohelperTarget)
+ }
+}
diff --git a/services/mgmt/node/impl/impl_test.go b/services/mgmt/node/impl/impl_test.go
index a5670c2..47b7b86 100644
--- a/services/mgmt/node/impl/impl_test.go
+++ b/services/mgmt/node/impl/impl_test.go
@@ -276,30 +276,7 @@
return path
}
-// generateSuidHelperScript builds a script to execute the test target as
-// a suidhelper instance and installs it in <root>/helper.
-func generateSuidHelperScript(t *testing.T, root string) string {
- output := "#!/bin/bash\n"
- output += "VEYRON_SUIDHELPER_TEST=1"
- output += " "
- output += "exec" + " " + os.Args[0] + " " + "-minuid=1" + " " + "-test.run=TestSuidHelper $*"
- output += "\n"
-
- vlog.VI(1).Infof("script\n%s", output)
-
- if err := os.MkdirAll(root, 0755); err != nil {
- t.Fatalf("MkdirAll failed: %v", err)
- }
- // Helper does not need to live under the node manager's root dir, but
- // we put it there for convenience.
- path := filepath.Join(root, "helper.sh")
- if err := ioutil.WriteFile(path, []byte(output), 0755); err != nil {
- t.Fatalf("WriteFile(%v) failed: %v", path, err)
- }
- return path
-}
-
-// readPID waits for the "ready:<PID>" line from the child and parses out the
+/// readPID waits for the "ready:<PID>" line from the child and parses out the
// PID of the child.
func readPID(t *testing.T, s *expect.Session) int {
m := s.ExpectRE("ready:([0-9]+)", -1)
diff --git a/services/mgmt/node/impl/node_invoker.go b/services/mgmt/node/impl/node_invoker.go
index 309d867..f731623 100644
--- a/services/mgmt/node/impl/node_invoker.go
+++ b/services/mgmt/node/impl/node_invoker.go
@@ -343,7 +343,7 @@
}
deferrer := func() {
- cleanupDir(workspace)
+ cleanupDir(workspace, "")
}
defer func() {
if deferrer != nil {
diff --git a/services/mgmt/node/impl/only_for_test.go b/services/mgmt/node/impl/only_for_test.go
index 3261c42..b110699 100644
--- a/services/mgmt/node/impl/only_for_test.go
+++ b/services/mgmt/node/impl/only_for_test.go
@@ -24,9 +24,14 @@
}
func init() {
- cleanupDir = func(dir string) {
+ cleanupDir = func(dir, helper string) {
parentDir, base := filepath.Dir(dir), filepath.Base(dir)
- renamed := filepath.Join(parentDir, "deleted_"+base)
+ var renamed string
+ if helper != "" {
+ renamed = filepath.Join(parentDir, "helper_deleted_"+base)
+ } else {
+ renamed = filepath.Join(parentDir, "deleted_"+base)
+ }
if err := os.Rename(dir, renamed); err != nil {
vlog.Errorf("Rename(%v, %v) failed: %v", dir, renamed, err)
}
@@ -38,3 +43,7 @@
vlog.VI(2).Infof("Mock isSetuid is reporting: %v", *mockIsSetuid)
return *mockIsSetuid
}
+
+func WrapBaseCleanupDir(path, helper string) {
+ baseCleanupDir(path, helper)
+}
diff --git a/services/mgmt/node/impl/util.go b/services/mgmt/node/impl/util.go
index 5f719b4..e28c4ea 100644
--- a/services/mgmt/node/impl/util.go
+++ b/services/mgmt/node/impl/util.go
@@ -3,6 +3,7 @@
import (
"io/ioutil"
"os"
+ "os/exec"
"path/filepath"
"time"
@@ -80,10 +81,24 @@
return nil
}
-// cleanupDir is defined like this so we can override its implementation for
-// tests.
-var cleanupDir = func(dir string) {
- if err := os.RemoveAll(dir); err != nil {
- vlog.Errorf("RemoveAll(%v) failed: %v", dir, err)
+func baseCleanupDir(path, helper string) {
+ if helper != "" {
+ out, err := exec.Command(helper, "--rm", path).CombinedOutput()
+ if err != nil {
+ vlog.Errorf("exec.Command(%s %s %s).CombinedOutput() failed: %v", helper, "--rm", path, err)
+ return
+ }
+ if len(out) != 0 {
+ vlog.Errorf("exec.Command(%s %s %s).CombinedOutput() generated output: %v", helper, "--rm", path, string(out))
+ }
+ } else {
+ if err := os.RemoveAll(path); err != nil {
+ vlog.Errorf("RemoveAll(%v) failed: %v", path, err)
+ }
}
}
+
+// cleanupDir is defined like this so we can override its implementation for
+// tests. cleanupDir will use the helper to delete application state possibly
+// owned by different accounts if helper is provided.
+var cleanupDir = baseCleanupDir
diff --git a/services/mgmt/node/impl/util_test.go b/services/mgmt/node/impl/util_test.go
index fef9b36..1011dd9 100644
--- a/services/mgmt/node/impl/util_test.go
+++ b/services/mgmt/node/impl/util_test.go
@@ -338,3 +338,26 @@
t.Fatalf("ListAssociations() got %v, expected %v", got, expected)
}
}
+
+// generateSuidHelperScript builds a script to execute the test target as
+// a suidhelper instance and returns the path to the script.
+func generateSuidHelperScript(t *testing.T, root string) string {
+ output := "#!/bin/bash\n"
+ output += "VEYRON_SUIDHELPER_TEST=1"
+ output += " "
+ output += "exec" + " " + os.Args[0] + " " + "-minuid=1" + " " + "-test.run=TestSuidHelper $*"
+ output += "\n"
+
+ vlog.VI(1).Infof("script\n%s", output)
+
+ if err := os.MkdirAll(root, 0755); err != nil {
+ t.Fatalf("MkdirAll failed: %v", err)
+ }
+ // Helper does not need to live under the node manager's root dir, but
+ // we put it there for convenience.
+ path := filepath.Join(root, "helper.sh")
+ if err := ioutil.WriteFile(path, []byte(output), 0755); err != nil {
+ t.Fatalf("WriteFile(%v) failed: %v", path, err)
+ }
+ return path
+}
