-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
core: add ACL role state schema and functionality. #13955
core: add ACL role state schema and functionality. #13955
Conversation
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.
LGTM; just the question on having an ID for policies
44bc65e
to
9107b63
Compare
9107b63
to
7ee6f9a
Compare
7ee6f9a
to
9f24ae3
Compare
This commit includes the new state schema for ACL roles along with state interaction functions for CRUD actions. The change also includes snapshot persist and restore functionality and the addition of FSM messages for Raft updates which will come via RPC endpoints.
9f24ae3
to
7a1e05f
Compare
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.
LGTM! Just the random thoughts that can be ignored
// Ensure the role hash is not zero to provide defense in depth. This | ||
// should be done outside the state store, so we do not spend time here | ||
// and thus Raft, when it, can be avoided. | ||
if len(role.Hash) == 0 { | ||
role.SetHash() | ||
} |
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 rambling, but I wish in retrospect the objects that have these Hash values were made from constructors or a factory or something - enforcing the hash to be set on creation rather than by hand
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.
That is a good shout. I will merge this PR as is to unblock some additional PR that are lined up, but will look into this approach as a follow up item.
if len(a.Hash) == 0 { | ||
a.SetHash() | ||
} |
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.
More rambling: one thing we could do is split SetHash
into
SetHash
- only set the hash if the length is zeroResetHash
always sets the hash to updated values
And that way we can avoid wrapping calls to SetHash
with the length check in all the places where we do that
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 like the approach and with the above i'll take a look into this as a follow up to enhance the hash setting and raise a followup to look into similar for other structs.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This commit includes the new state schema for ACL roles along with
state interaction functions for CRUD actions.
The change also includes snapshot persist and restore
functionality and the addition of FSM messages for Raft updates
which will come via RPC endpoints.
related: #13120
targets: feature branch