1
\$\begingroup\$

This package is a part of a web application which is an internal company tool. This web application may need access to a number of secretes stored in hashicorp vault. The secrets rarely change, although in case they occasionally do there is a UI button to invalidate the secrets cache so it could be reloaded. I felt that sync.Map is an overkill here, so I implemented a very simple cache behind sync.RWMutex: in all likelihood, each particular Vault path will ever only need to be loaded once, and it is unlikely that more than a few dozen paths will need to be read in total. On the other hand the application will be requesting secrets as part of it's normal work all the time, it will just be the same secrets over and over.

There are three files here. One is the cache, another to deal with Vault token refresh (the web app authenticates against Vault with an approle). And the remaining file implements the function the web application call to get the secrets.

The application is similar to assets management, it is used by less than half a dozen people responsible for it, so if there are any concurrency issues, I'm not likely to notice, yet I'd like not to make obvious mistakes.

I'm an experienced programmer, but I'm fairly inexperienced in Go, so while the focus of my question is thread safety and the cache implementation, I welcome any and all feedback, including one on Go best pattern and practices and styling.

cache.go:

package vault

import (
    "sync"
)

var cache = make(map[string]map[string]interface{})
var cacheLock sync.RWMutex

func getCached(key string) (map[string]interface{}, bool) {
    cacheLock.RLock()
    defer cacheLock.RUnlock()
    res, ok := cache[key]
    return res, ok
}

func writeCache(key string, value map[string]interface{}) {
    cacheLock.Lock()
    defer cacheLock.Unlock()
    cache[key] = value
}

// May ocasionally be requested from web UI
func ResetCache() {
    cacheLock.Lock()
    defer cacheLock.Unlock()
    cache = make(map[string]map[string]interface{})
}

token.go:

package vault

import (
    "fmt"
    "sync"

    vault "github.com/hashicorp/vault/api"
)

var token string
var tokenLock sync.RWMutex

func getToken() string {
    tokenLock.RLock()
    defer tokenLock.RUnlock()
    return token
}

func refreshToken(c *vault.Logical) (string, error) {
    tokenLock.Lock()
    defer tokenLock.Unlock()

    secret, err := c.Write("auth/approle/login", map[string]interface{}{
        "role_id":   config.RoleId,
        "secret_id": config.SecretId,
    })

    if err != nil {
        return "", fmt.Errorf("error logging into vault approle: %v", err)
    }

    token = secret.Auth.ClientToken
    return token, nil
}

client.go

package vault

import (
    "fmt"

    vault "github.com/hashicorp/vault/api"
)

type VaultConfig struct {
    Url      string
    RoleId   string
    SecretId string
}

var config VaultConfig

// This is called on the application start up
func Init(url, roleId, secretId string) {
    config = VaultConfig{
        Url:      url,
        RoleId:   roleId,
        SecretId: secretId,
    }
}

// This is the main package function. Give a path in vault and a key name under the path get the value
func Get(path, key string) (string, error) {
    data, ok := getCached(path)
    if !ok {
        var err error
        data, err = readFromVault(path)
        if err != nil {
            return "", err
        }
        writeCache(path, data)
    }
    if result, ok := data[key]; ok {
        r, ok := result.(string)
        if !ok {
            // I beleive this can never happen (unless vault api changes)
            panic("vault value is not a string")
        }
        return r, nil
    } else {
        return "", fmt.Errorf("failed to find key '%v' at '%v' in vault", key, path)
    }
}

func readFromVault(path string) (map[string]interface{}, error) {
    client, err := vault.NewClient(&vault.Config{
        Address: config.Url,
    })
    if err != nil {
        return nil, fmt.Errorf("error creating vault client for %s: %v", config.Url, err)
    }
    c := client.Logical()
    client.SetToken(getToken())
    l, err := c.Read(path)
    if err != nil {
        if re, ok := err.(*vault.ResponseError); !ok || re.StatusCode != 403 {
            return nil, fmt.Errorf("failed to read path '%v' in vault: %v", path, err)
        }
        // If that was 403 it could be because of the invalid/expired token
        token, err := refreshToken(c)
        if err != nil {
            return nil, err
        }
        client.SetToken(token)
        l, err = c.Read(path)
        if err != nil {
            return nil, fmt.Errorf("failed to read path '%v' in vault: %v", path, err)
        }
    }
    result, ok := l.Data["data"].(map[string]interface{})
    if !ok {
        // I beleive this can never happen (unless vault api changes)
        panic(fmt.Sprintf("vault data dictionary is of unexpected type '%T'", l.Data["data"]))
    }
    return result, nil
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

First up: whenever you're wondering if your code is safe for concurrent use, your first port of call really ought to be writing unit tests, marking them with t.Parallel() so all your tests can run in parallel (duh), and perhaps write a single test function that spawns a couple of routines that read and write to your cache. Run those tests with race detection enabled:

$ go test -race ./your/package

The race detector does a very good job at finding potential data races in your code. Anyway, let's look at your code:


The first thing that looks odd to me is that your vault package relies on 2 global variables, that are otherwise not linked. You seem to be storing some arbitrary set of cached values under a key, all stored as a map[string]interface{}. Your functions aren't exported (except for the ResetCache one), which means all code in the vault package should use the mutex to interact with this cached variable. Assuming you're doing that, the vault package should be fine WRT concurrency, but it's easy to imagine that someone forgets acquiring a lock, or messing other things up (deleting a line accidentally), and just like that, the entire package is rendered unsafe. When it comes to concurrency, the go standard library offers a type sync.Map, which you could use instead, but this is essentially a wrapper around a map[string]interface{}, rather than your map[string]map[string]interface{}. I would instead opt for a struct to keep the mutex and map together. This still requires anyone writing code in the package to use the mutex, but because it's a dedicated type, the code is more self-documenting, and essentially tells you that the mutex and map belong together:

package vault

type cache struct {
    data map[string]map[string]any // any is short for interface{}
    rmu  *sync.RWMutex
}

Note that the RWMutex is a pointer here. That's because adding methods can be done with a pointer receiver, or a value receiver. If the mutex is not a pointer, then value receivers will be using a copy of the mutex, rendering the interface unsafe once again:

func NewCache() *cache {
    return &cache{
        data: map[string]map[string]any{}, // no need for make
        rmu:  &sync.RWMutex{},
    }
}

func (c *cache) Reset() {
    c.rmu.Lock()
    defer c.rmu.Unlock()
    c.data = map[string]map[string]any{}
}

func (c cache) Get(k string) (map[string]any, bool) {
    c.rmu.RLock()
    defer c.rmu.RUnlock()
    return c.data[k]
}

Now imagine rmu is not a pointer. The Get call will receive a copy of the mutex, and a copy of the map, but a map is a reference type, so effecitvely it is shared should the Get function and Reset be called concurrently. Just like that, you'd have a data race, hence: the mutex has to be a pointer: a copy to a memory address after all points to the same object in memory.

Another thing to consider is that a package such as this can prove useful in a lot of different environments. Sometimes, you'd want to cache data that otherwise require a query to some type of database. You could add a cache object such as this where you store all objects using their ID. In that case, the restriction that you have of requiring data to be stored under a key as a map[string]any is very much an obstacle to its utility. Perhaps this is a decent use-case for a type that uses generics:

type cache[K comparable, V any] struct {
    data map[K]V
    rmu  *sync.Mutex
}

func New[K comparable, V any]() *cache[K, V] {
    return &cache{
        data: map[K]V{},
        rmu:  &sync.RWMutex{},
    }
}

If you now need a cache that stores map[string]any values using string keys, you'd create the instance like so:

mapCache := New[string, map[string]any]()

To cache DB objects that use a uint64 as ID (like a SERIAL INT or something), you can do that just as easily:

userCache := New[uint64, *entities.User]()

That does require your methods to be changed accordingly, but that's all very easy to do:

func (c *cache[K, V]) Get(key K) (V, bool) {
    c.rmu.RLock()
    defer c.rmu.RUnlock()
    return c.data[key]
}

With this implementation, using our 2 instances from earlier we can say that:

data, ok := mapCache("foo") // returns map[string]any, bool
user, ok := userCache(123)  // return *entities.User, bool

Overall, this makes the cache type a lot more flexible, and allows it to be used with any type of key, and any type of data. If the flexibility in terms of data you want to cache is not that important to you, I still maintain that supporting different key types makes a lot of sense. Clearly, you're wanting to have K-V store that is grouped at the top level. Say you're wanting to use this cache object in your entire project, but you want each package to make it easy to access the data that is relevant to it. You would then create a namespace type in a shared package:

package namespace

type Domain uint32

const (
    Database Domain = iota
    Requests
    Authenticaion
    // etc...
)

This is essentially how you'd create an enum in golang. We create a Domain type (which is ~uint32), and declare a bunch of iota constants of this type (this means the compiler will ensure all of the values will have distinct values).

Now our cache constructor would look like this:

type cache[K comparable] struct {
    data map[K]map[string]any
    rmu  *sync.RWMutex
}

func New[K comparable]() *cache[K] {
    return &cache{
        data: map[K]map[string]any{},
        rmu:  &sync.RWMutex{},
    }
}

you'd then create the cache instance like this:

cache := New[namespace.Domain]()

Each package would then be able to get the cache values that are relevant to it like this:

vals, ok := cache.Get(namespace.Database)

Another thing that doesn't sit well with me is in fact your ResetCache implementation. Let's say the data in the cache looks something like this:

{
    "foo": {"bar": 123, "foobar": true},
    "volatile": {"TTL": 123, "abc": "some value"},
}

Your cache implementation either treats all values as valid, or none. I'd argue that, if you're caching data in a map[string]map[string]any, you really would want to allow specific keys in the cache to be cleared/reset without it impacting the rest of the cache. Again: looking at the example of using this type of caching for querying data: you'd want to mark the cache as stale when you've executed an update on some or all of the records, but if you've updated a product catalog, you naturally want to mark the entities.Product cache as stale, but the entities.User data is still valid. I'd therefore argue that you probably want at least 2 methods to invalidate the cache:

func (c *cache[K, V]) ResetAll() {
    c.rmu.Lock()
    defer c.rmu.Unlock()
    c.data = map[K]V{}
}

// ResetKeys reset one or more cache entries by key
func (c *cache[K, V]) ResetKeys(keys ...K) {
    c.rmu.Lock()
    defer c.rmu.Unlock()
    for _, k := range keys {
        delete(c.data, k)
    }
}

Now the writeCache function looks fine to me, although if you want your cache implementation to be more broadly usable, having a CAS option would be nice:

func (c *cache[K, V]) CAS(key K, val V) bool {
    c.rmu.Lock()
    defer c.rmu.Unlock()
    if _, ok := c.data[key]; ok {
        return false // can't set the value, it's already set
    }
    c.data[key] = val
    return true // value was added to the cache
}

Now for the token file: broadly speaking, you could just create a type with a string field + a mutex, but a string is a simple value, but it's worth considering storing the token in a atomic.Value instead. If overall you're spending most of the time reading the value, it might be more efficient than having the overhead of dealing with an RWMutex. You can find more on the atomic package here

In your client.go file, the main difference will be that this:

client.SetToken(getToken())

will instead look more like this:

// assume the token is stored like this:
var tokenV atomic.Value
tokenV.Store(tokenStr)

token := tokenV.Load().(string) // get the token value
client.SetToken(token)

When refreshing the token, I'd probably pass in the expired token as an argument, so you can (atomically) check if the token has since been refreshed in another routine, but the key thing here would be to not just call Swap, but instead use the old value, get the new one and perform a CompareAndSwap:

func refreshToken(old string) (string, bool) {
    // get the new token
    ok := tokenV.CompareAndSwap(old, newToken)
    ret := tokenV.Load().(string)
    return ret, ok // returns the new token and true if you refershed the token, false if the token had already been refreshed
}

Depending on how you'll be using the token (I'm assuming the vast majority of access to the data will be simply reading the value), I suspect using an atomic value will outperform the mutex stuff, and there's only 1 variable to contend with. The overall interface that you'll want to expose will look something like this:

package token

var value atomic.Value

func init() {
    value.Set("")
}

func Get() string {
    s := value.Load().(string) // optionally use the ok flag to see if the type assertion worked, but this is a small package, and value is not exported, so should be fine
    return s
}

func Refresh(c *vault.Logical, old string) (string, bool) {
    // same as before, perform the request
    ok := value.CompareAndSwap(old, newToken)
    s := value.Load().(string)
    return s, ok
}

You may have noticed that I've renamed the functions from ResetCache to Reset[All|Keys], GetCached to Get, and similarly I renamed GetToken and RefreshToken to Get and Refresh. The reason for this is to avoid something referred to as stutter. As I explained above: the cache component is a "general purpose" package in essence. Its job it to take values, make them available on request as a simple map lookup. This package doesn't need to know about the token being a thing. It doesn't need to have access to the package, thus it shouldn't be imported. The token package is its own thing. It just exists to make a token string accessible in a concurrent-safe way, and it exports the interface that allows packages that import it to do just that. Therefore, I would expect the token.go and cache.go files to live in their own packages, which appropriately would be named token and cache respectively. If you had a function called RefreshToken in a package called token, your code could end up looking something like this:

currentToken := token.GetToken()
client.SetToken(currentToken)
// if you need to refresh the token:
newToken, ok := token.RefreshToken(currentToken)
// etc...

As you can see token.GetToken() is not only more to type, it doesn't communicate more than the more readable/brief: token.Get(). The same applies to this cache package:

valCache := cache.New[string, map[string]any]()
data, ok := valCache.Get("foo")

Is just as communicative as this:

valCache := cache.NewCache[string, map[string]any]()
data, ok := valCache.GetCached("foo")

It's quite important to not loose track of the way your packages will be used. We write packages to encapsulate or abstract away the intricacies that we don't want to deal with when writing logic. Should the user care that, when refreshing the token, they need to acquire a lock on said token, or that they should acquire a read lock on the token when getting that string? Of course not. In a package, we wrap that stuff in functions, which we can test, and thus ensure that the locks are correctly acquired and released. The code that uses our package only cares about Get() and Refresh(), nothing else.

Anything that makes a package feel clunky or counter-intuitive quickly (and I do mean surprisingly quickly) an obstacle, and eventually an excuse to circumnavigate your carefully crafted package. It's very common in even a medium-sized codebase to find a number of functions strewn about the place that look like this:

func max(a, b uint64) uint64 {
    if a > b {
       return a
    }
    return b
}

Then somewhere else you'll find something like this:

type Int interface {
    ~int | ~uint | ~int8 | ~uint8 | ~int32 | ~uint32 | ~int64 | ~uint64
}

func Max[T Int](a, b T) T {
    if a > b {
        return a
    }
    return b
}

only to stumble across some kind of utils or types package in that project that contains something like:

type Numeric interface {
    constraints.Integer | constraints.Float
}

func NumMax[T Numeric](a, b T) T {
    if a > b {
        return a
    }
    return b
}

func NumMin[T Numeric](a, b T) T {
    if a < b {
        return a
    }
    return b
}

Clearly, the last 2 functions can replace the first 2, so why did someone add their own max function? It's possible that it was just a convenience thing (the programmer didn't know about the NumMax being a thing), but in this particular case (and full disclosure: this is a real thing I've seen while working on a large project): when you have over 100k lines of code, it's very likely a max function like this already exists somewhere, so what do you, as a responsible person do? You look in the most obvious places, and do a quick search for func Max, only to discover that your search comes up empty. You shrug and think that it must not be as common a thing to need as you initially thought, so you just write it in the package you need, and call it a day. All this to say: names matter a lot.


I'll leave it at that for now.

I've been fighting the urge to bring this up, because you mentioned you're an experienced programmer, and I do think that considering thread safety (although in go we call it "safe for concurrent use", because routines may or may not be executed on different threads) is a hallmark of experience. That being said, to address concurrency concerns in their entirety, I really ought to add that, even though your cache implementation (through the functions you've shared) is safe for concurrent access, the values you're returning are still a map[string]any. Once you get those maps from the cache, you're still just dealing with a map which can cause data-races when iterating over a map that is being written to concurrently.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.