Rebuild CRLs on secondary performance clusters post migration and on new/updated issuers #15179
Conversation
stevendpclark
force-pushed
the
stevendpclark/crl-processing-on-secondary
branch
from
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.
This looks good to me. I wouldn't have caught what @cipherboy saw, but since that seems correct, I'll hold off approving.
stevendpclark
force-pushed
the
stevendpclark/crl-processing-on-secondary
branch
from
a897075
to
63f1645
Compare
|
Rebased existing commits (unchanged) on top of the current pki-pod-rotation branch. |
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.
Can we call this requestRebuildIfActiveNode? I initially thought this somehow sent a request to the active node.
There was a problem hiding this comment.
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.
What's the thought around the atomic here vs just a RWLock?
There was a problem hiding this comment.
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.
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.
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.
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.
…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.
stevendpclark
force-pushed
the
stevendpclark/crl-processing-on-secondary
branch
from
a26f7a5
to
6ecf617
Compare
stevendpclark
force-pushed
the
stevendpclark/crl-processing-on-secondary
branch
from
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.