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.

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

Move location of quit channel closing in exp manager #3638

Merged
merged 3 commits into from
Dec 1, 2017
Merged
Changes from 1 commit
Commits
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
60 changes: 42 additions & 18 deletions vault/expiration.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,13 @@ const (
// If a secret is not renewed in timely manner, it may be expired, and
// the ExpirationManager will handle doing automatic revocation.
type ExpirationManager struct {
router *Router
idView *BarrierView
tokenView *BarrierView
tokenStore *TokenStore
logger log.Logger
core *Core
router *Router
idView *BarrierView
tokenView *BarrierView
tokenStore *TokenStore
logger log.Logger
coreStateLock *sync.RWMutex
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are storing core, we don't need to also store the state lock

Copy link
Member Author

Choose a reason for hiding this comment

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

It's true, the main reason I kept it like this is I would really rather not plumb through core, I just also figured we should check standby and sealed while we were at it and didn't feel like storing two pointers to bools. So I left it the easiest way to plumb back out.


pending map[string]*time.Timer
pendingLock sync.RWMutex
Expand All @@ -72,25 +74,28 @@ type ExpirationManager struct {

// NewExpirationManager creates a new ExpirationManager that is backed
// using a given view, and uses the provided router for revocation.
func NewExpirationManager(router *Router, view *BarrierView, ts *TokenStore, logger log.Logger) *ExpirationManager {
if logger == nil {
logger = log.New("expiration_manager")
}

func NewExpirationManager(c *Core, view *BarrierView) *ExpirationManager {
exp := &ExpirationManager{
router: router,
idView: view.SubView(leaseViewPrefix),
tokenView: view.SubView(tokenViewPrefix),
tokenStore: ts,
logger: logger,
pending: make(map[string]*time.Timer),
core: c,
router: c.router,
idView: view.SubView(leaseViewPrefix),
tokenView: view.SubView(tokenViewPrefix),
tokenStore: c.tokenStore,
logger: c.logger,
pending: make(map[string]*time.Timer),
coreStateLock: &c.stateLock,

// new instances of the expiration manager will go immediately into
// restore mode
restoreMode: 1,
restoreLocks: locksutil.CreateLocks(),
quitCh: make(chan struct{}),
}

if exp.logger == nil {
exp.logger = log.New("expiration_manager")
}

return exp
}

Expand All @@ -103,7 +108,7 @@ func (c *Core) setupExpiration() error {
view := c.systemBarrierView.SubView(expirationSubPath)

// Create the manager
mgr := NewExpirationManager(c.router, view, c.tokenStore, c.logger)
mgr := NewExpirationManager(c, view)
c.expiration = mgr

// Link the token store to this
Expand Down Expand Up @@ -430,14 +435,17 @@ func (m *ExpirationManager) Stop() error {
m.logger.Debug("expiration: stop triggered")
defer m.logger.Debug("expiration: finished stopping")

// Do this before stopping pending timers to avoid potential races with
// expiring timers
close(m.quitCh)

m.pendingLock.Lock()
for _, timer := range m.pending {
timer.Stop()
}
m.pending = make(map[string]*time.Timer)
m.pendingLock.Unlock()

close(m.quitCh)
if m.inRestoreMode() {
for {
if !m.inRestoreMode() {
Expand Down Expand Up @@ -969,13 +977,29 @@ func (m *ExpirationManager) expireID(leaseID string) {
return
default:
}

m.coreStateLock.RLock()
if m.core.sealed {
m.logger.Error("expiration: sealed, not attempting further revocation of lease", "lease_id", leaseID)
m.coreStateLock.RUnlock()
return
}
if m.core.standby {
m.logger.Error("expiration: standby, not attempting further revocation of lease", "lease_id", leaseID)
m.coreStateLock.RUnlock()
return
}

err := m.Revoke(leaseID)
if err == nil {
if m.logger.IsInfo() {
m.logger.Info("expiration: revoked lease", "lease_id", leaseID)
}
m.coreStateLock.RUnlock()
return
}

m.coreStateLock.RUnlock()
m.logger.Error("expiration: failed to revoke lease", "lease_id", leaseID, "error", err)
time.Sleep((1 << attempt) * revokeRetryBase)
}
Expand Down