Address comments in review

cmd/snaptel/commands.go

@@ -101,13 +101,13 @@ var (
 			Subcommands: []cli.Command{
 				{
 					Name:   "load",
-					Usage:  "load <plugin_path> [--plugin-cert=<plugin_cert_path> --plugin-key=<plugin_key_path> --plugin-root-certs=<root_cert_paths>]",
+					Usage:  "load <plugin_path> [--plugin-cert=<plugin_cert_path> --plugin-key=<plugin_key_path> --plugin-ca-certs=<ca_cert_paths>]",
 					Action: loadPlugin,
 					Flags: []cli.Flag{
 						flPluginAsc,
 						flPluginCert,
 						flPluginKey,
-						flPluginRootCerts,
+						flPluginCACerts,
 					},
 				},
 				{
@@ -117,7 +117,7 @@ var (
 				},
 				{
 					Name:   "swap",
-					Usage:  "swap <load_plugin_path> <unload_plugin_type>:<unload_plugin_name>:<unload_plugin_version> or swap <load_plugin_path> -t <unload_plugin_type> -n <unload_plugin_name> -v <unload_plugin_version> [--plugin-cert=<plugin_cert_path> --plugin-key=<plugin_key_path> --plugin-root-certs=<root_cert_paths>]",
+					Usage:  "swap <load_plugin_path> <unload_plugin_type>:<unload_plugin_name>:<unload_plugin_version> or swap <load_plugin_path> -t <unload_plugin_type> -n <unload_plugin_name> -v <unload_plugin_version> [--plugin-cert=<plugin_cert_path> --plugin-key=<plugin_key_path> --plugin-ca-certs=<ca_cert_paths>]",
 					Action: swapPlugins,
 					Flags: []cli.Flag{
 						flPluginAsc,
@@ -126,7 +126,7 @@ var (
 						flPluginVersion,
 						flPluginCert,
 						flPluginKey,
-						flPluginRootCerts,
+						flPluginCACerts,
 					},
 				},
 				{

cmd/snaptel/flags.go

@@ -73,15 +73,15 @@ var (
 	}
 	flPluginCert = cli.StringFlag{
 		Name:  "plugin-cert, c",
-		Usage: "The plugin cert",
+		Usage: "The path to plugin certificate file",
 	}
 	flPluginKey = cli.StringFlag{
 		Name:  "plugin-key, k",
-		Usage: "The plugin key",
+		Usage: "The path to plugin private key file",
 	}
-	flPluginRootCerts = cli.StringFlag{
-		Name:  "plugin-root-certs, r",
-		Usage: "List of root cert paths for TLS to use (folder/file)",
+	flPluginCACerts = cli.StringFlag{
+		Name:  "plugin-ca-certs, r",
+		Usage: "List of CA cert paths for plugin (directory/file) to verify TLS clients",
 	}
 	flPluginType = cli.StringFlag{
 		Name:  "plugin-type, t",

cmd/snaptel/plugin.go

@@ -205,13 +205,13 @@ func listPlugins(ctx *cli.Context) error {
 	return nil
 }
 
-// storeTLSPaths extracts paths related to TLS (certificate, key, root certs)
+// storeTLSPaths extracts paths related to TLS (certificate, key, plugin CA certs)
 // from command line context into temporary files. Those files are appended to
 // list of paths returned from this function.
 func storeTLSPaths(ctx *cli.Context, paths []string) ([]string, error) {
 	pCert := ctx.String("plugin-cert")
 	pKey := ctx.String("plugin-key")
-	pRootCertPaths := ctx.String("plugin-root-certs")
+	pCACertPaths := ctx.String("plugin-ca-certs")
 	if pCert != pKey && (pCert == "" || pKey == "") {
 		return paths, fmt.Errorf("Error processing plugin TLS arguments - one of (certificate, key) arguments is missing")
 	}
@@ -237,14 +237,14 @@ func storeTLSPaths(ctx *cli.Context, paths []string) ([]string, error) {
 		}
 		paths = append(paths, tmpFile.Name())
 	}
-	if pRootCertPaths != "" {
-		tmpFile, err := ioutil.TempFile("", v1.TLSRootCertsPrefix)
+	if pCACertPaths != "" {
+		tmpFile, err := ioutil.TempFile("", v1.TLSCACertsPrefix)
 		if err != nil {
-			return paths, fmt.Errorf("Error processing plugin TLS root certificates - unable to create link:\n%v", err.Error())
+			return paths, fmt.Errorf("Error processing plugin TLS CA certificates - unable to create link:\n%v", err.Error())
 		}
-		_, err = tmpFile.WriteString(pRootCertPaths)
+		_, err = tmpFile.WriteString(pCACertPaths)
 		if err != nil {
-			return paths, fmt.Errorf("Error processing plugin TLS root certificates - unable to write link:\n%v", err.Error())
+			return paths, fmt.Errorf("Error processing plugin TLS CA certificates - unable to write link:\n%v", err.Error())
 		}
 		paths = append(paths, tmpFile.Name())
 	}

control/config.go

@@ -50,7 +50,7 @@ var (
 	defaultTempDirPath       = os.TempDir()
 	defaultTLSCertPath       = ""
 	defaultTLSKeyPath        = ""
-	defaultRootCertPaths     = ""
+	defaultCACertPaths       = ""
 )
 
 type pluginConfig struct {
@@ -91,7 +91,7 @@ type Config struct {
 	TempDirPath       string                       `json:"temp_dir_path"yaml:"temp_dir_path"`
 	TLSCertPath       string                       `json:"tls_cert_path"yaml:"tls_cert_path"`
 	TLSKeyPath        string                       `json:"tls_key_path"yaml:"tls_key_path"`
-	RootCertPaths     string                       `json:"root_cert_paths"yaml:"root_cert_paths"`
+	CACertPaths       string                       `json:"ca_cert_paths"yaml:"ca_cert_paths"`
 }
 
 const (
@@ -153,7 +153,7 @@ const (
 					"tls_key_path": {
 						"type": "string"
 					},
-					"root_cert_paths": {
+					"ca_cert_paths": {
 						"type": "string"
 					}
 				},
@@ -180,7 +180,7 @@ func GetDefaultConfig() *Config {
 		TempDirPath:       defaultTempDirPath,
 		TLSCertPath:       defaultTLSCertPath,
 		TLSKeyPath:        defaultTLSKeyPath,
-		RootCertPaths:     defaultRootCertPaths,
+		CACertPaths:       defaultCACertPaths,
 	}
 }
 
@@ -250,7 +250,7 @@ func (p *Config) GetPluginConfigDataNodeAll() cdata.ConfigDataNode {
 	return *p.Plugins.All
 }
 
-// IsTLSEnabled returns true if config values enable TLS security
+// IsTLSEnabled returns true if config values enable TLS in plugin communication
 func (p *Config) IsTLSEnabled() bool {
 	if p.TLSCertPath != "" && p.TLSKeyPath != "" {
 		return true

control/control.go

@@ -217,21 +217,21 @@ func New(cfg *Config) *pluginControl {
 		"_block": "new",
 	}).Debug("metric catalog created")
 
-	// Plugin Manager
 	managerOpts := []pluginManagerOpt{
 		OptSetPprof(cfg.Pprof),
 		OptSetTempDirPath(cfg.TempDirPath),
 	}
+	runnerOpts := []pluginRunnerOpt{}
+	// Plugin Manager
 	if cfg.IsTLSEnabled() {
-		if cfg.RootCertPaths != "" {
-			certPaths := filepath.SplitList(cfg.RootCertPaths)
+		if cfg.CACertPaths != "" {
+			certPaths := filepath.SplitList(cfg.CACertPaths)
 			c.grpcSecurity = client.SecurityTLSExtended(cfg.TLSCertPath, cfg.TLSKeyPath, client.SecureClient, certPaths)
 		} else {
 			c.grpcSecurity = client.SecurityTLSEnabled(cfg.TLSCertPath, cfg.TLSKeyPath, client.SecureClient)
 		}
-	}
-	if cfg.IsTLSEnabled() {
 		managerOpts = append(managerOpts, OptEnableManagerTLS(c.grpcSecurity))
+		runnerOpts = append(runnerOpts, OptEnableRunnerTLS(c.grpcSecurity))
 	}
 	c.pluginManager = newPluginManager(managerOpts...)
 	controlLogger.WithFields(log.Fields{
@@ -247,11 +247,7 @@ func New(cfg *Config) *pluginControl {
 	}).Debug("signing manager created")
 
 	// Plugin Runner
-	if cfg.IsTLSEnabled() {
-		c.pluginRunner = newRunner(OptEnableRunnerTLS(c.grpcSecurity))
-	} else {
-		c.pluginRunner = newRunner()
-	}
+	c.pluginRunner = newRunner(runnerOpts...)
 	controlLogger.WithFields(log.Fields{
 		"_block": "new",
 	}).Debug("runner created")
@@ -604,7 +600,7 @@ func (p *pluginControl) returnPluginDetails(rp *core.RequestedPlugin) (*pluginDe
 	details.Signature = rp.Signature()
 	details.CertPath = rp.CertPath()
 	details.KeyPath = rp.KeyPath()
-	details.RootCertPaths = rp.RootCertPaths()
+	details.CACertPaths = rp.CACertPaths()
 	details.TLSEnabled = rp.TLSEnabled()
 
 	if filepath.Ext(rp.Path()) == ".aci" {

control/control_security_test.go

@@ -91,7 +91,7 @@ func TestSecureCollector(t *testing.T) {
 			ap, err = runPlugin(plugin.Arg{}.
 				SetCertPath(tlsTestSrv+fixtures.TestCrtFileExt).
 				SetKeyPath(tlsTestSrv+fixtures.TestKeyFileExt).
-				SetRootCertPaths(tlsTestCA+fixtures.TestCrtFileExt).
+				SetCACertPaths(tlsTestCA+fixtures.TestCrtFileExt).
 				SetTLSEnabled(true), helper.PluginFilePath("snap-plugin-collector-mock2-grpc"),
 				security)
 			So(err, ShouldBeNil)
@@ -133,7 +133,7 @@ func TestSecureProcessor(t *testing.T) {
 			ap, err = runPlugin(plugin.Arg{}.
 				SetCertPath(tlsTestSrv+fixtures.TestCrtFileExt).
 				SetKeyPath(tlsTestSrv+fixtures.TestKeyFileExt).
-				SetRootCertPaths(tlsTestCA+fixtures.TestCrtFileExt).
+				SetCACertPaths(tlsTestCA+fixtures.TestCrtFileExt).
 				SetTLSEnabled(true), helper.PluginFilePath("snap-plugin-processor-passthru-grpc"),
 				security)
 			So(err, ShouldBeNil)
@@ -171,7 +171,7 @@ func TestSecurePublisher(t *testing.T) {
 			ap, err = runPlugin(plugin.NewArg(int(log.DebugLevel), false).
 				SetCertPath(tlsTestSrv+fixtures.TestCrtFileExt).
 				SetKeyPath(tlsTestSrv+fixtures.TestKeyFileExt).
-				SetRootCertPaths(tlsTestCA+fixtures.TestCrtFileExt).
+				SetCACertPaths(tlsTestCA+fixtures.TestCrtFileExt).
 				SetTLSEnabled(true), helper.PluginFilePath("snap-plugin-publisher-mock-file-grpc"),
 				security)
 			So(err, ShouldBeNil)
@@ -216,7 +216,7 @@ func TestSecureStreamingCollector(t *testing.T) {
 			ap, err = runPlugin(plugin.Arg{}.
 				SetCertPath(tlsTestSrv+fixtures.TestCrtFileExt).
 				SetKeyPath(tlsTestSrv+fixtures.TestKeyFileExt).
-				SetRootCertPaths(tlsTestCA+fixtures.TestCrtFileExt).
+				SetCACertPaths(tlsTestCA+fixtures.TestCrtFileExt).
 				SetTLSEnabled(true), helper.PluginFilePath("snap-plugin-stream-collector-rand1"),
 				security)
 			So(err, ShouldBeNil)
@@ -289,7 +289,7 @@ func TestInsecureConfigurationFails(t *testing.T) {
 		{
 			name: "SecureFrameworkInsecurePlugin_Fail",
 			msg: func(srv *plugin.Arg, cli *client.GRPCSecurity, f func(string)) {
-				// note: server root certs are used to validate client certs (and vice versa)
+				// note: server CA certs are used to validate client certs (and vice versa)
 				*srv = srv.SetTLSEnabled(false)
 				f("Attempting TLS connection between secure framework and insecure plugin")
 			},
@@ -302,17 +302,17 @@ func TestInsecureConfigurationFails(t *testing.T) {
 			},
 		},
 		{
-			name: "BadRootCertInFramework_Fail",
+			name: "BadCACertInFramework_Fail",
 			msg: func(srv *plugin.Arg, cli *client.GRPCSecurity, f func(string)) {
-				cli.RootCertPaths = []string{tlsTestCA + fixtures.TestBadCrtFileExt}
-				f("Attempting TLS connection between framework and plugin using incompatible root certs, bad cert in framework")
+				cli.CACertPaths = []string{tlsTestCA + fixtures.TestBadCrtFileExt}
+				f("Attempting TLS connection between framework and plugin using incompatible CA certs, bad cert in framework")
 			},
 		},
 		{
-			name: "PluginRootCertsUnknownToFramework_Fail",
+			name: "PluginCACertsUnknownToFramework_Fail",
 			msg: func(srv *plugin.Arg, cli *client.GRPCSecurity, f func(string)) {
-				cli.RootCertPaths = []string{}
-				f("Attempting TLS connection between secure framework and plugin with certificate without root certs known to framework")
+				cli.CACertPaths = []string{}
+				f("Attempting TLS connection between secure framework and plugin with certificate without CA certs known to framework")
 			},
 		},
 		{
@@ -330,17 +330,17 @@ func TestInsecureConfigurationFails(t *testing.T) {
 			},
 		},
 		{
-			name: "FrameworkRootCertsUnknownToPlugin_Fail",
+			name: "FrameworkCACertsUnknownToPlugin_Fail",
 			msg: func(srv *plugin.Arg, cli *client.GRPCSecurity, f func(string)) {
-				srv.RootCertPaths = ""
-				f("Attempting TLS connection between invalid framework with no root certs known to plugin and secure plugin")
+				srv.CACertPaths = ""
+				f("Attempting TLS connection between invalid framework with no CA certs known to plugin and secure plugin")
 			},
 		},
 		{
-			name: "BadRootCertInPlugin_Fail",
+			name: "BadCACertInPlugin_Fail",
 			msg: func(srv *plugin.Arg, cli *client.GRPCSecurity, f func(string)) {
-				srv.RootCertPaths = tlsTestCA + fixtures.TestBadCrtFileExt
-				f("Attempting TLS connection between framework and plugin using incompatible root certs, bad cert in plugin")
+				srv.CACertPaths = tlsTestCA + fixtures.TestBadCrtFileExt
+				f("Attempting TLS connection between framework and plugin using incompatible CA certs, bad cert in plugin")
 			},
 		},
 	}
@@ -349,7 +349,7 @@ func TestInsecureConfigurationFails(t *testing.T) {
 		pluginArgs := plugin.Arg{}.
 			SetCertPath(tlsTestSrv + fixtures.TestCrtFileExt).
 			SetKeyPath(tlsTestSrv + fixtures.TestKeyFileExt).
-			SetRootCertPaths(tlsTestCA + fixtures.TestCrtFileExt).
+			SetCACertPaths(tlsTestCA + fixtures.TestCrtFileExt).
 			SetTLSEnabled(true)
 		runThisCase := func(f func(msg string)) {
 			t.Run(tc.name, func(_ *testing.T) {
@@ -386,17 +386,17 @@ func TestInsecureConfigurationFails(t *testing.T) {
 	}
 }
 
-func (m *configTLSMock) setRootCertPaths(rootCertPaths string) *configTLSMock {
-	m.RootCertPaths = rootCertPaths
+func (m *configTLSMock) setCACertPaths(caCertPaths string) *configTLSMock {
+	m.CACertPaths = caCertPaths
 	return m
 }
 
 func TestSecuritySetupFromConfig(t *testing.T) {
 	var (
-		fakeSampleCert           = "/fake-samples/certs/server-cert"
-		fakeSampleKey            = "/fake-samples/keys/server-key"
-		fakeSampleRootCertsSplit = []string{"/fake-samples/root-ca/ca-one", "/fake-samples/root-ca/ca-two"}
-		fakeSampleRootCerts      = strings.Join(fakeSampleRootCertsSplit, string(filepath.ListSeparator))
+		fakeSampleCert         = "/fake-samples/certs/server-cert"
+		fakeSampleKey          = "/fake-samples/keys/server-key"
+		fakeSampleCACertsSplit = []string{"/fake-samples/root-ca/ca-one", "/fake-samples/root-ca/ca-two"}
+		fakeSampleCACerts      = strings.Join(fakeSampleCACertsSplit, string(filepath.ListSeparator))
 	)
 	tcs := []struct {
 		name           string
@@ -430,18 +430,18 @@ func TestSecuritySetupFromConfig(t *testing.T) {
 			wantManagersec: client.SecurityTLSEnabled(fakeSampleCert, fakeSampleKey, client.SecureClient),
 		},
 		{
-			name: "TLSEnabledRootCertsForwardedToSubmodules",
+			name: "TLSEnabledCACertsForwardedToSubmodules",
 			msg: func(f func(string)) {
-				f("having TLS enabled with root cert paths in config, plugin runner and manager receive same security values")
+				f("having TLS enabled with CA cert paths in config, plugin runner and manager receive same security values")
 			},
 			cfg: (*configTLSMock)(GetDefaultConfig()).
 				setTLSCertPath(fakeSampleCert).
 				setTLSKeyPath(fakeSampleKey).
-				setRootCertPaths(fakeSampleRootCerts).
+				setCACertPaths(fakeSampleCACerts).
 				export(),
 			wantError:      false,
-			wantRunnersec:  client.SecurityTLSExtended(fakeSampleCert, fakeSampleKey, client.SecureClient, fakeSampleRootCertsSplit),
-			wantManagersec: client.SecurityTLSExtended(fakeSampleCert, fakeSampleKey, client.SecureClient, fakeSampleRootCertsSplit),
+			wantRunnersec:  client.SecurityTLSExtended(fakeSampleCert, fakeSampleKey, client.SecureClient, fakeSampleCACertsSplit),
+			wantManagersec: client.SecurityTLSExtended(fakeSampleCert, fakeSampleKey, client.SecureClient, fakeSampleCACertsSplit),
 		},
 	}
 	var gotRunner *runner
@@ -484,7 +484,7 @@ func TestSecuritySetupFromConfig(t *testing.T) {
 }
 
 func runPlugin(args plugin.Arg, pluginPath string, security client.GRPCSecurity) (*availablePlugin, error) {
-	ep, err := fixtures.NewExecutablePlugin(args, pluginPath)
+	ep, err := plugin.NewExecutablePlugin(args, pluginPath)
 	if err != nil {
 		panic(err)
 	}

control/fixtures/fixtures.go

@@ -24,7 +24,6 @@ import (
 	"encoding/json"
 	"time"
 
-	"github.com/intelsdi-x/snap/control/plugin"
 	"github.com/intelsdi-x/snap/core"
 	"github.com/intelsdi-x/snap/core/cdata"
 	"github.com/intelsdi-x/snap/plugin/helper"
@@ -157,19 +156,19 @@ func (m MockRequestedMetric) Namespace() core.Namespace {
 	return m.namespace
 }
 
-func NewExecutablePlugin(a plugin.Arg, path string) (*plugin.ExecutablePlugin, error) {
-	// Travis optimization: Try starting the plugin three times before finally
-	// returning an error
-	var e error
-	var ep *plugin.ExecutablePlugin
-	for i := 0; i < 3; i++ {
-		ep, e = plugin.NewExecutablePlugin(a, path)
-		if e == nil {
-			break
-		}
-		if e != nil && i == 2 {
-			return nil, e
-		}
-	}
-	return ep, nil
-}
+// func NewExecutablePlugin(a plugin.Arg, path string) (*plugin.ExecutablePlugin, error) {
+// 	// Travis optimization: Try starting the plugin three times before finally
+// 	// returning an error
+// 	var e error
+// 	var ep *plugin.ExecutablePlugin
+// 	for i := 0; i < 3; i++ {
+// 		ep, e = plugin.NewExecutablePlugin(a, path)
+// 		if e == nil {
+// 			break
+// 		}
+// 		if e != nil && i == 2 {
+// 			return nil, e
+// 		}
+// 	}
+// 	return ep, nil
+// }