Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Several 2.0 regression fixes #876

Merged
merged 9 commits into from
Mar 30, 2017
Merged

Several 2.0 regression fixes #876

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd9db5e
Minor cosmetic commit
kontsevoy Mar 26, 2017
1755870
Logging fixes and more
kontsevoy Mar 26, 2017
35f80ca
Added 'data_dir' value to backend params
kontsevoy Mar 26, 2017
1cab059
Changes to Unix socket path to make it cross platform.
russjones Mar 28, 2017
d304e47
Use ioutil.TempDir to create path to socket instead.
russjones Mar 29, 2017
81b379e
Remove log line that contains err.
russjones Mar 29, 2017
fd46d88
try to fix the test
klizhentas Mar 30, 2017
835a754
Fixed Ansible regressions and added documentation for docker build.
russjones Mar 30, 2017
e9b1754
Added Ansible playbook for testing and docs on how to run it.
russjones Mar 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.assets/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,4 +77,4 @@ enter: bbox
#
.PHONY:release
release:
docker run $(DOCKERFLAGS) -i $(NOROOT) $(BBOX) /usr/bin/make release -e VERSION="$(VERSION)" -e ADDFLAGS="$(ADDFLAGS)"
docker run $(DOCKERFLAGS) -i $(NOROOT) $(BBOX) /usr/bin/make release -e ADDFLAGS="$(ADDFLAGS)"
13 changes: 2 additions & 11 deletions lib/client/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@ type Config struct {
// HostLogin is a user login on a remote host
HostLogin string

// HostPort is a remote host port to connect to
// HostPort is a remote host port to connect to. This is used for **explicit**
// port setting via -p flag, otherwise '0' is passed which means "use server default"
HostPort int

// ProxyHostPort is a host or IP of the proxy (with optional ":ssh_port,https_port").
Expand Down Expand Up @@ -276,16 +277,6 @@ func (c *Config) ProxySSHPort() (retval int) {
return retval
}

// NodeHostPort returns host:port string based on user supplied data
// either if user has set host:port in the connection string,
// or supplied the -p flag. If user has set both, -p flag data is ignored
func (c *Config) NodeHostPort() string {
if strings.Contains(c.Host, ":") {
return c.Host
}
return net.JoinHostPort(c.Host, strconv.Itoa(c.HostPort))
}

// ProxySpecified returns true if proxy has been specified
func (c *Config) ProxySpecified() bool {
return len(c.ProxyHostPort) > 0
Expand Down
1 change: 0 additions & 1 deletion lib/client/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ func (s *APITestSuite) TestNew(c *check.C) {

la := tc.LocalAgent()
c.Assert(la, check.NotNil)
c.Assert(tc.NodeHostPort(), check.Equals, "localhost:22")
}

func (s *APITestSuite) TestParseLabels(c *check.C) {
Expand Down
13 changes: 12 additions & 1 deletion lib/config/configuration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"golang.org/x/crypto/ssh"

"github.com/gravitational/teleport"
"github.com/gravitational/teleport/lib/backend"
"github.com/gravitational/teleport/lib/client"
"github.com/gravitational/teleport/lib/defaults"
"github.com/gravitational/teleport/lib/limiter"
Expand Down Expand Up @@ -191,6 +192,10 @@ func ApplyFileConfig(fc *FileConfig, cfg *service.Config) error {
return trace.Errorf("unsupported logger severity: '%v'", fc.Logger.Severity)
}

if strings.ToLower(fc.Logger.Output) == "syslog" {
utils.SwitchLoggingtoSyslog()
}

// apply connection throttling:
limiters := []limiter.LimiterConfig{
cfg.SSH.Limiter,
Expand Down Expand Up @@ -545,7 +550,7 @@ func Configure(clf *CommandLineFlags, cfg *service.Config) error {
}

cfg.Console = ioutil.Discard
utils.InitLoggerDebug()
utils.InitLogger(utils.LoggingForDaemon, log.DebugLevel)
}

// apply --roles flag:
Expand Down Expand Up @@ -608,6 +613,12 @@ func Configure(clf *CommandLineFlags, cfg *service.Config) error {
cfg.AuthServers = append(cfg.AuthServers, cfg.Auth.SSHAddr)
}

// add data_dir to the backend config:
if cfg.Auth.StorageConfig.Params == nil {
cfg.Auth.StorageConfig.Params = backend.Params{}
}
cfg.Auth.StorageConfig.Params["data_dir"] = cfg.DataDir
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russjones this overwrites the user-supplied value at all times, what should not be the case. We should only overwrite the value if user haven't supplied the value. You can fix that yourself as @kontsevoy is OOO at the moment.

Copy link
Contributor Author

@kontsevoy kontsevoy Mar 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klizhentas there are no user-supplied values in StorageConfig.Params [1]. Storage config is a static, backend-specific YAML configuration, with "data_dir" being a reserved word. In fact, we guarantee the presence of "data_dir" in .Params (and back-end code expects it to be there) This needs to be reflected in backend developer README. In other words, this behavior is as-designed.

[1] Bolt used to accept an arbitrary JSON string, so that's why you think there are user-supplied values. This is no longer true since the Great Christmas-2016 Back-end Redesign

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users can supply Params["path"] for backend, at least what I have observed in configs and the code, so I don't quite understand the fix then. Why is it setting "data_dir" and not "path" like for example "boltdb" requires.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question basically is - can you explain to me how does this change fixes the case reported here:

#867

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@russjones: @kontsevoy explained to me how it works, so this part lgtm, please fix the part with socket path and merge the whole thing


return nil
}

Expand Down
4 changes: 2 additions & 2 deletions lib/services/role.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,8 +266,8 @@ func (r *RoleV2) CheckAndSetDefaults() error {
}

func (r *RoleV2) String() string {
return fmt.Sprintf("Role(Name=%v,MaxSessionTTL=%v,Logins=%v,NodeLabels=%v,Namespaces=%v,Resources=%v)",
r.GetName(), r.GetMaxSessionTTL(), r.GetLogins(), r.GetNodeLabels(), r.GetNamespaces(), r.GetResources())
return fmt.Sprintf("Role(Name=%v,MaxSessionTTL=%v,Logins=%v,NodeLabels=%v,Namespaces=%v,Resources=%v,CanForwardAgent=%v)",
r.GetName(), r.GetMaxSessionTTL(), r.GetLogins(), r.GetNodeLabels(), r.GetNamespaces(), r.GetResources(), r.CanForwardAgent())
}

// RoleSpecV2 is role specification for RoleV2
Expand Down
17 changes: 16 additions & 1 deletion lib/srv/exec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ package srv
import (
"fmt"
"net"
"os"
"os/user"
"path"
"path/filepath"

"gopkg.in/check.v1"

Expand Down Expand Up @@ -81,7 +84,7 @@ func (s *ExecSuite) TestOSCommandPrep(c *check.C) {
cmd, err = prepareCommand(s.ctx)
c.Assert(err, check.IsNil)
c.Assert(cmd, check.NotNil)
c.Assert(cmd.Path, check.Equals, "/bin/ls")
c.Assert(cmd.Path, check.Equals, findExecutable("ls"))
c.Assert(cmd.Args, check.DeepEquals, []string{"ls", "-lh", "/etc"})
c.Assert(cmd.Dir, check.Equals, s.usr.HomeDir)
c.Assert(cmd.Env, check.DeepEquals, expectedEnv)
Expand Down Expand Up @@ -112,3 +115,15 @@ func (s *ExecSuite) OpenChannel(string, []byte) (ssh.Channel, <-chan *ssh.Reques
return nil, nil, nil
}
func (s *ExecSuite) Wait() error { return nil }

// findExecutable helper finds a given executable name (like 'ls') in $PATH
// and returns the full path
func findExecutable(execName string) string {
for _, dir := range filepath.SplitList(os.Getenv("PATH")) {
fp := path.Join(dir, execName)
if utils.IsFile(fp) {
return fp
}
}
return "not found in $PATH: " + execName
}
6 changes: 5 additions & 1 deletion lib/srv/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,9 @@ func (t *proxySubsys) proxyToHost(
// if port is 0, it means the client wants us to figure out
// which port to use
useExactPort := len(t.port) > 0 && t.port != "0"
ips, _ := net.LookupHost(t.host)
log.Debugf("proxy connecting to host=%v port=%v, exact port=%v\n", t.host, t.port, useExactPort)

ips, err := net.LookupHost(t.host)

// enumerate and try to find a server with self-registered with a matching name/IP:
var server services.Server
Expand All @@ -257,6 +259,7 @@ func (t *proxySubsys) proxyToHost(
log.Error(err)
continue
}

if t.host == ip || t.host == servers[i].GetHostname() || utils.SliceContainsStr(ips, ip) {
server = servers[i]
// found the server. see if we need to match the port
Expand All @@ -275,6 +278,7 @@ func (t *proxySubsys) proxyToHost(
t.port = strconv.Itoa(defaults.SSHServerListenPort)
}
serverAddr = net.JoinHostPort(t.host, t.port)
log.Warnf("server lookup failed: using default=%v", serverAddr)
}

// we must dial by server IP address because hostname
Expand Down
10 changes: 7 additions & 3 deletions lib/srv/sshserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ package srv
import (
"fmt"
"io"
"io/ioutil"
"net"
"os"
"os/exec"
Expand All @@ -42,7 +43,6 @@ import (
"github.com/gravitational/teleport/lib/sshutils"
"github.com/gravitational/teleport/lib/teleagent"
"github.com/gravitational/teleport/lib/utils"
"github.com/pborman/uuid"

log "github.com/Sirupsen/logrus"
"github.com/gravitational/trace"
Expand Down Expand Up @@ -872,13 +872,17 @@ func (s *Server) handleAgentForward(ch ssh.Channel, req *ssh.Request, ctx *ctx)

authChan, _, err := ctx.conn.OpenChannel("auth-agent@openssh.com", nil)
if err != nil {
return err
return trace.Wrap(err)
}
clientAgent := agent.NewClient(authChan)
ctx.setAgent(clientAgent, authChan)

pid := os.Getpid()
socketPath := filepath.Join(os.TempDir(), fmt.Sprintf("teleport-agent-%v.socket", uuid.New()))
socketDir, err := ioutil.TempDir(os.TempDir(), "teleport-")
if err != nil {
return trace.Wrap(err)
}
socketPath := filepath.Join(socketDir, fmt.Sprintf("teleport-%v.socket", pid))

agentServer := &teleagent.AgentServer{Agent: clientAgent}
err = agentServer.ListenUnixSocket(socketPath, uid, gid, 0600)
Expand Down
4 changes: 3 additions & 1 deletion lib/srv/sshserver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"fmt"
"io"
"net"
"os"
"os/user"
"regexp"
"strings"
Expand Down Expand Up @@ -238,7 +239,8 @@ func (s *SrvSuite) TestAgentForward(c *C) {
_, err = io.WriteString(writer, fmt.Sprintf("printenv %v\n\r", teleport.SSHAuthSock))
c.Assert(err, IsNil)

re := regexp.MustCompile(`/tmp/[^\s]+`)
pattern := fmt.Sprintf(`%vteleport-[0-9]+[^\s]+`, os.TempDir())
re := regexp.MustCompile(pattern)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not actually the line that has the problem, but it's the loop below:

 	for i := 0; i < 3; i++ {
 		_, err = reader.Read(buf)
 		c.Assert(err, IsNil)
 		matches = re.FindStringSubmatch(string(buf))
 		if len(matches) != 0 {
 			break
 		}
 		time.Sleep(50 * time.Millisecond)
 	}

On Linux tests hang on the line _, err = reader.Read(buf). Works fine on macOS.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@klizhentas ^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me try to fix that

buf := make([]byte, 4096)
var matches []string
for i := 0; i < 3; i++ {
Expand Down
74 changes: 38 additions & 36 deletions lib/utils/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,49 @@ import (
"github.com/gravitational/trace"
)

// InitLoggerCLI tools by default log into syslog, not stderr
func InitLoggerCLI() {
log.SetLevel(log.WarnLevel)
// clear existing hooks:
type LoggingPurpose int

const (
LoggingForDaemon LoggingPurpose = iota
LoggingForCLI
LoggingForTests
)

// InitLogger configures the global logger for a given purpose / verbosity level
func InitLogger(purpose LoggingPurpose, level log.Level) {
log.StandardLogger().Hooks = make(log.LevelHooks)
log.SetFormatter(&trace.TextFormatter{})
formatter := &trace.TextFormatter{}
formatter.DisableTimestamp = true
log.SetFormatter(formatter)
log.SetLevel(level)

switch purpose {
case LoggingForCLI:
SwitchLoggingtoSyslog()
case LoggingForDaemon:
log.SetOutput(os.Stderr)
case LoggingForTests:
log.SetLevel(level)
val, _ := strconv.ParseBool(os.Getenv(teleport.VerboseLogsEnvVar))
if val {
return
}
log.SetLevel(log.WarnLevel)
log.SetOutput(ioutil.Discard)
}
}

func InitLoggerForTests() {
InitLogger(LoggingForTests, log.WarnLevel)
}

// SwitchLoggingtoSyslog tells the logger to send the output to syslog
func SwitchLoggingtoSyslog() {
log.StandardLogger().Hooks = make(log.LevelHooks)
hook, err := logrusSyslog.NewSyslogHook("", "", syslog.LOG_WARNING, "")
if err != nil {
// syslog not available
log.SetOutput(os.Stderr)
log.Warn("syslog not available. reverting to stderr")
} else {
// ... and disable stderr:
Expand All @@ -51,37 +84,6 @@ func InitLoggerCLI() {
}
}

// InitLoggerDebug configures the logger to dump everything to stderr
func InitLoggerDebug() {
InitDebugLogger(log.DebugLevel)
}

// InitLoggerVerbose is a less chatty version of debug logger above
func InitLoggerVerbose() {
InitDebugLogger(log.InfoLevel)
}

func InitDebugLogger(level log.Level) {
// clear existing hooks:
log.StandardLogger().Hooks = make(log.LevelHooks)
log.SetFormatter(&trace.TextFormatter{})
log.SetOutput(os.Stderr)
log.SetLevel(level)
}

// InitLoggerForTests inits logger to discard ouput in tests unless
// DEBUG is set to "1"
func InitLoggerForTests() {
val, _ := strconv.ParseBool(os.Getenv(teleport.VerboseLogsEnvVar))
if val {
InitLoggerDebug()
return
}
log.SetLevel(log.WarnLevel)
log.StandardLogger().Hooks = make(log.LevelHooks)
log.SetOutput(ioutil.Discard)
}

// FatalError is for CLI front-ends: it detects gravitational/trace debugging
// information, sends it to the logger, strips it off and prints a clean message to stderr
func FatalError(err error) {
Expand Down
4 changes: 2 additions & 2 deletions tool/tctl/common/tctl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ type DeleteCommand struct {
}

func Run() {
utils.InitLoggerCLI()
utils.InitLogger(utils.LoggingForCLI, logrus.WarnLevel)
app := utils.InitCLIParser("tctl", GlobalHelpString)

// generate default tctl configuration:
Expand Down Expand Up @@ -840,7 +840,7 @@ func applyConfig(ccf *CLIConfig, cfg *service.Config) error {
}
// --debug flag
if ccf.Debug {
utils.InitLoggerDebug()
utils.InitLogger(utils.LoggingForCLI, logrus.DebugLevel)
logrus.Debugf("DEBUG loggign enabled")
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion tool/teleport/common/teleport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func Run(cmdlineArgs []string, testRun bool) (executedCommand string, conf *serv
}
// configure logger for a typical CLI scenario until configuration file is
// parsed
utils.InitLoggerCLI()
utils.InitLogger(utils.LoggingForDaemon, log.WarnLevel)
app := utils.InitCLIParser("teleport", "Clustered SSH service. Learn more at http://teleport.gravitational.com")

// define global flags:
Expand Down
1 change: 0 additions & 1 deletion tool/tsh/common/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ func (s *MainTestSuite) TestMakeClient(c *check.C) {
tc, err = makeClient(&conf, true)
c.Assert(err, check.IsNil)
c.Assert(tc, check.NotNil)
c.Assert(tc.Config.NodeHostPort(), check.Equals, "localhost:0") // SSH port must not be set to default!
c.Assert(tc.Config.ProxySSHHostPort(), check.Equals, "proxy:3023")
c.Assert(tc.Config.ProxyWebHostPort(), check.Equals, "proxy:3080")
localUser, err := client.Username()
Expand Down
7 changes: 4 additions & 3 deletions tool/tsh/common/tsh.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import (
"github.com/gravitational/teleport/lib/utils"
"github.com/gravitational/trace"

"github.com/Sirupsen/logrus"
"github.com/buger/goterm"
)

Expand Down Expand Up @@ -87,7 +88,7 @@ func Run(args []string, underTest bool) {
cf CLIConf
)
cf.IsUnderTest = underTest
utils.InitLoggerCLI()
utils.InitLogger(utils.LoggingForCLI, logrus.WarnLevel)

// configure CLI argument parser:
app := utils.InitCLIParser("tsh", "TSH: Teleport SSH client").Interspersed(false)
Expand Down Expand Up @@ -148,7 +149,7 @@ func Run(args []string, underTest bool) {

// apply -d flag:
if *debugMode {
utils.InitLoggerDebug()
utils.InitLogger(utils.LoggingForCLI, logrus.DebugLevel)
}

switch command {
Expand Down Expand Up @@ -391,7 +392,7 @@ export SSH_AGENT_PID=%v
// makeClient takes the command-line configuration and constructs & returns
// a fully configured TeleportClient object
func makeClient(cf *CLIConf, useProfileLogin bool) (tc *client.TeleportClient, err error) {
// apply defults
// apply defaults
if cf.MinsToLive == 0 {
cf.MinsToLive = int32(defaults.CertDuration / time.Minute)
}
Expand Down