-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Rebuild CRLs on secondary performance clusters post migration and on new/updated issuers #15179
Rebuild CRLs on secondary performance clusters post migration and on new/updated issuers #15179
Conversation
fc79a42
to
6086a1d
Compare
The last force push of the last commit, was just fixing up a formatting issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I wouldn't have caught what @cipherboy saw, but since that seems correct, I'll hold off approving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the handling of active node stuff is correct.
a897075
to
63f1645
Compare
Rebased existing commits (unchanged) on top of the current pki-pod-rotation branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple small questions
builtin/logical/pki/crl_util.go
Outdated
} | ||
|
||
// requestRebuildOnActiveNode will schedule a rebuild of the CRL from the next read or write api call assuming we are the active node of a cluster | ||
func (cb *crlBuilder) requestRebuildOnActiveNode(b *backend) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this requestRebuildIfActiveNode? I initially thought this somehow sent a request to the active node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup much better name.
// rebuildIfForced is to be called by readers or periodic functions that might need to trigger | ||
// a refresh of the CRL before the read occurs. | ||
func (cb *crlBuilder) rebuildIfForced(ctx context.Context, b *backend, request *logical.Request) error { | ||
if atomic.LoadUint32(&cb.forceRebuild) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the thought around the atomic here vs just a RWLock?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebuildIfForced
is called from a number of contexts/places where, if we aren't asked to rebuild the CRL, we shouldn't and return early (e.g., the period function and/or CRL fetching has a hook to this call -- because the invalidate function doesn't have access to the relevant data for managed keys).
We'd need to pair a RWLock
with a bool
, which is equivalent to an atomic
int-ish, I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, what @cipherboy said. We are kinda trying to recreate a sync.Once with resettable behaviour along with the ability. So both a RWLock would not be good enough as the invalidate
call mainly doesn't have the context (missing storage and more importantly mountpoint) to rebuild the CRL. A sync.Once wouldn't be good enough as we need to redo this operation/reset it on subsequent invalidate
calls in a safe manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think from the earlier call, a sync.Once
can be a pointer to a sync.Once
that allows it to be "resettable", though now you need an atomic "write pointer" I think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, but our use case isn't a direct match either for a sync.Once as the initial state, say for a single node, would be to never execute under a read api call occurred. That's another reason I did not use an actual sync.Once as the initialization code would have had to execute something arbitrary and then allow it to be reset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough for me.
…new/updated issuers - Hook into the backend invalidation function so that secondaries are notified of new/updated issuer or migrations occuring on the primary cluster. Upon notification schedule a CRL rebuild to take place upon the next process to read/update the CRL or within the periodic function if no request comes in.
- Address an issue that we were scheduling the rebuilding of a CRL on standby nodes, which would not be able to write to storage. - Fix an issue with standby nodes not correctly determining that a migration previously occurred.
a26f7a5
to
6ecf617
Compare
6ecf617
to
5b9c604
Compare
new/updated issuer or migrations occuring on the primary cluster. Upon notification
schedule a CRL rebuild to take place upon the next process to read/update the CRL
or within the periodic function if no request comes in.