Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPolicyGenerator binaries upgraded to v1.17.1; generator config and script updated to emit placement.labelSelector.matchExpressions; many generated ACM policy YAMLs migrated from apps.open-cluster-management.io PlacementRule to cluster.open-cluster-management.io/v1beta1 Placement with updated PlacementBinding references; two ManagedClusterSetBinding manifests added. ChangesPolicyGenerator upgrade and cluster selector format migration
🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels: 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-errors.Policy.yaml (1)
42-53:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove invalid
clusterConditionsfield from Placement v1beta1 spec.The
spec.clusterConditionsfield is not part of the Placement v1beta1 schema (cluster.open-cluster-management.io/v1beta1). It belongs to the legacy PlacementRule API. Placement selects clusters viaspec.predicates, and automatically filters out unavailable managed clusters by default—this stanza can be safely removed.This invalid field appears in at least three files in this PR:
deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-errors.Policy.yamldeploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-providers.Policy.yamldeploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-login.Policy.yamlApply the same removal consistently across all affected files.
Proposed fix
spec: - clusterConditions: - - status: "True" - type: ManagedClusterConditionAvailable predicates:🤖 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 `@deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-errors.Policy.yaml` around lines 42 - 53, Remove the invalid spec.clusterConditions stanza from the Placement v1beta1 resources (the Placement spec)—specifically delete the entire clusterConditions key and its array content while keeping spec.predicates intact; this should be applied to the three 50-rosa-oauth-tpl-*.Policy.yaml templates (errors, providers, login) so the Placement conforms to cluster.open-cluster-management.io/v1beta1 schema and the YAML remains syntactically valid after removal.
🧹 Nitpick comments (3)
hack/00-osd-managed-cluster-config-production.yaml.tmpl (1)
29963-29986: ⚖️ Poor tradeoffStale
placement-rule/pr-naming on the new Placement resources.These resources are now
kind: Placementbut retain PlacementRule-era names (autoscaler-podmonitor-placement-rule,pr-rosa-oauth-tpl-errors,pr-rosa-oauth-tpl-login,pr-rosa-oauth-tpl-providers). Functionally fine, but it makes the manifests harder to grep and reason about post-migration. Consider renaming in a follow-up (along with their PlacementBindingplacementRef.nameandmetadata.name); doing it in this PR risks orphaning the previously created PlacementRule objects on existing hubs unless the old names are also reaped.Also applies to: 45631-45656, 45695-45720, 45759-45784
🤖 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 `@hack/00-osd-managed-cluster-config-production.yaml.tmpl` around lines 29963 - 29986, Manifest resource names still use the old PlacementRule-era prefixes (e.g., autoscaler-podmonitor-placement-rule, pr-rosa-oauth-tpl-errors, pr-rosa-oauth-tpl-login, pr-rosa-oauth-tpl-providers) even though the kind is now Placement; update those Placement resource metadata.name values to a clearer Placement-oriented naming scheme and also update any corresponding PlacementBinding objects’ placementRef.name and metadata.name to match; do this as a follow-up change (not in this PR) and ensure you also add a migration/reaper plan for the old PlacementRule objects so they are not orphaned on existing hubs.hack/00-osd-managed-cluster-config-integration.yaml.tmpl (1)
29963-29985: ⚖️ Poor tradeoffResource name still says
-placement-rule— consider renaming for the Placement migration.
autoscaler-podmonitor-placement-ruleis now aPlacement, not aPlacementRule, but the name carries the legacy term. Likewise the matchingPlacementBindingis namedautoscaler-podmonitor-placement-binding. Renaming is non-trivial (the binding'splacementRef.nameand any downstream references would need to change in lockstep, and the live cluster would see a delete+create), so this is just an optional cleanup. If you keep the current name to avoid churn, consider it elsewhere when the resource is re-created.🤖 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 `@hack/00-osd-managed-cluster-config-integration.yaml.tmpl` around lines 29963 - 29985, The resource names still contain the legacy suffix "-placement-rule" (autoscaler-podmonitor-placement-rule) and its binding autoscaler-podmonitor-placement-binding; rename them to remove the legacy term (e.g., autoscaler-podmonitor-placement and autoscaler-podmonitor-binding) or choose a consistent new name, and update the PlacementBinding.placementRef.name and any downstream references to match exactly; if you opt to avoid live-cluster churn keep the current names but add a TODO/note to rename when the resources are recreated.hack/00-osd-managed-cluster-config-stage.yaml.tmpl (1)
29963-29966: 💤 Low valueStale "placement-rule" /
pr-naming on resources that are nowPlacements.Several migrated resources keep names tied to the old API:
- Line 29966:
autoscaler-podmonitor-placement-rule(nowkind: Placement)- Line 45634:
pr-rosa-oauth-tpl-errors- Line 45698:
pr-rosa-oauth-tpl-login- Line 45762:
pr-rosa-oauth-tpl-providersRenaming would be a breaking change for any external reference and would force
PlacementBinding.placementRef.nameupdates, so it's reasonable to defer. Just note this so future readers don't mistake them forPlacementRuleresources, and consider a follow-up cleanup once the migration has fully soaked.Also applies to: 45631-45634, 45695-45698, 45759-45762
🤖 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 `@hack/00-osd-managed-cluster-config-stage.yaml.tmpl` around lines 29963 - 29966, The manifest contains legacy resource names still using "placement-rule" / "pr-" prefixes (e.g., autoscaler-podmonitor-placement-rule, pr-rosa-oauth-tpl-errors, pr-rosa-oauth-tpl-login, pr-rosa-oauth-tpl-providers) while the resources are now kind: Placement; add a concise inline comment adjacent to each affected resource declaration (near the occurrences of autoscaler-podmonitor-placement-rule and the three pr-rosa-oauth-tpl-* resources) stating these are legacy names from the old PlacementRule API and that they map to current Placement objects, and add a short TODO noting any future rename must update PlacementBinding.placementRef.name to avoid accidental breakage and that a follow-up cleanup can be performed after migration soak.
🤖 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
`@deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-providers.Policy.yaml`:
- Around line 43-49: The Placement manifest includes an invalid
spec.clusterConditions field (the clusterConditions block under the Placement
spec) which is not part of cluster.open-cluster-management.io/v1beta1
PlacementSpec; remove the entire clusterConditions section (the status/type
entries under clusterConditions) so the PlacementSpec contains only valid fields
(e.g., predicates -> requiredClusterSelector -> labelSelector ->
matchExpressions), or if you actually need clusterConditions semantics migrate
this logic to a deprecated PlacementRule resource instead; update the YAML to
delete the clusterConditions subtree and leave the
predicates/requiredClusterSelector intact.
In `@hack/00-osd-managed-cluster-config-integration.yaml.tmpl`:
- Around line 23278-23282: Remove the duplicate apiGroups entry
"cluster.open-cluster-management.io" from the relevant Role/ClusterRole
definition in the template so apiGroups contains each group only once; locate
the resource listing apiGroups (the block that also includes permissions for
ManagedCluster/ManagedClusterSet) and delete the redundant
"cluster.open-cluster-management.io" line so the YAML/RBAC linter no longer
flags a duplicate entry.
- Around line 45631-45647: The Placement resources in the template include the
obsolete top-level field "clusterConditions" which is invalid for kind:
Placement (cluster.open-cluster-management.io/v1beta1); remove all
"clusterConditions" blocks from any Placement documents (e.g., the
rosa-oauth-tpl login/errors/providers Placement entries) and any other
occurrences in the template, leaving only supported spec fields (clusterSets,
numberOfClusters, predicates, prioritizerPolicy, tolerations, decisionStrategy,
spreadPolicy); search for the symbol "clusterConditions" and delete the entire
block and any trailing commas/empty maps so the resulting Placement spec
validates against the v1beta1 schema.
- Around line 2371-2395: The Placement resources in this template rely on
predicate filtering but lack any ManagedClusterSetBinding, so they will see zero
candidate clusters; add ManagedClusterSetBinding custom resources that bind the
openshift-acm-policies and openshift-rosa-oauth-tpl-policies namespaces to the
appropriate ManagedClusterSet(s) that contain your ROSA/HCP management and
hosted clusters. For each target ManagedClusterSet, create a
ManagedClusterSetBinding with metadata.namespace set to the target namespace
(openshift-acm-policies or openshift-rosa-oauth-tpl-policies) and
spec.managedClusterSet set to the ManagedClusterSet name so the Placement
objects (Placement) in those namespaces can select clusters from that set.
Ensure you add one binding per namespace per ManagedClusterSet that should be
considered by the Placement predicates.
In `@hack/00-osd-managed-cluster-config-production.yaml.tmpl`:
- Around line 23276-23282: The apiGroups list in the template contains a
duplicated value: cluster.open-cluster-management.io was added twice (one
occurrence replaced apps.open-cluster-management.io), so edit the apiGroups
array in hack/00-osd-managed-cluster-config-production.yaml.tmpl to remove the
duplicate cluster.open-cluster-management.io and restore/keep
apps.open-cluster-management.io if intended; ensure each api group value (e.g.,
cluster.open-cluster-management.io and apps.open-cluster-management.io) appears
only once in the apiGroups list.
- Around line 45636-45647: Remove the invalid clusterConditions field from all
Placement v1beta1 resource specs in this template: locate each Placement
resource that contains the clusterConditions key (appearing on each Placement
manifest carried over from PlacementRule v1) and delete the entire
clusterConditions stanza so the spec only contains valid fields like
numberOfClusters, clusterSets, predicates, prioritizerPolicy, tolerations,
decisionStrategy, and spreadPolicy; ensure you remove the clusterConditions
entries from all three Placement manifests so the API server will not reject or
prune the resource and, if you still need to filter by availability, replace it
with a predicates[].requiredClusterSelector.claimSelector-based selector or
policy-based reporting as noted.
In `@hack/00-osd-managed-cluster-config-stage.yaml.tmpl`:
- Around line 23276-23282: The apiGroups list in the RBAC rule contains a
duplicate "cluster.open-cluster-management.io" entry (likely from a migration) —
remove the duplicated "cluster.open-cluster-management.io" so the list contains
each group only once; if the original "apps.open-cluster-management.io" entry is
intentionally required (to tolerate legacy PlacementRule objects), restore
"apps.open-cluster-management.io" instead of dropping it. Locate the RBAC rule's
apiGroups array (the rule that currently lists
cluster.open-cluster-management.io twice) and edit that array to either
deduplicate entries or replace the extra duplicate with
"apps.open-cluster-management.io" as appropriate.
- Around line 45631-45647: The Placement resources for pr-rosa-oauth-tpl-errors,
pr-rosa-oauth-tpl-login, and pr-rosa-oauth-tpl-providers contain an invalid
spec.clusterConditions block; remove that block from each Placement (or replace
it with Placement-native fields) and instead add equivalent tolerations for the
cluster.open-cluster-management.io/unavailable and
cluster.open-cluster-management.io/unreachable taints (or omit if the
controller’s terminating-cluster filter suffices); also update the upstream
generator source
(deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-{errors,login,providers}.Policy.yaml)
so future renders of hack/00-osd-managed-cluster-config-stage.yaml.tmpl do not
reintroduce spec.clusterConditions.
---
Outside diff comments:
In `@deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-errors.Policy.yaml`:
- Around line 42-53: Remove the invalid spec.clusterConditions stanza from the
Placement v1beta1 resources (the Placement spec)—specifically delete the entire
clusterConditions key and its array content while keeping spec.predicates
intact; this should be applied to the three 50-rosa-oauth-tpl-*.Policy.yaml
templates (errors, providers, login) so the Placement conforms to
cluster.open-cluster-management.io/v1beta1 schema and the YAML remains
syntactically valid after removal.
---
Nitpick comments:
In `@hack/00-osd-managed-cluster-config-integration.yaml.tmpl`:
- Around line 29963-29985: The resource names still contain the legacy suffix
"-placement-rule" (autoscaler-podmonitor-placement-rule) and its binding
autoscaler-podmonitor-placement-binding; rename them to remove the legacy term
(e.g., autoscaler-podmonitor-placement and autoscaler-podmonitor-binding) or
choose a consistent new name, and update the PlacementBinding.placementRef.name
and any downstream references to match exactly; if you opt to avoid live-cluster
churn keep the current names but add a TODO/note to rename when the resources
are recreated.
In `@hack/00-osd-managed-cluster-config-production.yaml.tmpl`:
- Around line 29963-29986: Manifest resource names still use the old
PlacementRule-era prefixes (e.g., autoscaler-podmonitor-placement-rule,
pr-rosa-oauth-tpl-errors, pr-rosa-oauth-tpl-login, pr-rosa-oauth-tpl-providers)
even though the kind is now Placement; update those Placement resource
metadata.name values to a clearer Placement-oriented naming scheme and also
update any corresponding PlacementBinding objects’ placementRef.name and
metadata.name to match; do this as a follow-up change (not in this PR) and
ensure you also add a migration/reaper plan for the old PlacementRule objects so
they are not orphaned on existing hubs.
In `@hack/00-osd-managed-cluster-config-stage.yaml.tmpl`:
- Around line 29963-29966: The manifest contains legacy resource names still
using "placement-rule" / "pr-" prefixes (e.g.,
autoscaler-podmonitor-placement-rule, pr-rosa-oauth-tpl-errors,
pr-rosa-oauth-tpl-login, pr-rosa-oauth-tpl-providers) while the resources are
now kind: Placement; add a concise inline comment adjacent to each affected
resource declaration (near the occurrences of
autoscaler-podmonitor-placement-rule and the three pr-rosa-oauth-tpl-*
resources) stating these are legacy names from the old PlacementRule API and
that they map to current Placement objects, and add a short TODO noting any
future rename must update PlacementBinding.placementRef.name to avoid accidental
breakage and that a follow-up cleanup can be performed after migration soak.
🪄 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: 8b618b0a-7073-4c3f-a098-ba77725de237
📒 Files selected for processing (7)
deploy/hypershift-autoscaler-podmonitor/01-autoscaler-podmonitor.yamldeploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-errors.Policy.yamldeploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-login.Policy.yamldeploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-providers.Policy.yamlhack/00-osd-managed-cluster-config-integration.yaml.tmplhack/00-osd-managed-cluster-config-production.yaml.tmplhack/00-osd-managed-cluster-config-stage.yaml.tmpl
| clusterConditions: | ||
| - status: "True" | ||
| type: ManagedClusterConditionAvailable | ||
| clusterSelector: | ||
| matchExpressions: | ||
| predicates: | ||
| - requiredClusterSelector: | ||
| labelSelector: | ||
| matchExpressions: |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Open Cluster Management, does cluster.open-cluster-management.io/v1beta1 PlacementSpecsupportclusterConditions? Please provide the official schema/docs fields list.
💡 Result:
No. In cluster.open-cluster-management.io/v1beta1, PlacementSpec does not support a clusterConditions field. Official schema/docs fields list for PlacementSpec (v1beta1) - numberOfClusters [1] - clusterSets [1] - predicates [1] - prioritizerPolicy [1] - spreadPolicy [1] - tolerations [1] - decisionStrategy [1] Additionally, the user-facing Placement docs describe “Placement conditions” as status/conditions of the Placement resource (e.g., PlacementMisconfigured, PlacementSatisfied), not as a clusterConditions field in spec. PlacementSpec [2].
Citations:
- 1: https://github.com/open-cluster-management-io/placement/blob/main/README.md
- 2: https://open-cluster-management.io/docs/concepts/content-placement/placement/
Remove invalid spec.clusterConditions field from Placement v1beta1.
clusterConditions is not a valid field in cluster.open-cluster-management.io/v1beta1 PlacementSpec. It belongs to the deprecated PlacementRule resource. This field will fail schema validation or be silently dropped by the API server, causing unexpected placement behavior.
Suggested fix
spec:
- clusterConditions:
- - status: "True"
- type: ManagedClusterConditionAvailable
predicates:
- requiredClusterSelector:
labelSelector:
matchExpressions:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clusterConditions: | |
| - status: "True" | |
| type: ManagedClusterConditionAvailable | |
| clusterSelector: | |
| matchExpressions: | |
| predicates: | |
| - requiredClusterSelector: | |
| labelSelector: | |
| matchExpressions: | |
| spec: | |
| predicates: | |
| - requiredClusterSelector: | |
| labelSelector: | |
| matchExpressions: |
🤖 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 `@deploy/rosa-oauth-templates-policies/50-rosa-oauth-tpl-providers.Policy.yaml`
around lines 43 - 49, The Placement manifest includes an invalid
spec.clusterConditions field (the clusterConditions block under the Placement
spec) which is not part of cluster.open-cluster-management.io/v1beta1
PlacementSpec; remove the entire clusterConditions section (the status/type
entries under clusterConditions) so the PlacementSpec contains only valid fields
(e.g., predicates -> requiredClusterSelector -> labelSelector ->
matchExpressions), or if you actually need clusterConditions semantics migrate
this logic to a deprecated PlacementRule resource instead; update the YAML to
delete the clusterConditions subtree and leave the
predicates/requiredClusterSelector intact.
Ajpantuso
left a comment
Ajpantuso
left a comment
There was a problem hiding this comment.
The hack/* files should not be modified directly as they will be generated with the make generate-hive-templates target.
The primary change here will be to update the PolicyGen Version. The latest version will fully remove PlacementRule generation.
I suspect the Placements still need to be updated like you are doing with this PR, but let's see what the new PolicyGen output looks like
|
Thanks @Ajpantuso I will update the Policy Generator version in the makefile. I believe we would also need similar changes in the Dockerfile and Dcokerfile.prow too. |
1c953b6 to
4f30d5a
Compare
Nanyte25
left a comment
Nanyte25
left a comment
There was a problem hiding this comment.
Thanks for this @ankitkurmi — the PlacementRule → Placement migration is the right direction and the generator script changes look clean overall. A few things I'd like to understand before approving, see inline comments below.
Also — have these been validated against staging? Given the number of policies touched and the generator version bump (v1.9.1/v1.12.4 → v1.17.1), a staging smoke-test confirming PolicyStatus shows Compliant for a representative cluster would give a lot of confidence here.
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/generate-policy-config.py (1)
101-101:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t overwrite
labelSelectorwith legacy map shape during migration.Line 101 replaces the structured selector with
cluster_selectorsdirectly. That can break the intendedlabelSelector.matchExpressionscontract and may drop selector semantics from legacy configs.♻️ Suggested compatibility-safe conversion
- policy_template['policyDefaults']['placement']['labelSelector'] = cluster_selectors + # Convert legacy clusterSelectors map -> labelSelector.matchExpressions + policy_template['policyDefaults']['placement']['labelSelector'] = { + 'matchExpressions': [ + {'key': k, 'operator': 'In', 'values': [str(v)]} + for k, v in cluster_selectors.items() + ] + }#!/bin/bash # Inspect deploy config selector shapes to validate migration safety. python - <<'PY' import glob import oyaml as yaml paths = sorted(glob.glob('deploy/**/config.yaml', recursive=True)) found = 0 non_scalar = [] for p in paths: with open(p) as f: cfg = yaml.safe_load(f) or {} if 'clusterSelectors' in cfg: found += 1 cs = cfg['clusterSelectors'] if not isinstance(cs, dict) or any(isinstance(v, (dict, list)) for v in cs.values()): non_scalar.append((p, cs)) print(f"config.yaml files with clusterSelectors: {found}") if non_scalar: print("Potentially incompatible clusterSelectors shapes:") for p, cs in non_scalar: print(f"- {p}: {cs}") else: print("All detected clusterSelectors are scalar maps.") PY🤖 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 `@scripts/generate-policy-config.py` at line 101, The current assignment overwrites policy_template['policyDefaults']['placement']['labelSelector'] with cluster_selectors and can drop matchExpressions semantics; instead, detect if cluster_selectors is a flat scalar dict and convert it into labelSelector={'matchExpressions': [{'key': k, 'operator': 'In', 'values': [v]} for k,v in cluster_selectors.items()]} (treat each scalar as an equality/In match), merging with any existing policy_template['policyDefaults']['placement']['labelSelector']['matchExpressions'] if present; if cluster_selectors is not a simple scalar map, do not overwrite and leave the existing labelSelector intact (or log/raise for manual migration) so selector semantics are preserved.
🤖 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 `@Dockerfile.prow`:
- Line 18: The Dockerfile.prow currently ADDs the remote binary
"linux-amd64-PolicyGenerator" (installed as /opt/app-root/bin/PolicyGenerator)
without integrity verification; update the Dockerfile.prow to fetch the asset,
verify its sha256 equals
679504829060a38d9923fb6cfd5642323e581504c5a831ac02a87084d5d79b04 before
installing, e.g. replace the direct ADD of
"https://.../linux-amd64-PolicyGenerator" with a multi-step approach that
downloads the file into a build stage or temp location, computes and checks the
sha256 sum against the pinned digest, and only then moves/installs it to
/opt/app-root/bin/PolicyGenerator (fail the build if the checksum does not
match).
---
Duplicate comments:
In `@scripts/generate-policy-config.py`:
- Line 101: The current assignment overwrites
policy_template['policyDefaults']['placement']['labelSelector'] with
cluster_selectors and can drop matchExpressions semantics; instead, detect if
cluster_selectors is a flat scalar dict and convert it into
labelSelector={'matchExpressions': [{'key': k, 'operator': 'In', 'values': [v]}
for k,v in cluster_selectors.items()]} (treat each scalar as an equality/In
match), merging with any existing
policy_template['policyDefaults']['placement']['labelSelector']['matchExpressions']
if present; if cluster_selectors is not a simple scalar map, do not overwrite
and leave the existing labelSelector intact (or log/raise for manual migration)
so selector semantics are preserved.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f80e2220-2652-4091-8168-99b4f5f6f993
📒 Files selected for processing (4)
DockerfileDockerfile.prowscripts/generate-policy-config.pyscripts/policy-generator-config.yaml
✅ Files skipped from review due to trivial changes (1)
- Dockerfile
| RUN pip install --disable-pip-version-check oyaml | ||
|
|
||
| ADD --chmod=755 https://github.com/open-cluster-management-io/policy-generator-plugin/releases/download/v1.12.4/linux-amd64-PolicyGenerator /opt/app-root/bin/PolicyGenerator | ||
| ADD --chmod=755 https://github.com/open-cluster-management-io/policy-generator-plugin/releases/download/v1.17.1/linux-amd64-PolicyGenerator /opt/app-root/bin/PolicyGenerator |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether upstream release exposes checksum/signature assets for v1.17.1
curl -fsSL https://api.github.com/repos/open-cluster-management-io/policy-generator-plugin/releases/tags/v1.17.1 \
| jq -r '.assets[]?.name'Repository: openshift/managed-cluster-config
Length of output: 316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REPO="open-cluster-management-io/policy-generator-plugin"
TAG="v1.17.1"
# List all asset names with quick hints (size/prefix) and then filter for checksum/signature-like files.
curl -fsSL "https://api.github.com/repos/${REPO}/releases/tags/${TAG}" \
| jq -r '
"ALL_ASSETS:",
(.assets[]? | "\(.name)\t\(.size // "null")") ,
"",
"CHECKSUM_OR_SIGNATURE_CANDIDATES:",
(.assets[]? | select((.name|ascii_downcase) | test("sha256|sha512|md5|checksum|checksums|sums|sign|signature|asc|sig|gpg|provenance|cosign|sbom|attestation")) | "\(.name)\t\(.size // "null")")
' | sed -n '1,120p'
# If any checksum-like asset exists, show the first matching lines to see whether it covers linux-amd64 PolicyGenerator.
# (No downloading of the main binary; only tiny text checksum files if present.)
ASSET_URLS=$(
curl -fsSL "https://api.github.com/repos/${REPO}/releases/tags/${TAG}" \
| jq -r '.assets[]? | select((.name|ascii_downcase) | test("sha256|checksum|checksums|sums|sha512")) | .browser_download_url'
)
if [ -n "${ASSET_URLS}" ]; then
echo ""
echo "SAMPLE_CHECKSUM_FILE_CONTENTS:"
echo "${ASSET_URLS}" | head -n 5 | while read -r url; do
echo "---- ${url} ----"
curl -fsSL "$url" | sed -n '1,40p'
done
fiRepository: openshift/managed-cluster-config
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REPO="open-cluster-management-io/policy-generator-plugin"
TAG="v1.17.1"
URL="https://github.com/${REPO}/releases/download/${TAG}/linux-amd64-PolicyGenerator"
curl -fsSL "$URL" -o /tmp/PolicyGenerator
sha256sum /tmp/PolicyGeneratorRepository: openshift/managed-cluster-config
Length of output: 126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
REPO="open-cluster-management-io/policy-generator-plugin"
TAG="v1.17.1"
URL="https://github.com/${REPO}/releases/download/${TAG}/linux-amd64-PolicyGenerator"
curl -fsSL "$URL" -o /tmp/PolicyGenerator
python3 - <<'PY'
import hashlib
p="/tmp/PolicyGenerator"
h=hashlib.sha256()
with open(p,'rb') as f:
for chunk in iter(lambda: f.read(1024*1024), b''):
h.update(chunk)
print(h.hexdigest())
PYRepository: openshift/managed-cluster-config
Length of output: 141
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Dockerfile.prow"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: openshift/managed-cluster-config
Length of output: 120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="Dockerfile.prow"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
awk 'NR>=1 && NR<=80 {printf "%4d:%s\n", NR, $0}' "$FILE"Repository: openshift/managed-cluster-config
Length of output: 856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "sha256sum|sha512sum|cosign|gpg|openssl dgst|sha256" -S .Repository: openshift/managed-cluster-config
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "sha256sum|sha512sum|cosign|gpg|openssl dgst|sha256" -S .Repository: openshift/managed-cluster-config
Length of output: 50391
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "sha256sum|sha512sum|cosign|gpg|openssl dgst|sha256" -S .Repository: openshift/managed-cluster-config
Length of output: 50391
Verify PolicyGenerator binary integrity in Dockerfile.prow (pin sha256).
Dockerfile.prow line 18 downloads an executable via ADD with no checksum/signature verification. The upstream open-cluster-management-io/policy-generator-plugin v1.17.1 release assets contain only the platform binaries (no checksum/signature files), so digest pinning is required. Expected linux-amd64-PolicyGenerator sha256: 679504829060a38d9923fb6cfd5642323e581504c5a831ac02a87084d5d79b04.
🔒 Suggested hardening
+ARG POLICYGEN_VERSION=v1.17.1
+ARG POLICYGEN_SHA256=679504829060a38d9923fb6cfd5642323e581504c5a831ac02a87084d5d79b04
-ADD --chmod=755 https://github.com/open-cluster-management-io/policy-generator-plugin/releases/download/v1.17.1/linux-amd64-PolicyGenerator /opt/app-root/bin/PolicyGenerator
+ADD https://github.com/open-cluster-management-io/policy-generator-plugin/releases/download/${POLICYGEN_VERSION}/linux-amd64-PolicyGenerator /opt/app-root/bin/PolicyGenerator
+RUN python3 - <<'PY'
+import hashlib, pathlib, sys
+p = pathlib.Path("/opt/app-root/bin/PolicyGenerator")
+got = hashlib.sha256(p.read_bytes()).hexdigest()
+expected = "${POLICYGEN_SHA256}"
+if got != expected:
+ print(f"sha256 mismatch: got {got} expected {expected}", file=sys.stderr)
+ sys.exit(1)
+PY
+RUN chmod 0755 /opt/app-root/bin/PolicyGenerator🤖 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 `@Dockerfile.prow` at line 18, The Dockerfile.prow currently ADDs the remote
binary "linux-amd64-PolicyGenerator" (installed as
/opt/app-root/bin/PolicyGenerator) without integrity verification; update the
Dockerfile.prow to fetch the asset, verify its sha256 equals
679504829060a38d9923fb6cfd5642323e581504c5a831ac02a87084d5d79b04 before
installing, e.g. replace the direct ADD of
"https://.../linux-amd64-PolicyGenerator" with a multi-step approach that
downloads the file into a build stage or temp location, computes and checks the
sha256 sum against the pinned digest, and only then moves/installs it to
/opt/app-root/bin/PolicyGenerator (fail the build if the checksum does not
match).
|
Thanks @Ankit152 — the updated diff looks significantly better. All the issues I raised have been addressed: ✅ Two remaining items before I can approve: 1. ManagedClusterSetBinding (CodeRabbit critical) # Verify on a staging management cluster
oc get managedclustersetbinding -n openshift-acm-policies2. Staging validation still outstanding Otherwise Looks good to me |
|
/lgtm |
I logged into a stage
This makes sense. Thanks @Nanyte25 for the reviews! |
|
@Ankit152 I've captured a pre-migration baseline on staging SC Pre-migration baseline (PlacementRules)
Compliant (47 policies): NonCompliant (pre-existing, not migration-related): Unknown (expected — label selectors targeting absent cluster types): Post-migration pass criteria (to be validated after PR merges to staging)
Once this PR merges and deploys to |
|
/lgtm |
|
/lgtm |
|
@bmeng and @Ajpantuso Can one of you please approve or @joshbranham |
|
@Ankit152 the Final diff looks correct ✅
Full sign-off checklist:
|
This was posted in Slack and asked for a review, I left a comment there about a week ago asking what had been done to test and validate this change? If you can provide some form of proof this functions correctly in staging or integration, that would suffice. |
Ankit responded today so I will review when I get some time :) |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankit152, Nanyte25 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 |
…sources Signed-off-by: Ankit152 <ankitkurmi152@gmail.com>
Signed-off-by: Ankit152 <ankitkurmi152@gmail.com>
|
/retest |
…ns into generated Policy and Placement resources
|
@Ankit152: The following test 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. |
|
After running |
|
@Ankit152 I figured I'd give investigating a try. Running I did notice that the pipeline shows all 52 
Maybe you could temporarily add |
|
@Ankit152 I took another look at this PR and the issues are identified in the diff of the CI failure — there are two issues in the latest diff. Root cause — The script is now setting # Current (wrong) — sets labelSelector to flat dict:
policy_template['policyDefaults']['placement']['labelSelector'] = cluster_selectors
# cluster_selectors = {"hypershift.open-cluster-management.io/hosted-cluster": "true"}When CI runs Restore the # Correct — builds matchExpressions structure:
match_expressions = []
for key, value in cluster_selectors.items():
match_expressions.append({
'key': key,
'operator': 'In',
'values': [value]
})
policy_template['policyDefaults']['placement']['labelSelector'] = {
'matchExpressions': match_expressions
}Secondary issue — Makefile The first container run in the # Remove this redundant first run:
$(CONTAINER_ENGINE) run ... "${GEN_POLICY_CONFIG}; ${GEN_POLICY_CONFIG_SP}; ${GEN_POLICY}; ${GEN_CMO_CONFIG}"
# Keep only this one:
$(CONTAINER_ENGINE) run ... "${GEN_POLICY_CONFIG}; ${GEN_POLICY_CONFIG_SP}; ${GEN_POLICY}; ${GEN_CMO_CONFIG}; scripts/add-annotations-tolerations.py"Steps to fix:
|
|
@ankit and @joshbranham I found and fixed the CI failure root cause. The issue was in https://redhat.atlassian.net/browse/ROSAENG-3773 The fix detects when config.yaml already provides matchExpressions and passes them through unchanged, preserving the NoEgress / win-li / version-exclusion filters. I've pushed the fix to a branch on my fork: To get it into your PR, cherry-pick it: git remote add mfreer git@github.com:Nanyte25/managed-cluster-config.git
git fetch mfreer slsre-534
git cherry-pick a3802d52
git push origin <your-pr-branch>I verified locally that |
|
cc @Ankit152 (author) if you want to take Mark's feedback, otherwise Mark can you just open a new PR so we can get this wrapped up (ticket assigned to you anyways) |
|
Superseded by #2852. I've taken over this work (ROSAENG-3773 reassigned to me from @Ankit152) and opened a fresh PR from my fork that carries forward all the commits here plus the fix for the CI failure that was blocking this one. Root cause of the CI failure: All review history and decisions from this PR (ManagedClusterSetBinding, gap test, staging validation) are carried into #2852. Thanks @Ankit152 for the original work and @holysoles for helping investigate the CI issue. Closing in favour of #2852. |
5 participants
What type of PR is this?
(bug/feature/cleanup/documentation)
Cleanup
What this PR does / why we need it?
Migrate ACM PlacementRule resources to Placement resources
Which Jira/Github issue(s) this PR fixes?
Fixes SLSRE-534
Special notes for your reviewer:
Pre-checks (if applicable):
Tested latest changes against a cluster
Included documentation changes with PR
If this is a new object that is not intended for the FedRAMP environment (if unsure, please reach out to team FedRAMP), please exclude it with:
Summary by CodeRabbit
Chores
Updates