OCPCLOUD-3368: Adding annotations to manifests for CVO to identify - #588
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughThe PR adds ChangesCompatibilityRequirements annotations
ClusterAPI annotations
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
manifests/0000_20_crd-compatibility-checker_08_deployment.yaml (1)
38-87: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd explicit securityContext to the container spec.
The container lacks explicit
securityContextsettings. While the pod template annotation referencesrestricted-v3SCC, the coding guidelines require explicit security settings in the manifest itself.As per coding guidelines, Kubernetes manifests should include:
runAsNonRoot: truereadOnlyRootFilesystem: trueallowPrivilegeEscalation: false- Drop ALL capabilities and add only required ones
🔒 Proposed securityContext addition
- name: compatibility-requirements-controllers image: registry.ci.openshift.org/openshift:cluster-capi-operator command: - ./crd-compatibility-checker args: - --diagnostics-address=:8443 + securityContext: + runAsNonRoot: true + allowPrivilegeEscalation: false + readOnlyRootFilesystem: true + capabilities: + drop: + - ALL env:🤖 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 `@manifests/0000_20_crd-compatibility-checker_08_deployment.yaml` around lines 38 - 87, Add an explicit securityContext to the container spec for the container named compatibility-requirements-controllers: set runAsNonRoot: true, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, and configure capabilities to drop ["ALL"] (and only add any specific capabilities if truly required). Update the container block that contains ports/volumeMounts/readinessProbe/livenessProbe to include this securityContext so the manifest no longer relies solely on the pod SCC annotation.Source: Coding guidelines
🧹 Nitpick comments (1)
manifests/0000_20_crd-compatibility-checker_08_deployment.yaml (1)
76-79: ⚡ Quick winConsider adding resource limits to complement requests.
The container defines resource requests but no limits. As per coding guidelines, resource limits (cpu, memory) should be set on every container to prevent resource exhaustion and ensure predictable scheduling behavior.
📊 Proposed resource limits addition
resources: requests: cpu: 10m memory: 50Mi + limits: + cpu: 100m + memory: 200Mi🤖 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 `@manifests/0000_20_crd-compatibility-checker_08_deployment.yaml` around lines 76 - 79, The container resource spec currently only sets requests (cpu: 10m, memory: 50Mi) in the resources block; add corresponding resource limits to prevent resource exhaustion. Update the same resources section for the container in manifests/0000_20_crd-compatibility-checker_08_deployment.yaml by adding limits.cpu and limits.memory (e.g., cpu: "100m" and memory: "128Mi" or values appropriate for the app) alongside the existing requests so both requests and limits are defined for the container.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@manifests/0000_20_crd-compatibility-checker_08_deployment.yaml`:
- Around line 38-87: Add an explicit securityContext to the container spec for
the container named compatibility-requirements-controllers: set runAsNonRoot:
true, readOnlyRootFilesystem: true, allowPrivilegeEscalation: false, and
configure capabilities to drop ["ALL"] (and only add any specific capabilities
if truly required). Update the container block that contains
ports/volumeMounts/readinessProbe/livenessProbe to include this securityContext
so the manifest no longer relies solely on the pod SCC annotation.
---
Nitpick comments:
In `@manifests/0000_20_crd-compatibility-checker_08_deployment.yaml`:
- Around line 76-79: The container resource spec currently only sets requests
(cpu: 10m, memory: 50Mi) in the resources block; add corresponding resource
limits to prevent resource exhaustion. Update the same resources section for the
container in manifests/0000_20_crd-compatibility-checker_08_deployment.yaml by
adding limits.cpu and limits.memory (e.g., cpu: "100m" and memory: "128Mi" or
values appropriate for the app) alongside the existing requests so both requests
and limits are defined for the container.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 75630441-0604-4f2c-9c0c-02f159d02830
📒 Files selected for processing (35)
manifests/0000_20_cluster-api-tls-config_role.yamlmanifests/0000_20_crd-compatibility-checker_00_namespace.yamlmanifests/0000_20_crd-compatibility-checker_02_service_account.yamlmanifests/0000_20_crd-compatibility-checker_03_rbac_roles.yamlmanifests/0000_20_crd-compatibility-checker_04_rbac_bindings.yamlmanifests/0000_20_crd-compatibility-checker_05_metrics-service.yamlmanifests/0000_20_crd-compatibility-checker_07_webhook-service.yamlmanifests/0000_20_crd-compatibility-checker_08_deployment.yamlmanifests/0000_20_crd-compatibility-checker_09_allow-egress-operators.yamlmanifests/0000_20_crd-compatibility-checker_10_allow-ingress-to-webhook.yamlmanifests/0000_30_cluster-api-installer_00_namespace.yamlmanifests/0000_30_cluster-api-installer_00_tombstones.yamlmanifests/0000_30_cluster-api-installer_01_metrics-service.yamlmanifests/0000_30_cluster-api-installer_01_serviceaccount.yamlmanifests/0000_30_cluster-api-installer_02_clusterrole.yamlmanifests/0000_30_cluster-api-installer_03_clusterrolebinding.yamlmanifests/0000_30_cluster-api-installer_05_deployment.yamlmanifests/0000_30_cluster-api-installer_06_clusterapi.yamlmanifests/0000_30_cluster-api_00_namespace.yamlmanifests/0000_30_cluster-api_00_tombstones-4.22-tpnu.yamlmanifests/0000_30_cluster-api_01_credentials-request.yamlmanifests/0000_30_cluster-api_02_service_account.yamlmanifests/0000_30_cluster-api_02_webhook-service.yamlmanifests/0000_30_cluster-api_03_rbac_roles.yamlmanifests/0000_30_cluster-api_04_rbac_bindings.yamlmanifests/0000_30_cluster-api_10_metrics-service.yamlmanifests/0000_30_cluster-api_10_webhooks.yamlmanifests/0000_30_cluster-api_11_deployment.yamlmanifests/0000_30_cluster-api_12_clusteroperator.yamlmanifests/0000_30_cluster-api_13_allow-ingress-to-metrics-controllers.yamlmanifests/0000_30_cluster-api_14_allow-ingress-to-metrics-operators.yamlmanifests/0000_30_cluster-api_15_allow-egress-controllers.yamlmanifests/0000_30_cluster-api_16_allow-egress-operators.yamlmanifests/0000_30_cluster-api_17_default-deny.yamlmanifests/0000_30_cluster-api_18_allow-ingress-to-webhook.yaml
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | ||
| capability.openshift.io/name: ClusterAPI |
There was a problem hiding this comment.
| capability.openshift.io/name: ClusterAPI | |
| capability.openshift.io/name: CompatibilityRequirements |
This ClusterRole is also used by the CompatibilityRequirements operator (see system:openshift:compatibility-requirements-read-tls-configuration)
miyadav
changed the title
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
There was a problem hiding this comment.
This file looks like a rebase error? We shouldn't be adding this.
There was a problem hiding this comment.
yeah rebased and pushed again , seems like the rebase miss resulted in annotations not being added as well which is also done.
|
@miyadav could you rebase this one? thanks |
openshift-ci
Bot
added
the
needs-rebase
|
@miyadav Also there are commits with Draft in them, let's fix it, TY |
openshift-ci
Bot
removed
the
needs-rebase
| # These NetworkPolicies allows egress traffic required for the CAPI operator | ||
| # deployments. | ||
| # The operator needs broad internet access for cluster management operations, | ||
| # cloud provider API calls, and communication with various services. | ||
| # | ||
| # This policy allows all egress traffic from the capi-controllers pod, which is | ||
| # necessary because the operator needs to communicate with: | ||
| # - Kubernetes API server for cluster management operations | ||
| # - Cloud provider APIs for infrastructure management | ||
| # - Container registries and other external services | ||
| # | ||
| # This approach is more practical than overly granular rules since the operator | ||
| # needs broad access to function properly in various environments. | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| annotations: | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | ||
| capability.openshift.io/name: ClusterAPI | ||
| name: allow-egress-operators | ||
| namespace: openshift-cluster-api | ||
| spec: | ||
| egress: | ||
| # Allow all egress traffic - operator needs broad access | ||
| - {} # Empty rule allows all egress | ||
| podSelector: | ||
| matchExpressions: | ||
| - key: k8s-app | ||
| operator: In | ||
| values: | ||
| - capi-controllers | ||
| policyTypes: | ||
| - Egress | ||
| --- | ||
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| annotations: | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| release.openshift.io/feature-gate: "ClusterAPIMachineManagement" | ||
| capability.openshift.io/name: ClusterAPI | ||
| name: allow-egress-operators | ||
| namespace: openshift-cluster-api-operator | ||
| spec: | ||
| egress: | ||
| # Allow all egress traffic - operator needs broad access | ||
| - {} # Empty rule allows all egress | ||
| podSelector: | ||
| matchExpressions: | ||
| - key: k8s-app | ||
| operator: In | ||
| values: | ||
| - capi-operator | ||
| policyTypes: |
There was a problem hiding this comment.
the same is present already with _14 and _05 manifests , this is rebase issue. Will remove these. Also while checking these realised other manifest related to promethues/servicemonitors are also present which also needs annotation , added them too. PTAL
|
/retest-required |
1 similar comment
|
/retest-required |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
…ntrollers/clusteroperator/suite_test.go, matching the pattern used by infracluster/suite_test.go,since the default 1s timeout is too short
| namespace: openshift-compatibility-requirements-operator | ||
| annotations: | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| include.release.openshift.io/single-node-developer: "true" | ||
| exclude.release.openshift.io/internal-openshift-hosted: "true" | ||
| release.openshift.io/feature-gate: "CRDCompatibilityRequirementOperator" | ||
| capability.openshift.io/name: CompatibilityRequirements | ||
| labels: | ||
| k8s-app: compatibility-requirements-controllers |
There was a problem hiding this comment.
This isn't a CVO-managed asset. These annotations don't do anything here. We shouldn't be touching this.
There was a problem hiding this comment.
Let me update this , thanks
|
/test unit |
|
/approve |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mdbooth The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
miyadav
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@miyadav: This pull request references OCPCLOUD-3368 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
/retest-required |
|
/test e2e-gcp-ovn-techpreview |
|
We can have this test as substitute for e2e-gcp-ovn-techpreview since it is not related to our changes , if it fails again. |
|
/test e2e-gcp-ovn-techpreview |
openshift-ci-robot
added
the
verified
|
@miyadav: This PR has been marked as verified by DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/retest |
|
Mass failures were due to kube rebase. |
|
@miyadav: 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. |
dbddb63
into
openshift:main
5 participants
Manifests changes , annoations added for capability to be identified by CVO . ( linked PR )
WIP - OCPCLOUD-3368
/hold
Generated by - claude-opus-4-6(2.1.169)
Summary by CodeRabbit