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

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Fix to make mender-connect request a new JWT token on its own, so its not dependent on mender-update to do so #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 21 additions & 27 deletions app/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,57 +312,51 @@ func (d *MenderShellDaemon) dbusEventLoop(client mender.AuthClient) {
break
}

if d.needsReconnect() {
Copy link
Author

Choose a reason for hiding this comment

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

In this newest commit I removed this extra check "if d.needsReconnect" early in the function which set the needsReconnect value to True. Rather the code now only have one check. Having these two checks doing the same thing must have delayed the tests and made it fail.

log.Trace("dbusEventLoop: daemon needs to reconnect")
needsReconnect = true
}

p, err := client.WaitForJwtTokenStateChange()
log.Tracef("dbusEventLoop: WaitForJwtTokenStateChange %v, err %v", p, err)

if len(p) > 1 &&
p[0].ParamType == dbus.GDBusTypeString &&
p[1].ParamType == dbus.GDBusTypeString {

token := p[0].ParamData.(string)
serverURL := p[1].ParamData.(string)

d.processJwtTokenStateChange(token, serverURL)
if len(token) > 0 && len(serverURL) > 0 {
log.Tracef("dbusEventLoop: got a token len=%d, ServerURL=%s", len(token), serverURL)
if token != d.serverJwt || serverURL != d.serverUrl {
log.Debugf(
"dbusEventLoop: new token or ServerURL, reconnecting. len=%d, ServerURL=%s",
len(token),
serverURL,
)
needsReconnect = true
e := MenderShellDaemonEvent{
event: EventReconnect,
data: token,
id: "(dbusEventLoop)",
}
d.serverJwt = token
d.serverUrl = serverURL
d.postEvent(e)
log.Tracef("(dbusEventLoop) posting Event: %s", e.event)
Copy link
Contributor

Choose a reason for hiding this comment

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

Either we move this entry to before the d.postEvent(e) line or we change the text for something like "posted Event: ..."

needsReconnect = false

// If the server (Mender client proxy) closed the connection, it is likely that
// both messageLoop is asking for a reconnection and we got JwtTokenStateChange
// signal. So drain here the reconnect channel and reconnect only once
if d.needsReconnect() {
log.Debug("dbusEventLoop: drained reconnect req channel")
log.Trace("dbusEventLoop: daemon needs to reconnect")
Copy link
Author

Choose a reason for hiding this comment

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

Reference point for message above. Only now one check for setting needsReconnect to True

Copy link
Contributor

Choose a reason for hiding this comment

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

Okey. But I think now the comment above with regards to "draining" the channel is obsolete, as with the current code this is the only point where the message loop can signal a re-connection.

I think the code is good but we need to adjust the miss-leading comment (or remove?)

needsReconnect = true
}

}
// TODO: moving these assignments one scope up would make d.authorized redundant...
d.serverJwt = token
d.serverUrl = serverURL
}
}
if needsReconnect && d.authorized {
jwtToken, serverURL, _ := client.GetJWTToken()
e := MenderShellDaemonEvent{
event: EventReconnect,
data: jwtToken,
id: "(dbusEventLoop)",

if needsReconnect || d.needsReconnect() {
log.Trace("dbusEventLoop: daemon needs to reconnect")
_, err := client.FetchJWTToken()
if err != nil {
log.Errorf("dbusEventLoop: error fetching JWT token")
}
log.Tracef("(dbusEventLoop) posting Event: %s", e.event)
d.serverUrl = serverURL
d.serverJwt = jwtToken
d.postEvent(e)
needsReconnect = false
}
}

log.Trace("dbusEventLoop: returning")
}

Expand Down
135 changes: 77 additions & 58 deletions app/daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,94 +739,113 @@ func TestTimeToSweepSessions(t *testing.T) {
func TestDBusEventLoop(t *testing.T) {
currentUser, err := user.Current()
if err != nil {
t.Errorf("cant get current user: %s", err.Error())
return
t.Fatalf("cant get current user: %s", err)
lluiscampos marked this conversation as resolved.
Show resolved Hide resolved
}

testCases := []struct {
name string
err error
token string
err error
timeout time.Duration
}{
{
name: "stopped-gracefully",
token: "the-token",
name: "stopped-gracefully",
token: "the-token",
timeout: 4 * time.Second,
},
{
name: "token_not_returned_wait_forever",
token: "",
timeout: 15 * time.Second,
},
{
name: "token_not_returned_try_reconnection",
token: "",
timeout: 2 * time.Second,
},
}

for _, tc := range testCases {
if tc.name == "token_not_returned_wait_forever" {
timeout := time.After(tc.timeout)
done := make(chan bool)
go func() {
t.Run(tc.name, func(t *testing.T) {
d := NewDaemon(&config.MenderShellConfig{
MenderShellConfigFromFile: config.MenderShellConfigFromFile{
ShellCommand: "/bin/sh",
User: currentUser.Name,
Terminal: config.TerminalConfig{
Width: 24,
Height: 80,
},
},
})
d := NewDaemon(&config.MenderShellConfig{
MenderShellConfigFromFile: config.MenderShellConfigFromFile{
ShellCommand: "/bin/sh",
User: currentUser.Name,
Terminal: config.TerminalConfig{
Width: 24,
Height: 80,
},
},
})

dbusAPI := &dbusmocks.DBusAPI{}
defer dbusAPI.AssertExpectations(t)
client := &authmocks.AuthClient{}
client.On("WaitForJwtTokenStateChange").Return([]dbus.SignalParams{
{
ParamType: "s",
ParamData: tc.token,
},
}, tc.err)
client.On("GetJWTToken").Return(tc.token, "", tc.err)
d.dbusEventLoop(client)
})
done <- true
}()
dbusAPI := &dbusmocks.DBusAPI{}
defer dbusAPI.AssertExpectations(t)
client := &authmocks.AuthClient{}

select {
case <-timeout:
t.Logf("ok: expected to run forever")
case <-done:
}
} else {
t.Run(tc.name, func(t *testing.T) {
d := NewDaemon(&config.MenderShellConfig{
MenderShellConfigFromFile: config.MenderShellConfigFromFile{
ShellCommand: "/bin/sh",
User: currentUser.Name,
Terminal: config.TerminalConfig{
Width: 24,
Height: 80,
},
t.Run(tc.name, func(t *testing.T) {
switch tc.name {
case "token_not_returned_wait_forever":
timeout := time.After(tc.timeout)
done := make(chan bool)
client.On("WaitForJwtTokenStateChange").Return([]dbus.SignalParams{
{
ParamType: "",
ParamData: nil,
},
})
}, tc.err)
go func() {
d.dbusEventLoop(client)
done <- true
}()
select {
case <-timeout:
t.Logf("ok: expected to run forever")
case <-done:
}

dbusAPI := &dbusmocks.DBusAPI{}
defer dbusAPI.AssertExpectations(t)
client := &authmocks.AuthClient{}
case "stopped-gracefully":
timeout := time.After(tc.timeout)
done := make(chan bool)
client.On("WaitForJwtTokenStateChange").Return([]dbus.SignalParams{
{
ParamType: "s",
ParamData: tc.token,
},
}, tc.err)
client.On("GetJWTToken").Return(tc.token, "", tc.err)
go func() {
time.Sleep(time.Second)
d.dbusEventLoop(client)
done <- true
}()

select {
case <-timeout:
client.AssertNotCalled(t, "FetchJWTToken")
d.stop = true
<-done
case <-done:
t.Error("dbusEventLoop returned unexpectedly early")
}

case "token_not_returned_try_reconnection":
timeout := time.After(tc.timeout)
done := make(chan bool)
client.On("WaitForJwtTokenStateChange").Return([]dbus.SignalParams{
{
ParamType: "",
ParamData: nil,
},
}, tc.err)
go func() {
d.dbusEventLoop(client)
client.AssertCalled(t, "FetchJWTToken")
done <- true
}()
d.dbusEventLoop(client)
})
}
select {
case <-timeout:
t.Log("ok: expected to run forever")
case <-done:
}
}
})
}
}

Expand Down