[wip]: ClusterAPIMachineManagementAWS: Strip Down Provider manifests#614
Open
hongkailiu wants to merge 1 commit into
Open
[wip]: ClusterAPIMachineManagementAWS: Strip Down Provider manifests#614hongkailiu wants to merge 1 commit into
hongkailiu wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughRemoves awsmachinepools support from OpenShift CAPI operator manifests: deletes CRD and webhook entries, removes RBAC resource/permission lines, reserializes AWS CRD schema (adds nodeInfo), and adds kustomize patches that enforce these deletions. ChangesAWS Machine Pool Removal from OpenShift Operator Manifests
🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
hongkailiu
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openshift/kustomization.yaml (2)
47-61: ⚡ Quick winHardcoded indices are fragile—consider adding
testguards.These patches rely on specific array indices (
/rules/21,/rules/20,/rules/18) that will silently remove wrong rules if upstream manifests change order or count. JSON Patch supportstestoperations to verify content before removal:- op: test path: /rules/20/resources/0 value: awsmachinepools/finalizers - op: remove path: /rules/20This makes failures explicit: if the expected resource isn't at that index, the patch fails rather than silently removing unrelated rules.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/kustomization.yaml` around lines 47 - 61, The current JSONPatch removes resources by hardcoded indices under the ClusterRole named "capa-manager-role" (paths /rules/21, /rules/20, /rules/18) which is brittle; add JSON Patch "test" operations before each "remove" to assert the expected resource value (e.g., verify that /rules/20/resources/0 equals "awsmachinepools/finalizers" and similar tests for the other targeted resources) so the patch will fail if the manifest layout changed, then keep the corresponding "remove" op for the same path; update the patch block that targets kind: ClusterRole name: capa-manager-role to include these test-then-remove pairs for each removal.
63-77: ⚡ Quick winSame fragility applies to webhook index removals.
Add
testoperations to verify the webhook name at each index before removal:- op: test path: /webhooks/7/name value: default.awsmachinepool.infrastructure.cluster.x-k8s.io - op: remove path: /webhooks/7This ensures the patch fails loudly if the webhook order changes upstream, rather than removing unrelated webhook configurations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openshift/kustomization.yaml` around lines 63 - 77, The remove ops for MutatingWebhookConfiguration (capa-mutating-webhook-configuration at /webhooks/7) and ValidatingWebhookConfiguration (capa-validating-webhook-configuration at /webhooks/8) are fragile; add preceding test operations that assert the webhook name matches default.awsmachinepool.infrastructure.cluster.x-k8s.io (test path /webhooks/7/name for the mutating entry and /webhooks/8/name for the validating entry) before performing the remove so the patch will fail if webhook ordering or indices change upstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openshift/kustomization.yaml`:
- Around line 47-61: The current JSONPatch removes resources by hardcoded
indices under the ClusterRole named "capa-manager-role" (paths /rules/21,
/rules/20, /rules/18) which is brittle; add JSON Patch "test" operations before
each "remove" to assert the expected resource value (e.g., verify that
/rules/20/resources/0 equals "awsmachinepools/finalizers" and similar tests for
the other targeted resources) so the patch will fail if the manifest layout
changed, then keep the corresponding "remove" op for the same path; update the
patch block that targets kind: ClusterRole name: capa-manager-role to include
these test-then-remove pairs for each removal.
- Around line 63-77: The remove ops for MutatingWebhookConfiguration
(capa-mutating-webhook-configuration at /webhooks/7) and
ValidatingWebhookConfiguration (capa-validating-webhook-configuration at
/webhooks/8) are fragile; add preceding test operations that assert the webhook
name matches default.awsmachinepool.infrastructure.cluster.x-k8s.io (test path
/webhooks/7/name for the mutating entry and /webhooks/8/name for the validating
entry) before performing the remove so the patch will fail if webhook ordering
or indices change upstream.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 85839f8f-08f0-47d7-81fb-20d1f73f1408
📒 Files selected for processing (2)
openshift/capi-operator-manifests/default/manifests.yamlopenshift/kustomization.yaml
hongkailiu
force-pushed
the
strip-down-manifests
branch
from
a07fb41 to
3ca608f
Compare
hongkailiu
changed the title
Member
Author
|
/test e2e-aws-capi-techpreview |
Use patchesStrategicMerge to remove awsmachinepool CRD and webhooks from the OpenShift manifests. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Hongkai Liu <hongkailiu@users.noreply.github.com>
hongkailiu
force-pushed
the
strip-down-manifests
branch
from
3ca608f to
2dbb82a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openshift/kustomization.yaml`:
- Around line 44-59: The current JSON6902 patch in openshift/kustomization.yaml
targets fixed indices (/rules/21..., /rules/20..., /rules/18/...) and is
removing the wrong RBAC rules from ClusterRole "capa-manager-role"; replace the
fragile index-based JSON6902 patch with a deterministic patch (use
patchesStrategicMerge or a whole-ClusterRole override) that explicitly removes
any resources matching "awsmachinepools", "awsmachinepools/status" and
"awsmachinepools/finalizers" from the rules array for the ClusterRole named
capa-manager-role (i.e., supply a ClusterRole patch that lists the desired rules
without those entries so kustomize merges deterministically), and add a CI
manifest check that renders the manifests and asserts that ClusterRole
"capa-manager-role" has no resources matching "awsmachinepools" (including
/status and /finalizers) to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7e93c1f1-67eb-492c-935b-6a63e0204fa7
📒 Files selected for processing (5)
openshift/capi-operator-manifests/default/manifests.yamlopenshift/kustomization.yamlopenshift/patches/remove-awsmachinepool-crd.yamlopenshift/patches/remove-awsmachinepool-mutating-webhook.yamlopenshift/patches/remove-awsmachinepool-validating-webhook.yaml
✅ Files skipped from review due to trivial changes (3)
- openshift/patches/remove-awsmachinepool-crd.yaml
- openshift/patches/remove-awsmachinepool-mutating-webhook.yaml
- openshift/patches/remove-awsmachinepool-validating-webhook.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- openshift/capi-operator-manifests/default/manifests.yaml
Comment on lines
+44
to
+59
| # index may not be stable in the JSON patch below but no better way has been identified so far | ||
| # Remove awsmachinepools from ClusterRole RBAC rules | ||
| # Remove in reverse order (higher indices first) to avoid index shifting | ||
| # Rule 21: Remove only awsmachinepools/status (index 0) from the resources array | ||
| # Rule 20: Remove entire rule (only contains awsmachinepools/finalizers) | ||
| # Rule 18: Remove only awsmachinepools (index 2) from the resources array | ||
| - target: | ||
| kind: ClusterRole | ||
| name: capa-manager-role | ||
| patch: |- | ||
| - op: remove | ||
| path: /rules/21/resources/0 | ||
| - op: remove | ||
| path: /rules/20 | ||
| - op: remove | ||
| path: /rules/18/resources/2 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the rule indices in the source ClusterRole match expectations
CLUSTERROLE_FILE=$(fd -t f 'role.yaml' config/rbac 2>/dev/null | head -1)
if [ -z "$CLUSTERROLE_FILE" ]; then
echo "ClusterRole file not found in config/rbac/"
exit 1
fi
echo "=== Checking rule indices for awsmachinepools resources ==="
# Use yq or grep to find awsmachinepools in rules
if command -v yq &>/dev/null; then
yq eval '.rules | to_entries | .[] | select(.value.resources[]? | test("awsmachinepool")) | "Rule " + (.key | tostring) + ": " + (.value.resources | join(", "))' "$CLUSTERROLE_FILE"
else
# Fallback: show context around awsmachinepool occurrences
grep -n -B5 -A2 "awsmachinepool" "$CLUSTERROLE_FILE" || echo "No awsmachinepool references found"
fiRepository: openshift/cluster-api-provider-aws
Length of output: 173
🏁 Script executed:
#!/bin/bash
set -euo pipefail
MANIFESTS="$(kustomize build openshift)"
if command -v yq >/dev/null 2>&1; then
RESOURCES="$(printf '%s\n' "$MANIFESTS" | yq eval \
'select(.kind=="ClusterRole" and .metadata.name=="capa-manager-role") | .rules[]?.resources[]?' - \
| sort -u)"
echo "$RESOURCES" | rg -i 'awsmachinepools?'
echo "$RESOURCES" | rg -i 'awsmachinepools?'
if echo "$RESOURCES" | rg -i 'awsmachinepools?'; then
echo "Found awsmachinepools* in ClusterRole/capa-manager-role; patch is wrong or indices changed."
exit 1
fi
else
# Fallback: coarse check
if printf '%s\n' "$MANIFESTS" | rg -i "kind: ClusterRole|name: capa-manager-role|awsmachinepools?"; then
echo "Unable to precisely validate without yq; ensure ClusterRole/capa-manager-role has no awsmachinepools* resources."
exit 1
fi
fiRepository: openshift/cluster-api-provider-aws
Length of output: 128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for capa-manager-role =="
rg -n "capa-manager-role" .
echo "== Inspect openshift/kustomization.yaml patch section =="
# Print around the reported lines 44-59 if file exists
if [ -f openshift/kustomization.yaml ]; then
sed -n '35,80p' openshift/kustomization.yaml | nl -ba
else
echo "openshift/kustomization.yaml not found"
fi
echo "== Locate ClusterRole definition(s) =="
# Try common locations and match whole resource blocks
rg -n --type-add 'yaml:*.y*ml' --type yaml -S "kind:\s*ClusterRole" -g'*.y*ml' .
echo "== Search for awsmachinepools references =="
rg -n -i "awsmachinepools|awsmachinepool" .Repository: openshift/cluster-api-provider-aws
Length of output: 567
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== openshift/kustomization.yaml (lines 40-70) =="
sed -n '40,70p' openshift/kustomization.yaml
echo
echo "== openshift/capi-operator-manifests/default/manifests.yaml: find ClusterRole capa-manager-role block =="
# Show context around the ClusterRole name occurrences
rg -n "name:\s*capa-manager-role" openshift/capi-operator-manifests/default/manifests.yaml
echo
echo "== Context around first capa-manager-role occurrence in manifests.yaml (around line 270-340) =="
sed -n '250,340p' openshift/capi-operator-manifests/default/manifests.yaml
echo
echo "== Search awsmachinepools in manifests.yaml =="
rg -n -i "awsmachinepools|awsmachinepool" openshift/capi-operator-manifests/default/manifests.yaml || trueRepository: openshift/cluster-api-provider-aws
Length of output: 3049
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="openshift/capi-operator-manifests/default/manifests.yaml"
echo "== Find kind: ClusterRole and its proximity to capa-manager-role =="
rg -n "kind:\s*ClusterRole|name:\s*capa-manager-role" "$FILE" | head -n 60
echo
echo "== Show around first capa-manager-role (expected ClusterRole) =="
# show around line 22 occurrence
sed -n '1,130p' "$FILE"
echo
echo "== Search for awsmachinepools resources inside manifests.yaml =="
rg -n -i "resources:\s*\n.*awsmachinepools|awsmachinepools/(status|finalizers)?|awsmachinepools" "$FILE" || true
echo
echo "== More targeted: print any lines mentioning awsmachinepools (case-insensitive) =="
rg -n -i "awsmachinepools" "$FILE" || trueRepository: openshift/cluster-api-provider-aws
Length of output: 2620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== openshift/kustomization.yaml (top) =="
sed -n '1,140p' openshift/kustomization.yaml
echo
echo "== Python parse: ClusterRole/capa-manager-role rules indices 0..(max) =="
python3 - <<'PY'
import sys, re
from pathlib import Path
try:
import yaml
except Exception:
import subprocess
subprocess.check_call([sys.executable, "-m", "pip", "install", "--quiet", "pyyaml"])
import yaml
path = Path("openshift/capi-operator-manifests/default/manifests.yaml")
docs = list(yaml.safe_load_all(path.read_text()))
cluster_role = None
for d in docs:
if not isinstance(d, dict):
continue
if d.get("kind") == "ClusterRole" and d.get("metadata", {}).get("name") == "capa-manager-role":
cluster_role = d
break
if not cluster_role:
print("ClusterRole/capa-manager-role not found in manifests.yaml")
sys.exit(1)
rules = cluster_role.get("rules") or []
print(f"rules_count={len(rules)}")
targets = [18,20,21]
for idx in targets:
if idx >= len(rules):
print(f"rules[{idx}] = <out of range>")
continue
r = rules[idx]
apiGroups = r.get("apiGroups")
resources = r.get("resources")
verbs = r.get("verbs")
print(f"\nrules[{idx}]:")
print(f" apiGroups={apiGroups}")
print(f" resources={resources}")
print(f" verbs={verbs}")
print("\n== Any resources containing awsmachinepool(s) anywhere in ClusterRole rules ==")
hits=[]
for i,r in enumerate(rules):
for res in (r.get("resources") or []):
if isinstance(res,str) and re.search(r"awsmachinepools?", res, re.IGNORECASE):
hits.append((i,res))
if hits:
for i,res in hits[:50]:
print(f" rules[{i}] resource={res}")
if len(hits)>50:
print(f" ... total_hits={len(hits)}")
else:
print(" no awsmachinepool(s) resources found in ClusterRole/capa-manager-role")
PYRepository: openshift/cluster-api-provider-aws
Length of output: 2914
Fix fragile JSON6902 hard-coded RBAC indices in openshift/kustomization.yaml
The patch removals in openshift/kustomization.yaml (/rules/21/resources/0, /rules/20, /rules/18/resources/2) are documented as targeting awsmachinepools*, but the current ClusterRole/capa-manager-role rule array at those indices maps to awsmachines, the */status rules (awsmachines/status, awsmanagedclusters/status, awsmanagedmachinepools/status), and awsmanagedclusters respectively—meaning the patch is very likely removing the wrong RBAC rules. Update the patch to target the intended awsmachinepools* entries based on the rendered ClusterRole (not fixed indices), and add a CI check on the rendered manifests ensuring ClusterRole/capa-manager-role no longer includes awsmachinepools* resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openshift/kustomization.yaml` around lines 44 - 59, The current JSON6902
patch in openshift/kustomization.yaml targets fixed indices (/rules/21...,
/rules/20..., /rules/18/...) and is removing the wrong RBAC rules from
ClusterRole "capa-manager-role"; replace the fragile index-based JSON6902 patch
with a deterministic patch (use patchesStrategicMerge or a whole-ClusterRole
override) that explicitly removes any resources matching "awsmachinepools",
"awsmachinepools/status" and "awsmachinepools/finalizers" from the rules array
for the ClusterRole named capa-manager-role (i.e., supply a ClusterRole patch
that lists the desired rules without those entries so kustomize merges
deterministically), and add a CI manifest check that renders the manifests and
asserts that ClusterRole "capa-manager-role" has no resources matching
"awsmachinepools" (including /status and /finalizers) to prevent regressions.
|
@hongkailiu: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
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
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.
Summary by CodeRabbit