Update cluster_role and cluster_role_info modules #88

aljazkosir · 2019-10-30T14:12:11Z

This PR is a follow-up of #70. We add cluster role and cluster role info support as a new modules. Cluster roles adds support for Cluster-wide resource types in addition to namespaced resource types.

tadeboro

Thanks for the great implementation, @aljazkosir. But I need to mark this PR with needs more work until we make sure our idempotency checks are working as expected. Can you please check this and report back with the results? Thanks.

And if we indeed find some troubles when digging deeper, we might need to go back and fix the role module. I am mentioning @mancabizjak here just to keep her in the loop about the latest developments in the Tadej failed to properly review the role module area ;)

tadeboro · 2019-10-30T16:31:22Z

plugins/modules/cluster_role.py

+    description:
+      - Rules that the cluster role applies.
+    type: list
+    required: yes


Is the rules parameter really required? It feels a bit strange that we are demanding its presence while at the same time specifying it in a required_if condition and not making it mandatory in the argument spec. And while we are here, we should probably also check the role module and resolve any issues there also. (I am assuming here that this module started as a role clone and inherited some warts.)

And again, we really need to give sanity test some much-needed love and polish. Letting things like this slip is really not OK.

Indeed, it's confusing that the sanity checker is happy with or without this line. We should remove this here and in the role module as well, as rules is only conditionally required.

tadeboro · 2019-10-30T16:40:28Z

plugins/modules/cluster_role.py

+                    resource_names=dict(
+                        type="list",
+                    ),
+


Nitpick: this empty line is not really needed.

tadeboro · 2019-10-30T16:47:48Z

plugins/modules/cluster_role.py

+    payload = arguments.get_mutation_payload(
+        module.params, "rules"
+    )
+    payload['metadata'].pop('namespace')  # Remove it, since it's not available to cluster roles


Maybe we should teac our arguments.get_mutation_payload method to ignore namespace metadata field if the auth["namespace"] is None? We already filter the other payload arguments this way, so this would not be too much of an exception.

tadeboro · 2019-10-30T17:56:29Z

tests/integration/modules/molecule/cluster_role/playbook.yml

+              - get
+              - list


Does this test fail if we swap the verbs? If the answer is yes, we need to create a custom comparator function that will ignore the order of verbs/resources/resource names.

nice catch :D, tests are failing if verbs are swapped. Custom comparator it is.

👍 Great catch indeed, this one slipped us through for role.

mancabizjak · 2019-11-04T08:10:24Z

plugins/modules/cluster_role.py

+    description:
+      - Rules that the cluster role applies.
+    type: list
+    required: yes


Indeed, it's confusing that the sanity checker is happy with or without this line. We should remove this here and in the role module as well, as rules is only conditionally required.

mancabizjak · 2019-11-04T08:35:22Z

tests/integration/modules/molecule/cluster_role/playbook.yml

+              - get
+              - list


👍 Great catch indeed, this one slipped us through for role.

mancabizjak · 2019-11-04T08:39:59Z

tests/unit/modules/test_cluster_role_info.py

+
+
+class TestClusterRoleInfo(ModuleTestCase):
+    def test_get_all_roles(self, mocker):


I would rename the method to test_get_all_cluster_roles, just to be consistent with naming in test_cluster_role.py.

mancabizjak · 2019-11-04T08:41:06Z

tests/unit/modules/test_cluster_role_info.py

+        assert path == "/clusterroles"
+        assert context.value.args[0]["objects"] == [1, 2, 3]
+
+    def test_get_single_role(self, mocker):


Same as above, I suggest renaming to test_get_single_cluster_role for consistency.

aljazkosir · 2019-11-05T07:29:47Z

I fixed the issues, I did some changes regarding the custom comparator for rules. I created a new file `rule_utils˙ as we talked about in other PR and also backed it with some tests. I would love a feedback on the suggested changes :).

mancabizjak

I reviewed the changes, looks great 👍 Very happy to see such a comprehensive test suite for the role_utils! Just a couple of minor comments/clarifications from my side.

mancabizjak · 2019-11-05T07:43:54Z

plugins/module_utils/role_utils.py

+    sorted_current = _sort_rules(current_rules)
+    sorted_desired = _sort_rules(desired_rules)
+
+    if len(sorted_current) != len(sorted_desired):


You can move this check to the beginning of the function (if len(current_rules) != len(desired_rules)), to avoid unnecessary sorting in case the lengths are different.

mancabizjak · 2019-11-05T07:52:24Z

plugins/module_utils/utils.py

+    return False
+
+
+def sync(state, client, path, payload, check_mode, comparator=do_differ):


I would really like if we called this argument compare instead of comparator 🙂 To me, it makes more sense when the code reads like if compare(a, b) instead of if comparator(a,b) later on. Plus, comparator sounds kind of classy (no pun intended), meaning that I would expect it to point to (an instance of a) class rather than a simple function.

This is my personal preference ofc, interested to hear what @tadeboro thinks :D

mancabizjak · 2019-11-05T08:04:33Z

tests/unit/modules/test_cluter_role.py

@@ -0,0 +1,122 @@
+from __future__ import absolute_import, division, print_function


test_cluter_role.py -> test_cluster_role.py 😱 oops 😀

tests/unit/module_utils/test_role_utils.py

mancabizjak · 2019-11-05T08:17:12Z

tests/unit/module_utils/test_role_utils.py

+            [{'a': []}]
+        ) is False
+
+    def test_rules_when_value_is_none(self):


I find this test a bit confusing, what exactly did you want to test here? Maybe it's just the actual test data that confuses me - I would at least make the value of a the same in current and desired rule set (or remove a entirely, if we want to test the behavior for None case in isolation)?

Yeah, I wanted to test the None cases, so a should not really matter in that case.

Maybe it would've be better if I used the actuall keys and values used in real case data instead of a and b and numbers.

mancabizjak · 2019-11-05T09:08:15Z

plugins/module_utils/role_utils.py

+        if key == 'rules':
+            if _do_rules_differ(current['rules'], desired['rules']):
+                return True
+        elif value != current.get(key):


Although I really like how nice and readable L38 is, we already have desired['rules'] stored in value and we could also potentially store current['rules'] (which is exactly current.get(key) from L40) to current_value.

Something like:

for key, value in desired.items(): current_value = current.get(key) if key == 'rules': if _do_rules_differ(current_value, value): return True elif value != current_value: return True

I don't mind if you leave it as is though, I will have to slightly modify this part myself in #83 🙂.

Heh, that condition was not in the loop at first, that's why it looks like this 😅. I moved it there few seconds before the push. I'll change it, it looks better anyway 👍

mancabizjak · 2019-11-05T09:37:35Z

tests/unit/module_utils/test_role_utils.py

+        }
+        assert role_utils.do_differ(current, desired) is True
+
+    def test_rule_exists_but_with_additional_rules(self):


I think you meant test_role_exists_but_with_additional_rules

tadeboro

One more solid piece of code. It is almost a pleasure to read PRs like this one ;)

I added two comments regarding the code itself, but what I would also like to see is changes being split into two or three commits: one for each changeset. One the change of the sync method definition, one for module implementation (and an optional commit for modification of get_mutation_payload function).

plugins/module_utils/arguments.py

tadeboro · 2019-11-05T08:57:51Z

plugins/module_utils/role_utils.py

+def _sort_rules(rules):
+    for rule in rules:
+        for key in rule:
+            if rule[key]:
+                rule[key].sort()
+    return rules


This function mutates the input data, which is something I would rather see we avoid as much as possible. Creating a copy of the data might seem expensive, but not as expensive as tracking down the exact mutation place once something will go wrong ;)

But to be completely honest, I would try to avoid sorting the data altogether and instead convert the list of rules into a set of rules. This would mimic the semantics of the API far better. Because, let us face it, sorting two lists and then comparing them is just a low-level implementation of set comparison.

A rule in a mathematical sense is a triplet of sets (verbs, resources, and resource names). So set of rules would be represented In python as

return { (set(r["verbs"]), set(r["resources"]), set(r["resource_names"]) for r in rules }

I left out is None checks, but the general idea should be clear from the code above. We can compare those sets using regular ==.

@tadeboro I tried with the suggested approach but I had some problems. If I used "one line loop" I was getting generators inside set, which made it impossible to compare. I solved it by implementing a normal for loop that adds a rule one at a time.

Generators inside sets? That sounds like a misplaced ). I would expect troubles with sets inside a tuple that is then placed in a set itself because the whole tuple becomes unhashable.

This is a bit more polished version of the code that I had in mind:

{ ( frozenset(r["verbs"] or []), frozenset(r["resources"] or []), frozenset(r["resource_names"] or []) ) for r in rules }

Can you please check again what went wrong? Thanks.

Hmm I see, one too many () 😅. Wouldn't be better to use r.get('verbs', []) or [] for cases that key is completely missing?

Ugh, yes, forgot that arguments are not always ansible-supplied and thus some keys can be missing. Great catch. And now you can ignore my comment in review about this ;)

tadeboro

There is only one serious thing to resolve before we can merge: making sure we know what happens with empty lists in arguments. Other than that, I have no major complaints. Great work.

plugins/module_utils/role_utils.py

tadeboro · 2019-11-06T11:39:31Z

plugins/module_utils/role_utils.py

@@ -42,3 +42,40 @@ def _do_subjects_differ(a, b):
    sorted_a = sorted(a, key=lambda x: (x['type'], x['name']))
    sorted_b = sorted(b, key=lambda x: (x['type'], x['name']))
    return sorted_a != sorted_b
+
+
+def _rule_sets(rules):


Maybe we should keep this name singular since we always return a single set?

plugins/modules/cluster_role.py

tadeboro · 2019-11-06T11:47:49Z

plugins/modules/cluster_role.py

+        )
+    )
+
+    module.params['auth']['namespace'] = None  # Making sure we are not fallbacking to default


That comment is the best!

tadeboro · 2019-11-06T13:47:03Z

plugins/modules/cluster_role.py

+def validate_module_params(module):
+    params = module.params
+    if params['state'] == 'present':
+        if not params['rules']:
+            module.fail_json(
+                msg='state is present but all of the following are missing: rules'
+            )


I there a reason why this is not implemented using required_if?

required_if only checks if value is None, here we also check if rules is empty array.

tadeboro

This is ready to go. I will wait for @aljazkosir to report back with the answer about the custom validation of rules parameter and then we can smash the merge button. Thanks!

tadeboro · 2019-11-06T13:52:48Z

tests/integration/modules/molecule/cluster_role/playbook.yml

+
+    - assert:
+        that:
+          - result.objects | length == 8 # There are 6 default cluster roles


This will save us a ton of debugging once the Sensu changes defaul settings.

tadeboro · 2019-11-06T13:57:33Z

@mancabizjak Once you clear this PR, can you please merge it? Thank you.

mancabizjak

Looks great 👍 I will create a follow-up PR for the role module in order to mirror custom comparator and additional validation check added here.

aljazkosir requested review from tadeboro and mancabizjak October 30, 2019 14:12

aljazkosir changed the title ~~Update cluster_role and cluster_role_info modules~~ [WIP] Update cluster_role and cluster_role_info modules Oct 30, 2019

aljazkosir force-pushed the update-cluster-role branch from e7ea2c6 to 831ff66 Compare October 30, 2019 15:02

tadeboro suggested changes Oct 30, 2019

View reviewed changes

mancabizjak suggested changes Nov 4, 2019

View reviewed changes

mancabizjak mentioned this pull request Nov 4, 2019

Update role_binding and role_binding_info modules #83

Merged

aljazkosir force-pushed the update-cluster-role branch 3 times, most recently from 2b3faea to 67d08b6 Compare November 5, 2019 06:48

aljazkosir changed the title ~~[WIP] Update cluster_role and cluster_role_info modules~~ Update cluster_role and cluster_role_info modules Nov 5, 2019

aljazkosir force-pushed the update-cluster-role branch from 67d08b6 to 9182afe Compare November 5, 2019 07:27

aljazkosir force-pushed the update-cluster-role branch from 9182afe to c86c9ef Compare November 5, 2019 07:37

mancabizjak reviewed Nov 5, 2019

View reviewed changes

aljazkosir force-pushed the update-cluster-role branch 2 times, most recently from f5da62a to e9e647b Compare November 5, 2019 11:00

tadeboro suggested changes Nov 5, 2019

View reviewed changes

aljazkosir requested review from tadeboro and mancabizjak November 5, 2019 11:04

aljazkosir force-pushed the update-cluster-role branch from e9e647b to d1529c0 Compare November 6, 2019 09:52

Ignore namespace in metadata if it's forced to None

e75db35

aljazkosir force-pushed the update-cluster-role branch from d1529c0 to cfb48d8 Compare November 6, 2019 10:31

tadeboro reviewed Nov 6, 2019

View reviewed changes

aljazkosir force-pushed the update-cluster-role branch from cfb48d8 to 14180ef Compare November 6, 2019 13:19

Update cluster_role and cluster_role_info modules

8d6b9f6

aljazkosir force-pushed the update-cluster-role branch from 14180ef to 8d6b9f6 Compare November 6, 2019 13:23

tadeboro reviewed Nov 6, 2019

View reviewed changes

tadeboro approved these changes Nov 6, 2019

View reviewed changes

mancabizjak approved these changes Nov 7, 2019

View reviewed changes

mancabizjak merged commit fb44410 into master Nov 7, 2019

mancabizjak deleted the update-cluster-role branch November 7, 2019 07:15

This was referenced Nov 7, 2019

Prepare cluster_role_binding and cluster_role_binding_info modules #77

Closed

Prepare cluster_role and cluster_role_info modules #75

Closed

Fix validation and idempotence checks in role module #90

Closed



		class TestClusterRoleInfo(ModuleTestCase):
		def test_get_all_roles(self, mocker):

		return False


		def sync(state, client, path, payload, check_mode, comparator=do_differ):

		@@ -0,0 +1,122 @@
		from __future__ import absolute_import, division, print_function

Update cluster_role and cluster_role_info modules #88

Update cluster_role and cluster_role_info modules #88

Conversation

aljazkosir commented Oct 30, 2019 • edited Loading

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aljazkosir commented Nov 5, 2019 • edited Loading

mancabizjak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mancabizjak Nov 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tadeboro commented Nov 6, 2019

mancabizjak left a comment

Choose a reason for hiding this comment

aljazkosir commented Oct 30, 2019 •

edited

Loading

aljazkosir commented Nov 5, 2019 •

edited

Loading

mancabizjak Nov 5, 2019 •

edited

Loading