Skip to content

Commit b1c9e9c

Browse files
anniegracehuanniegracehu
committed
Round 20 fixes: fall through to base role, create-path strictness, YAML error message
- _revoke_rbac now tries both -update and base roles. If -update exists but no rule matches the permission file, fall through and operate on base instead of silently reporting 'nothing to revoke'. Extracted _fetch_clusterrole + _walk_rules_for_revoke to keep the main path legible. - _kubectl_or_raise takes ignore_not_found (default True for idempotent delete/read callers). _write_perm_configmap_resources passes ignore_not_found=False on the create: if the namespace doesn't exist, kubectl returns NotFound on the configmap create and was silently swallowed. - _load_permission_data now catches yaml.YAMLError and wraps it in a ValueError with the malformed-file message, so a syntax error in a user-supplied permission file produces a clear error instead of a raw yaml traceback.
1 parent 535f16a commit b1c9e9c

1 file changed

Lines changed: 82 additions & 51 deletions

File tree

‎provider-kubeconfig.py‎

Lines changed: 82 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,10 @@ def _load_permission_data(self, permissionfile):
5555
try:
5656
perms_data = json.loads(contents)
5757
except json.JSONDecodeError:
58-
perms_data = yaml.safe_load(contents)
58+
try:
59+
perms_data = yaml.safe_load(contents)
60+
except yaml.YAMLError as exc:
61+
raise ValueError(f"Permission file is neither valid JSON nor YAML: {exc}") from exc
5962
if not isinstance(perms_data, dict) or "perms" not in perms_data:
6063
raise ValueError("Permission file must define a top-level 'perms' object.")
6164
if not isinstance(perms_data["perms"], dict):
@@ -126,6 +129,7 @@ def _write_perm_configmap_resources(self, sa, namespace, kubeconfig, resources):
126129
"create configmap " + cfg_map_name + " -n " + namespace
127130
+ " --from-file=" + cfg_map_filename,
128131
kubeconfig,
132+
ignore_not_found=False,
129133
)
130134

131135

@@ -768,57 +772,48 @@ def _revoke_rbac(self, permissionfile, sa, namespace, kubeconfig):
768772
revoke_rule_list, _ = self._parse_permission_rules(perms)
769773
revoke_norm = self._normalize_rule_list(revoke_rule_list)
770774

771-
role_name = sa + "-update"
772-
rolebinding_name = sa + "-update"
773-
out, err, rc = run_command_with_code("kubectl get clusterrole " + role_name + " -o json" + kubeconfig)
774-
if rc != 0 and "(NotFound)" not in err:
775-
raise RuntimeError(f"Failed to fetch clusterrole {role_name!r}: {err.strip()}")
776-
if rc != 0:
777-
# If revoking without updating first, revoke against the base SA role.
778-
role_name = sa
779-
rolebinding_name = sa
780-
out, err, rc = run_command_with_code("kubectl get clusterrole " + role_name + " -o json" + kubeconfig)
781-
if rc != 0 and "(NotFound)" not in err:
782-
raise RuntimeError(f"Failed to fetch clusterrole {role_name!r}: {err.strip()}")
783-
784-
if not out:
775+
# Try both the -update role and the base role. If nothing matches in -update
776+
# but the permission is granted by the base role, fall through and edit base.
777+
update_role = self._fetch_clusterrole(sa + "-update", kubeconfig)
778+
base_role = self._fetch_clusterrole(sa, kubeconfig)
779+
if update_role is None and base_role is None:
785780
print(f"No clusterrole {sa!r} or {sa + '-update'!r} found; nothing to revoke.")
786781
return False
787782

788-
retained_keys = set()
789-
# Keys covered by rules the revoke actually touched. Only these may be
790-
# dropped from the perms configmap — permissions granted by a separate
791-
# role (e.g. the base role when we're editing only -update) must stay.
792-
touched_keys = set()
793-
role_obj = json.loads(out)
794-
existing_rules = role_obj.get("rules") or []
795-
remaining_rules = []
796-
for rule in existing_rules:
797-
existing_norm = self._normalize_rule(rule)
798-
matched = [r for r in revoke_norm if self._rule_matches_revoke(existing_norm, r)]
799-
if not matched:
800-
remaining_rules.append(rule)
801-
retained_keys.update(self._retained_keys_for_rule(rule))
802-
continue
803-
touched_keys.update(self._retained_keys_for_rule(rule))
804-
remaining_rules.extend(self._split_rule_after_revoke(rule, matched, retained_keys))
805-
806-
# If we're editing <sa>-update, any key the base <sa> role still grants
807-
# must stay in the configmap even when we stripped it from -update.
808-
if role_name != sa:
809-
base_out, base_err, base_rc = run_command_with_code(
810-
"kubectl get clusterrole " + sa + " -o json" + kubeconfig
811-
)
812-
if base_rc != 0 and "(NotFound)" not in base_err:
813-
raise RuntimeError(f"Failed to fetch clusterrole {sa!r}: {base_err.strip()}")
814-
if base_out:
815-
for rule in json.loads(base_out).get("rules") or []:
816-
retained_keys.update(self._retained_keys_for_rule(rule))
817-
818-
if not touched_keys:
819-
print(f"Nothing to revoke: no rule in clusterrole {role_name!r} matched the permission file.")
783+
target = None
784+
if update_role is not None:
785+
walk = self._walk_rules_for_revoke(update_role, revoke_norm)
786+
if walk["touched_keys"]:
787+
target = ("update", sa + "-update", update_role, walk)
788+
if target is None and base_role is not None:
789+
walk = self._walk_rules_for_revoke(base_role, revoke_norm)
790+
if walk["touched_keys"]:
791+
target = ("base", sa, base_role, walk)
792+
793+
if target is None:
794+
present = [
795+
name for name, role in (
796+
(sa + "-update", update_role),
797+
(sa, base_role),
798+
) if role is not None
799+
]
800+
print(f"Nothing to revoke: no rule in {' or '.join(present)} matched the permission file.")
820801
return False
821802

803+
_, role_name, role_obj, walk = target
804+
rolebinding_name = role_name
805+
retained_keys = walk["retained_keys"]
806+
touched_keys = walk["touched_keys"]
807+
existing_rules = walk["existing_rules"]
808+
remaining_rules = walk["remaining_rules"]
809+
810+
# If we edited -update, keys the base role still grants must stay in the configmap.
811+
# (The reverse isn't needed — if we edited base, -update's grants were already
812+
# narrower and we've either dropped or re-applied all it covered.)
813+
if role_name.endswith("-update") and base_role is not None:
814+
for rule in base_role.get("rules") or []:
815+
retained_keys.update(self._retained_keys_for_rule(rule))
816+
822817
# Matching a rule's scope doesn't mean we actually stripped any verbs — e.g.,
823818
# revoking 'delete' against an existing rule that only granted 'get' is a no-op.
824819
# Compare normalized rule lists to detect real change.
@@ -872,11 +867,47 @@ def _revoke_rbac(self, permissionfile, sa, namespace, kubeconfig):
872867
self._write_perm_configmap_resources(sa, namespace, kubeconfig, remaining_resources)
873868
return True
874869

875-
def _kubectl_or_raise(self, args, kubeconfig):
876-
"""Run a kubectl subcommand and raise if it fails (ignore NotFound)."""
870+
def _kubectl_or_raise(self, args, kubeconfig, *, ignore_not_found=True):
871+
"""Run a kubectl subcommand and raise on failure. Skip NotFound only for
872+
idempotent deletes/reads; create paths should pass ignore_not_found=False
873+
so a missing namespace surfaces as a hard error."""
877874
_, err, rc = run_command_with_code("kubectl " + args + kubeconfig)
878-
if rc != 0 and "(NotFound)" not in err:
879-
raise RuntimeError(f"kubectl {args} failed: {err.strip()}")
875+
if rc == 0:
876+
return
877+
if ignore_not_found and "(NotFound)" in err:
878+
return
879+
raise RuntimeError(f"kubectl {args} failed: {err.strip()}")
880+
881+
def _fetch_clusterrole(self, name, kubeconfig):
882+
"""Return the ClusterRole dict, or None if NotFound. Raises on other errors."""
883+
out, err, rc = run_command_with_code("kubectl get clusterrole " + name + " -o json" + kubeconfig)
884+
if rc == 0 and out:
885+
return json.loads(out)
886+
if "(NotFound)" in err:
887+
return None
888+
raise RuntimeError(f"Failed to fetch clusterrole {name!r}: {err.strip()}")
889+
890+
def _walk_rules_for_revoke(self, role_obj, revoke_norm):
891+
"""Apply `revoke_norm` to `role_obj`'s rules; return split result + tracking sets."""
892+
retained_keys = set()
893+
touched_keys = set()
894+
existing_rules = role_obj.get("rules") or []
895+
remaining_rules = []
896+
for rule in existing_rules:
897+
existing_norm = self._normalize_rule(rule)
898+
matched = [r for r in revoke_norm if self._rule_matches_revoke(existing_norm, r)]
899+
if not matched:
900+
remaining_rules.append(rule)
901+
retained_keys.update(self._retained_keys_for_rule(rule))
902+
continue
903+
touched_keys.update(self._retained_keys_for_rule(rule))
904+
remaining_rules.extend(self._split_rule_after_revoke(rule, matched, retained_keys))
905+
return {
906+
"retained_keys": retained_keys,
907+
"touched_keys": touched_keys,
908+
"existing_rules": existing_rules,
909+
"remaining_rules": remaining_rules,
910+
}
880911

881912
def _split_rule_after_revoke(self, rule, matched_revoke_rules, retained_keys):
882913
"""Split an existing rule into the rules that remain after applying matched revokes.

0 commit comments

Comments
 (0)