ACL: add ACL binding rule RPC and HTTP API handlers. #15529
Conversation
This change adds a new table that will store ACL binding rule objects. The two indexes allow fast lookups by their ID, or by which auth method they are linked to. Snapshot persist and restore functionality ensures this table can be saved and restored from snapshots. In order to write and delete the object to state, new Raft messages have been added. All RPC request and response structs, along with object functions such as diff and canonicalize have been included within this work as it is nicely separated from the other areas of work.
This change add the RPC ACL binding rule handlers. These handlers are responsible for the creation, updating, reading, and deletion of binding rules. The write handlers are feature gated so that they can only be used when all federated servers are running the required version.
| // TODO: version constraint will be updated for every beta or rc until we reach | ||
| // 1.5, otherwise it's hard to test the functionality | ||
| var minACLBindingRuleVersion = version.Must(version.NewVersion("1.4.3-dev")) |
There was a problem hiding this comment.
Once we go to 1.5-beta we can update this to 1.5.0 and then use Version.Core on the version we check.
nomad/acl_endpoint.go
Outdated
| lookupBindingRule, err := stateSnapshot.GetACLBindingRule(nil, bindingRule.ID) | ||
| if err != nil { | ||
| return structs.NewErrRPCCodedf(400, "ACL binding rule lookup failed: %v", err) | ||
| } | ||
| if lookupBindingRule != nil { | ||
| reply.ACLBindingRules = append(reply.ACLBindingRules, lookupBindingRule) | ||
| } |
There was a problem hiding this comment.
State store read methods don't usually return an error unless its internal. And in any case if we get either an error or a nil value for this query, that seems like an internal error (5xx) to me rather than a request error (4xx) -- we should have just written this value so if we didn't, something went very wrong!
There was a problem hiding this comment.
Agreed on both counts, thanks! Fixed within 4754d09
api/acl_test.go
Outdated
| @@ -655,3 +654,85 @@ func TestACLAuthMethods(t *testing.T) { | |||
| must.Len(t, 0, aclAuthMethodsListResp) | |||
| assertQueryMeta(t, queryMeta) | |||
| } | |||
|
|
|||
| func TestACLACLBindingRules(t *testing.T) { | |||
There was a problem hiding this comment.
| func TestACLACLBindingRules(t *testing.T) { | |
| func TestACLBindingRules(t *testing.T) { |
| // decoded. Only perform this check on updates as a generic error on | ||
| // creation might be confusing to operators as there is no specific binding | ||
| // rule request path. | ||
| if ruleID != "" && ruleID != aclBindingRule.ID { |
There was a problem hiding this comment.
Doesn't this condition imply that we allow users to set the ID on creation if they include it in the request body? The server is supposed to create it as a UUID.
There was a problem hiding this comment.
This function can be used by new creations or updates to existing binding rules. Therefore I feel it's worth checking that if the caller included an ID within the payload, that this is checked against the request path to ensure they match, otherwise we don't know which the correct one is. The RPC handler will check that the ID is found within state, indicating it is an update, before moving forward.
This comment did get me thinking about this function and I made a small modification within 4754d09 which sets the aclBindingRule.ID to the request path ID, if the former has been left empty by the caller.
Co-authored-by: Piotr Kazmierczak <[email protected]>
|
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. |
Best viewed commit by commit which is split by Nomad subsystem.
Builds on #15511
Related to #13120