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

Simplify a lot of the mount tuning code #3285

Merged
merged 7 commits into from
Sep 5, 2017
Merged

Simplify a lot of the mount tuning code #3285

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9a60767
Simplify a lot of the mount tuning code.
jefferai Sep 4, 2017
f4fd0f8
Merge branch 'master' into simplify-mount-tuning
jefferai Sep 4, 2017
8bc1bcc
Merge branch 'master' into simplify-mount-tuning
jefferai Sep 4, 2017
313ac65
Merge branch 'master' into simplify-mount-tuning
jefferai Sep 5, 2017
bd930cb
Merge remote-tracking branch 'oss/master' into simplify-mount-tuning
jefferai Sep 5, 2017
1da3048
Address review feedback
jefferai Sep 5, 2017
21aa35a
Fix test
jefferai Sep 5, 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
66 changes: 55 additions & 11 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ func NewSystemBackend(core *Core) *SystemBackend {
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["tune_max_lease_ttl"][0]),
},
"description": &framework.FieldSchema{
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["auth_desc"][0]),
},
},
Callbacks: map[logical.Operation]framework.OperationFunc{
logical.ReadOperation: b.handleAuthTuneRead,
Expand All @@ -255,6 +259,10 @@ func NewSystemBackend(core *Core) *SystemBackend {
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["tune_max_lease_ttl"][0]),
},
"description": &framework.FieldSchema{
Type: framework.TypeString,
Description: strings.TrimSpace(sysHelp["auth_desc"][0]),
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
Expand Down Expand Up @@ -1544,40 +1552,52 @@ func (b *SystemBackend) handleTuneWriteCommon(
lock = &b.Core.mountsLock
}

lock.Lock()
defer lock.Unlock()

// Check again after grabbing the lock
mountEntry = b.Core.router.MatchingMountEntry(path)
if mountEntry == nil {
b.Backend.Logger().Error("sys: tune failed: no mount entry found", "path", path)
return handleError(fmt.Errorf("sys: tune of path '%s' failed: no mount entry found", path))
}
if mountEntry != nil && !mountEntry.Local && repState == consts.ReplicationSecondary {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the HasState function:

repState.HasState(consts.ReplicationPerformanceSecondary)

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 a copy and paste from above, which means it got missed before too :-/

Copy link
Contributor

@briankassouf briankassouf Sep 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It was changed in my locking PR, so i think it has since been updated.

return logical.ErrorResponse("cannot tune a non-local mount on a replication secondary"), nil
}

// Timing configuration parameters
{
var newDefault, newMax *time.Duration
var newDefault, newMax time.Duration
defTTL := data.Get("default_lease_ttl").(string)
switch defTTL {
case "":
newDefault = mountEntry.Config.DefaultLeaseTTL
case "system":
tmpDef := time.Duration(0)
newDefault = &tmpDef
newDefault = time.Duration(0)
default:
tmpDef, err := parseutil.ParseDurationSecond(defTTL)
if err != nil {
return handleError(err)
}
newDefault = &tmpDef
newDefault = tmpDef
}

maxTTL := data.Get("max_lease_ttl").(string)
switch maxTTL {
case "":
newMax = mountEntry.Config.MaxLeaseTTL
case "system":
tmpMax := time.Duration(0)
newMax = &tmpMax
newMax = time.Duration(0)
default:
tmpMax, err := parseutil.ParseDurationSecond(maxTTL)
if err != nil {
return handleError(err)
}
newMax = &tmpMax
newMax = tmpMax
}

if newDefault != nil || newMax != nil {
lock.Lock()
defer lock.Unlock()
if newDefault != mountEntry.Config.DefaultLeaseTTL ||
newMax != mountEntry.Config.MaxLeaseTTL {

if err := b.tuneMountTTLs(path, mountEntry, newDefault, newMax); err != nil {
b.Backend.Logger().Error("sys: tuning failed", "path", path, "error", err)
Expand All @@ -1586,7 +1606,31 @@ func (b *SystemBackend) handleTuneWriteCommon(
}
}

return nil, nil
var resp *logical.Response
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be used anywhere, should we just return nil, nil below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it was used for a warning before but that code is now removed, will fix it.


description := data.Get("description").(string)
if description != "" {
oldDesc := mountEntry.Description
mountEntry.Description = description

// Update the mount table
var err error
switch {
case strings.HasPrefix(path, "auth/"):
err = b.Core.persistAuth(b.Core.auth, mountEntry.Local)
default:
err = b.Core.persistMounts(b.Core.mounts, mountEntry.Local)
}
if err != nil {
mountEntry.Description = oldDesc
return handleError(err)
}
if b.Core.logger.IsInfo() {
b.Core.logger.Info("core: mount tuning of description successful", "path", path)
}
}

return resp, nil
}

// handleLease is use to view the metadata for a given LeaseID
Expand Down
71 changes: 20 additions & 51 deletions vault/logical_system_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,61 +7,31 @@ import (
)

// tuneMount is used to set config on a mount point
func (b *SystemBackend) tuneMountTTLs(path string, me *MountEntry, newDefault, newMax *time.Duration) error {
meConfig := &me.Config
func (b *SystemBackend) tuneMountTTLs(path string, me *MountEntry, newDefault, newMax time.Duration) error {
zero := time.Duration(0)

if newDefault == nil && newMax == nil {
return nil
}
if newDefault == nil && newMax != nil &&
*newMax == meConfig.MaxLeaseTTL {
return nil
}
if newMax == nil && newDefault != nil &&
*newDefault == meConfig.DefaultLeaseTTL {
return nil
}
if newMax != nil && newDefault != nil &&
*newDefault == meConfig.DefaultLeaseTTL &&
*newMax == meConfig.MaxLeaseTTL {
return nil
}
switch {
case newDefault == zero && newMax == zero:
// No checks needed

if newMax != nil && newDefault != nil && *newMax < *newDefault {
return fmt.Errorf("new backend max lease TTL of %d less than new backend default lease TTL of %d",
int(newMax.Seconds()), int(newDefault.Seconds()))
}
case newDefault == zero && newMax != zero:
// No default/max conflict, no checks needed

if newMax != nil && newDefault == nil {
if meConfig.DefaultLeaseTTL != 0 && *newMax < meConfig.DefaultLeaseTTL {
return fmt.Errorf("new backend max lease TTL of %d less than backend default lease TTL of %d",
int(newMax.Seconds()), int(meConfig.DefaultLeaseTTL.Seconds()))
}
}
case newDefault != zero && newMax == zero:
// No default/max conflict, no checks needed

if newDefault != nil {
if meConfig.MaxLeaseTTL == 0 {
if newMax == nil && *newDefault > b.Core.maxLeaseTTL {
return fmt.Errorf("new backend default lease TTL of %d greater than system max lease TTL of %d",
int(newDefault.Seconds()), int(b.Core.maxLeaseTTL.Seconds()))
}
} else {
if newMax == nil && *newDefault > meConfig.MaxLeaseTTL {
return fmt.Errorf("new backend default lease TTL of %d greater than backend max lease TTL of %d",
int(newDefault.Seconds()), int(meConfig.MaxLeaseTTL.Seconds()))
}
case newDefault != zero && newMax != zero:
Copy link
Contributor

Choose a reason for hiding this comment

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

This switch statement might be cleaner if it only had this case and a default case. Unless you like the verbosity of the other statements + comments.

Copy link
Member Author

Choose a reason for hiding this comment

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

In this instance I found it useful to think through the different cases and leave appropriate comments.

if newMax < newDefault {
return fmt.Errorf("backend max lease TTL of %d would be less than backend default lease TTL of %d",
int(newMax.Seconds()), int(newDefault.Seconds()))
}
}

origMax := meConfig.MaxLeaseTTL
origDefault := meConfig.DefaultLeaseTTL
origMax := me.Config.MaxLeaseTTL
origDefault := me.Config.DefaultLeaseTTL

if newMax != nil {
meConfig.MaxLeaseTTL = *newMax
}
if newDefault != nil {
meConfig.DefaultLeaseTTL = *newDefault
}
me.Config.MaxLeaseTTL = newMax
me.Config.DefaultLeaseTTL = newDefault

// Update the mount table
var err error
Expand All @@ -72,13 +42,12 @@ func (b *SystemBackend) tuneMountTTLs(path string, me *MountEntry, newDefault, n
err = b.Core.persistMounts(b.Core.mounts, me.Local)
}
if err != nil {
meConfig.MaxLeaseTTL = origMax
meConfig.DefaultLeaseTTL = origDefault
me.Config.MaxLeaseTTL = origMax
me.Config.DefaultLeaseTTL = origDefault
return fmt.Errorf("failed to update mount table, rolling back TTL changes")
}

if b.Core.logger.IsInfo() {
b.Core.logger.Info("core: mount tuning successful", "path", path)
b.Core.logger.Info("core: mount tuning of leases successful", "path", path)
}

return nil
Expand Down