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

Question: Fencing tokens for storage #205

Open
eest opened this issue Sep 22, 2022 · 4 comments
Open

Question: Fencing tokens for storage #205

eest opened this issue Sep 22, 2022 · 4 comments
Labels
feature request Request for new feature or functionality question Further information is requested

Comments

@eest
Copy link

eest commented Sep 22, 2022

What is your question?

When implementing a storage backend the comment describing Lock() mentions the use of fencing tokens:

certmagic/storage.go

Lines 108 to 112 in 9826a4c

// Additionally, implementations may wish to support fencing
// tokens (https://martin.kleppmann.com/2016/02/08/how-to-do-distributed-locking.html)
// in order to be robust against long process pauses, extremely
// high network latency (or other factors that get in the way of
// renewing lock leases).

The way I understand this is that once you have the lock further interaction with the backend while holding the lock, like writing a file, should then include such a token so that the write can fail if the token is out of date.

What I am not understanding is this: given that Lock() takes a name, potentially meaning there could be multiple different locks held for different operations at the same time, how would I map a Store() request with the appropriate lock so that it can access the correct fencing token?

What have you already tried?

I have investigated the Store() call and from what I can tell the only information available to it is the key name and data that should be written.

@eest eest added the question Further information is requested label Sep 22, 2022
@mholt
Copy link
Member

mholt commented Sep 22, 2022

That is a great question and is probably why I haven't implemented it yet.

I'm willing to revise the Storage interface to accept a fencing token as a parameter. We can actually do it in a non-breaking way by making the new parameter variadic (for now -- I'd want to help the few implementations I know of to update, then remove the variadicity).

@eest
Copy link
Author

eest commented Sep 23, 2022

It seems to me that since a storage implementation where the call to Lock() returns some object that you need to unlock inside the call to Unlock() you are already needing to keep track of that lock object inside the storage struct. This can be done with something like a map[string]<whatever> (protected by its own mutex when using it) where the string key is based on that name.

Since this is the case it seems to me the easiest approach for how this would work for Store()and friends is that the additional variable sent to it is the same name that is sent to Lock() and Unlock(). This also means that you dont have to decide on how to represent a given storage backends fencing token format (is it an int? does the int ever go negative? could it be a string?) since that would be stored inside whatever custom thing you use as a value for the map.

Does that make sense?

@mholt
Copy link
Member

mholt commented Sep 29, 2022

@eest Yeah, I think so:

type FileStorage struct {
    ...
    fencingTokens   map[string]uint
    fencingTokensMu sync.Mutex
}

(or something like that)

And the fencing tokens are handled completely internally.

Did I read you right?

@eest
Copy link
Author

eest commented Oct 1, 2022

@mholt: Yes, that is correct. See for example the Consul based storage implementation linked from the wiki which already stores a lock and mutex inside its struct:
https://github.com/pteich/caddy-tlsconsul/blob/3811ba667582f042ca05f6d894757ab33d972542/storage.go#L26-L27

... or the redis based one which skips a separate mutex but uses a *sync.Map instead:
https://github.com/gamalan/caddy-tlsredis/blob/66e1c61412decebb977790af25f7a6ce5c9b5878/storageredis.go#L136

The implementation specifics differ a bit, but they both store the lock object they create in a map keyed by the name supplied to Lock() or Unlock(). It should not be a huge leap to store additional information like a fencing token in those setups (maybe there could even already be something fencing token capable inside whatever is stored in the map as it is).

@mholt mholt added the feature request Request for new feature or functionality label Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new feature or functionality question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants