ACL refactors: Functionize more of the unpacking stage #4039
+570
−230
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
So that it's beneath its helpers. No code changes otherwise. Signed-off-by: Reid Wahl <[email protected]>
Further improvements are coming. For now, we have a recursive call and a new (temporary) forward declaration of parse_acl_entry(). Signed-off-by: Reid Wahl <[email protected]>
This involves removing some const qualifiers, because pcmk__xpath_search() takes a non-const xmlDoc. Our convention is to use a non-const pointer argument whenever we use one of the argument's fields (in this case, the doc field) as a non- const. At first, this feels like a use case for pcmk__xe_resolve_idref(), but that function would need to be extended in at least two ways: * It would need to match a PCMK_XA_ID attribute instead of a PCMK_XA_ID_REF attribute. * It would need to match a referenced element whose type differs from that of the referencing element (PCMK_XE_ACL_ROLE vs. PCMK_XE_ROLE). Also note that we search the entire document now. This is technically wasteful but is in keeping with how an XML idref would normally be resolved. An ID should be unique within the document. Signed-off-by: Reid Wahl <[email protected]>
And rename to unpack_acl_target(). We could simplify further, except that we've been allowing <acl_target> and <acl_group> elements to contain <acl_permission> elements and unpacking them in violation of the schema. I've marked this as "fix" to add to the change log, because it does change behavior: if <role> elements are nested within <acl_role> elements, they will now be ignored. They should never have been configured in the first place. Thus far, I have made an effort to maintain backward compatibility even for configurations that the schema doesn't allow. I have not done so here. A workaround would risk an infinite loop of following references, and trying to prevent that is not worth the trouble. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
After significant refactoring, I feel more confident about when a given function is always called with a given element type. Signed-off-by: Reid Wahl <[email protected]>
And correct the Doxygen. This function doesn't "mark up the XML tree" or "tack things onto relevant nodes" at all. It simply finds the ACLs that apply to the given user and adds them to a list. pcmk__apply_acls() does the "tacking on." Signed-off-by: Reid Wahl <[email protected]>
This fixes a minor regression that was introduced via commit f87588f. (Pacemaker 3.0.1). As of that commit, if the acl_source argument of xml_acl_filtered_copy() is NULL, we pass acl_source down the following call path: xml_acl_filtered_copy() -> pcmk__enable_acl() -> pcmk__unpack_acl() -> get_xpath_object() At that point, we dereference it by passing xml_obj->doc to the new pcmk__xpath_search() function. The new function accepts an xmlDoc, while the old xpath_search() accepted an xmlNode. Prior to that commit, we passed xml_obj itself to xpath_search(). If xml_obj were NULL, xpath_search() would fail the CRM_CHECK(xml_top != NULL) condition and return NULL without dereferencing it. Here we simply return early without allocating target XML if acl_source or its doc is NULL. This seems reasonable: if no source is provided for ACL definitions, then docpriv->acls will be NULL after unpacking, and nothing will get filtered out. Signed-off-by: Reid Wahl <[email protected]>
Also rename to pcmk__enable_acls(). Signed-off-by: Reid Wahl <[email protected]>
And add Doxygen to free_acl() and pcmk__free_acls(). Signed-off-by: Reid Wahl <[email protected]>
And improve the log message. Signed-off-by: Reid Wahl <[email protected]>
This allows for consolidating three log messages into one more thorough one. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
pcmk__g_strcat() was the result of intensive benchmarking and our obsession with performance a few years ago. In the worst case, it made a few percentage points of difference in run time on certain scheduler inputs, versus using function calls that have printf specifiers. I really don't think that improvement is enough to justify the decrease in readability. g_string_append_printf() seems more readable here. Signed-off-by: Reid Wahl <[email protected]>
We've been supporting these, but we should at least notify the user that it's incorrect. Also add Doxygen to create_acl() and make the struct anonymous in the xml_acl_t typedef. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Also: * Add Doxygen. * Move it to a position above parse_acl_mode(). * Match exactly. Existing callers always pass a single enum value. Signed-off-by: Reid Wahl <[email protected]>
Signed-off-by: Reid Wahl <[email protected]>
Also rename variables, use uint32_t for the flag group, and don't call pcmk__is_set() on the enum (mode) argument since it's a single flag. Signed-off-by: Reid Wahl <[email protected]>
nrwahl2
force-pushed
the
nrwahl2-acls_first
branch
from
66b5a6a to
9f40e00
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the next batch of #4013.