Round 7 revoke fixes: wildcard in split path, whitespace dedup, stale kubectl annotation

- _parse_permission_rules: dedup by stripped key so 'pods ' and 'pods'
  don't both end up in the configmap.
- _split_rule_after_revoke: apply '*' wildcard to the cell filter inside
  both URL and resource branches — previously the match phase accepted
  a revoke with apiGroups:['*'] / resources:['*'] but the per-cell loop
  did 'ag not in mr_apis' without wildcard handling, so the rule was
  re-emitted unchanged (silent no-op on existing specific rules).
- _revoke_rbac minimal-apply: drop kubectl.kubernetes.io/last-applied-
  configuration from the preserved annotations — stale snapshot confuses
  kubectl's 3-way merge on subsequent applies.
- Docstring on _atomic_rule_after_revoke: note normalized-rules contract.
- Quote '--verb=*' in integration test so the shell doesn't glob-expand
  against the test's cwd.
- 5 new unit tests: whitespace dedup, revoke-side '*' against specific
  existing cells, atomic no-resources path, URL-gate ordering, URL-path
  output ordering determinism across input permutations.

Author: anniegracehu

provider-kubeconfig.py

@@ -69,8 +69,9 @@ def _parse_permission_rules(self, perms):
                 for api_group, res_actions in perms.items():
                     for res in res_actions:
                         for resource, verbs in res.items():
-                            if resource not in resources:
-                                resources.append(resource.strip())
+                            resource_key = resource.strip()
+                            if resource_key not in resources:
+                                resources.append(resource_key)
                             rule_group = {}
                             if api_group == "non-apigroup":
                                 if NON_RESOURCE_URL_PREFIX in resource:
@@ -86,7 +87,7 @@ def _parse_permission_rules(self, perms):
                                     rule_group["resources"] = [parts[0].strip()]
                                     rule_group["resourceNames"] = [parts[1].strip()]
                                 else:
-                                    rule_group["resources"] = [resource]
+                                    rule_group["resources"] = [resource_key]
                             rule_list.append(rule_group)
                 return rule_list, resources
 
@@ -783,7 +784,13 @@ def _revoke_rbac(self, permissionfile, sa, namespace, kubeconfig):
                         if remaining_rules:
                                 # Apply a minimal object to avoid stale metadata/resourceVersion apply conflicts.
                                 labels = role_obj.get("metadata", {}).get("labels")
-                                annotations = role_obj.get("metadata", {}).get("annotations")
+                                annotations = role_obj.get("metadata", {}).get("annotations") or {}
+                                # Strip kubectl's own last-applied snapshot — it's stale after the rewrite
+                                # and confuses kubectl's 3-way merge on subsequent applies.
+                                annotations = {
+                                        k: v for k, v in annotations.items()
+                                        if k != "kubectl.kubernetes.io/last-applied-configuration"
+                                }
                                 metadata = {"name": role_name}
                                 if labels:
                                         metadata["labels"] = labels
@@ -827,9 +834,10 @@ def _split_rule_after_revoke(self, rule, matched_revoke_rules, retained_keys):
                         cell_verbs = {url: set(existing_verbs) for url in existing_urls}
                         for mr in matched_revoke_rules:
                                 mr_urls = set(mr["nonResourceURLs"])
+                                mr_urls_wild = "*" in mr_urls
                                 mr_verbs = mr["verbs"]
                                 for url in cell_verbs:
-                                        if url not in mr_urls:
+                                        if not mr_urls_wild and url not in mr_urls:
                                                 continue
                                         cell_verbs[url] = self._strip_to_set(cell_verbs[url], mr_verbs)
                         out_rules = []
@@ -860,13 +868,17 @@ def _split_rule_after_revoke(self, rule, matched_revoke_rules, retained_keys):
                 for mr in matched_revoke_rules:
                         mr_apis = set(mr["apiGroups"])
                         mr_ress = set(mr["resources"])
+                        mr_apis_wild = "*" in mr_apis
+                        mr_ress_wild = "*" in mr_ress
                         mr_names = set(mr["resourceNames"]) if mr["resourceNames"] else None
                         mr_verbs = mr["verbs"]
                         if not mr_apis or not mr_ress:
                                 continue
                         for cell in cell_verbs:
                                 ag, res, rn = cell
-                                if ag not in mr_apis or res not in mr_ress:
+                                if not mr_apis_wild and ag not in mr_apis:
+                                        continue
+                                if not mr_ress_wild and res not in mr_ress:
                                         continue
                                 if mr_names is not None and rn not in mr_names:
                                         continue
@@ -912,6 +924,7 @@ def _atomic_rule_after_revoke(self, rule, matched_revoke_rules, retained_keys):
                 """Atomic path: strip the union of matched revoke verbs from the whole rule.
                 Used when the existing rule has '*' scope or no resource dimension —
                 RBAC cannot express 'everything except X', so we trim globally or drop.
+                `matched_revoke_rules` must be normalized (dict entries from _normalize_rule).
                 """
                 combined_verbs = [v for mr in matched_revoke_rules for v in mr["verbs"]]
                 stripped = self._strip_verbs(rule.get("verbs", []), combined_verbs)

tests/test_provider_kubeconfig.py

@@ -376,6 +376,58 @@ def test_split_rule_wildcard_scope_multi_revoke_composition(self):
         self.assertEqual(len(result), 1)
         self.assertEqual(sorted(result[0]["verbs"]), ["list"])
 
+    def test_parse_permission_rules_whitespace_dedup(self):
+        """Trailing whitespace in a resource key must not create a duplicate."""
+        perms = {"": [{"pods": ["get"]}, {"pods ": ["delete"]}]}
+        _, resources = self.generator._parse_permission_rules(perms)
+        self.assertEqual(resources, ["pods"])
+
+    def test_split_rule_wildcard_revoke_matches_specific_existing(self):
+        """Revoke-side '*' in apiGroups/resources must trim specific existing cells."""
+        rule = {"apiGroups": [""], "resources": ["pods"], "verbs": ["get", "delete"]}
+        matched = [self.generator._normalize_rule(
+            {"apiGroups": ["*"], "resources": ["*"], "verbs": ["delete"]}
+        )]
+        result = self.generator._split_rule_after_revoke(rule, matched, set())
+        self.assertEqual(len(result), 1)
+        self.assertEqual(sorted(result[0]["verbs"]), ["get"])
+
+    def test_atomic_rule_no_resources_no_retained_keys(self):
+        """Atomic path on a rule with no resources/URLs contributes no retained keys."""
+        rule = {"apiGroups": [""], "verbs": ["get", "delete"]}
+        matched = [self.generator._normalize_rule(
+            {"apiGroups": [""], "resources": [], "verbs": ["delete"]}
+        )]
+        retained = set()
+        result = self.generator._split_rule_after_revoke(rule, matched, retained)
+        self.assertEqual(retained, set())
+        # _atomic_rule_after_revoke trims via _strip_verbs even without matching scope.
+        self.assertEqual(len(result), 1)
+
+    def test_rule_matches_revoke_url_gate_runs_before_resource_names(self):
+        """A URL revoke against a URL rule matches even if revoke lacks resourceNames."""
+        existing = self.generator._normalize_rule(
+            {"nonResourceURLs": ["/metrics"], "verbs": ["get"]}
+        )
+        url_revoke = self.generator._normalize_rule(
+            {"nonResourceURLs": ["/metrics"], "verbs": ["get"]}
+        )
+        self.assertTrue(self.generator._rule_matches_revoke(existing, url_revoke))
+
+    def test_split_rule_url_path_output_ordering_deterministic(self):
+        """URL-path output order is stable across input permutations."""
+        rule_a = {"nonResourceURLs": ["/a", "/b", "/c"], "verbs": ["get", "list"]}
+        rule_b = {"nonResourceURLs": ["/c", "/b", "/a"], "verbs": ["get", "list"]}
+        matched = [self.generator._normalize_rule(
+            {"nonResourceURLs": ["/b"], "verbs": ["list"]}
+        )]
+        out_a = self.generator._split_rule_after_revoke(rule_a, matched, set())
+        out_b = self.generator._split_rule_after_revoke(rule_b, matched, set())
+        self.assertEqual(
+            [(sorted(r["verbs"]), sorted(r["nonResourceURLs"])) for r in out_a],
+            [(sorted(r["verbs"]), sorted(r["nonResourceURLs"])) for r in out_b],
+        )
+
     def test_permission_fixture_files_parse_json_and_yaml(self):
         """Fixture files in tests/permission_files should load and parse via script helpers."""
         fixture_dir = os.path.join(ROOT, "tests", "permission_files")
@@ -945,7 +997,7 @@ def test_revoke_on_wildcard_existing_verbs_drops_matching_scope_rule(self):
             _run_command("kubectl create sa " + sa + " -n " + ns + self.kubeconfig_flag)
             _run_command(
                 "kubectl create clusterrole " + sa
-                + " --verb=* --resource=pods" + self.kubeconfig_flag
+                + " '--verb=*' --resource=pods" + self.kubeconfig_flag
             )
             _run_command(
                 "kubectl create clusterrolebinding " + sa