When tainting a route during setup, pre-calculate the namespace specific path #15067
Conversation
| @@ -835,6 +835,9 @@ func (c *Core) setupCredentials(ctx context.Context) error { | |||
|
|
|||
| // Ensure the path is tainted if set in the mount table | |||
| if entry.Tainted { | |||
| // Calculate any namespace prefixes here, because when Taint() is called, there won't be | |||
| // a namespace to pull from the context. This is similar to what we do above in c.router.Mount(). | |||
| path = entry.Namespace().Path + path | |||
| c.router.Taint(ctx, path) | |||
There was a problem hiding this comment.
When this is called as part of postUnseal(), as best as I can tell, we're passing an empty context all the way through these methods, originating here: https://github.com/hashicorp/vault/blob/main/vault/core.go#L1693 That means the namespace specific path that we need to add to the path won't exist in the context, as Taint() expects it to. Instead, we use the namespace specific path from the mount entry.
| @@ -47,6 +47,8 @@ func NewRouter() *Router { | |||
| storagePrefix: radix.New(), | |||
| mountUUIDCache: radix.New(), | |||
| mountAccessorCache: radix.New(), | |||
| // this will get replaced in production with a real logger but it's useful to have a default in place for tests | |||
| logger: hclog.NewNullLogger(), | |||
There was a problem hiding this comment.
The router has a logger, but before this PR, it didn't get assigned as part of NewRouter(). That made lots of tests that exercise the router fail with nil pointer exceptions, due to the new logging lines. The actual logger for the router gets assigned here, as part of CreateCore() but it felt like a good idea to have a default assigned to the router from the get-go. Does this seem acceptable as is, or should this default be a more legit logger?
vault/router.go
Outdated
| } | ||
| r.l.RUnlock() | ||
| if !ok { | ||
| r.logger.Trace("route entry not found", "path", req.Path) |
There was a problem hiding this comment.
This could also result in a very high log volume. Can you use a metric instead?
There was a problem hiding this comment.
I ended up removing this in favor of augmented error messages. I could add additional metrics as well, but after thinking this over a bit, I wasn't sure what kind of metric to add. Are you thinking of a counter each time this failure happens? Or something else?
No description provided.