[CNV-63822] Test plan out-out of role aggregation#28
[CNV-63822] Test plan out-out of role aggregation#28rlobillo wants to merge 2 commits intoRedHatQE:mainfrom
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:
WalkthroughAdds a new Quality Engineering Software Test Plan (STP) document for the Role Aggregation Opt-Out feature (CNV-63822) under Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Around line 122-123: Update the test framework table entry that currently
reads "ginkgo for tier1 tests inside kubevirt repo" to use the proper noun
"Ginkgo" (i.e., "Ginkgo for tier1 tests inside kubevirt repo"); locate the table
row under the "Test Framework" heading and replace the lowercase "ginkgo" token
with "Ginkgo" to match upstream documentation and the adjacent "pytest" usage.
- Around line 158-161: The table rows for KubeVirt PR `#16350` are missing entries
in the "Test Scenario(s)" column; update the four rows whose descriptions start
with "RoleAggregationStrategy config should keep aggregate labels when
RoleAggregationStrategy is nil", "RoleAggregationStrategy configuration should
keep aggregate labels when RoleAggregationStrategy is AggregateToDefault",
"RoleAggregationStrategy configuration should create ClusterRole without
aggregate labels when RoleAggregationStrategy is Manual", and
"RoleAggregationStrategy configuration should remove aggregate labels from
existing ClusterRole when strategy changes to Manual" by adding a concrete test
scenario text for each (briefly describe the test steps and expected outcome)
or, if upstream merge is pending, insert "TBD" in the Test Scenario(s) column
for each row so traceability is preserved.
- Line 162: Change the YAML boolean to lowercase: update the example that sets
the HCO spec key spec.roleAggregation.enabled from `False` to `false` so it
conforms to YAML/Kubernetes boolean conventions and standard YAML 1.2
formatting.
🧹 Nitpick comments (2)
stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md (2)
9-10: Include the VEP issue in the Metadata “Enhancement(s)” field.The enhancement reference is currently a PR; consider adding the VEP issue link (and keep the PR in notes) so the metadata points to the canonical enhancement record. Based on learnings: “In the openshift-virtualization-tests-design-docs repository, not all features have OpenShift enhancement PRs. When no enhancement PR exists, it's acceptable to reference only the HLD (High-Level Design) document in the Enhancement(s) field of the STP metadata table.”
Also applies to: 42-42
158-161: Avoid repeating the same Requirement ID across consecutive rows.For the KubeVirt PR
#16350rows, leave the Requirement ID blank after the first row to reduce redundancy. Based on learnings: “In the openshift-virtualization-tests-design-docs repository, when documenting test scenarios in the "Test Scenarios & Traceability" table where multiple test scenarios fall under the same epic, it's acceptable and preferred to leave the Requirement ID cells empty for subsequent rows after the first row which contains the epic ID (e.g., CNV-28924 (epic)). Repeating the epic ID in each row is considered redundant.”
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should keep aggregate labels when RoleAggregationStrategy is AggregateToDefault` || tier1 automation | P0 | | ||
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should create ClusterRole without aggregate labels when RoleAggregationStrategy is Manual` || tier1 auto | P0 | | ||
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual` || tier1 auto | P0 | | ||
| | CNV-63822 (Acceptance 1) | Feature can be enabled via config | Set `spec.roleAggregation.enabled: False` in HCO CR; verify config persists | tier2 automation | P0 | |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file to understand the context
git ls-files | grep -E "stp\.md|STP" | head -20Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 216
🏁 Script executed:
# Find and read the specific file
find . -name "stp.md" -path "*CNV-63822*" 2>/dev/null | head -5Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 151
🏁 Script executed:
# Check if file exists in the repo
find . -name "stp.md" | head -10Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 178
🏁 Script executed:
# Read the specific file to see the context around line 162
cat -n ./stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md | sed -n '155,170p'Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 3024
🏁 Script executed:
# Let's see more context to understand what this table is documenting
cat -n ./stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md | sed -n '145,175p'Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 3603
🏁 Script executed:
# Look for any HCO API specification or examples in the repo
find . -type f \( -name "*.md" -o -name "*.yaml" -o -name "*.yml" \) -exec grep -l "spec.roleAggregation" {} \; 2>/dev/nullRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 151
🏁 Script executed:
# Search for any references to HCO CR or YAML configs in the doc
grep -r "roleAggregation\|HCO CR" ./stps --include="*.md" | head -20Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 476
🏁 Script executed:
# Check if there are any actual YAML examples or API definitions in the repo
find . -type f -name "*.yaml" -o -name "*.yml" | xargs grep -l "roleAggregation" 2>/dev/null | head -10Repository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 77
🌐 Web query:
Kubernetes YAML boolean format lowercase false vs False
💡 Result:
In Kubernetes manifests, false and False are both typically accepted as booleans when the target Kubernetes API field is a boolean, because the YAML parsers commonly used in the Kubernetes toolchain (Go YAML) preserve YAML 1.1-style boolean resolution (case variants like False, and even yes/no/on/off in some contexts). [1][3][4]
That said, the portable / safest form is:
- Use lowercase JSON-compatible booleans:
true/false(YAML 1.2 aligns booleans to these). [2]
And if you actually need the literal string "false" (common in Kubernetes):
- ConfigMap
data:values are strings, so write"false"(quoted), notfalse(boolean). [5]
Rule of thumb: use false (lowercase) for boolean-typed fields; use "false" (quoted) for string-typed fields (ConfigMap data, most env var values, etc.). [2][5]
Sources:
[1] YAML 1.1 bool type (includes false|False|FALSE, etc.) https://yaml.org/type/bool.html
[2] YAML 1.2 spec (booleans true/false) https://yaml.org/spec/1.2-old/spec.html
[3] go-yaml issue discussing YAML 1.1 booleans vs YAML 1.2 behavior go-yaml/yaml#214
[4] Go YAML docs noting 1.1 bool compatibility https://pkg.go.dev/go.yaml.in/yaml/v4
[5] ConfigMap data must be strings; quote "false" https://stackoverflow.com/questions/63905890/kubernetes-how-to-set-boolean-type-variable-in-configmap
🌐 Web query:
YAML 1.2 boolean values lowercase true false
💡 Result:
In YAML 1.2, the canonical boolean forms are the lowercase scalars:
truefalse[1]
However, under YAML 1.2’s Core schema (the recommended default), many parsers also accept the case variants:
True,TRUE,False,FALSE[2]
If you need strict JSON compatibility (YAML as a JSON superset), use only lowercase true / false. [1]
Sources:
[1] YAML 1.2.2 “Changes in version 1.2” (yaml.org)
[2] YAML 1.2 discussion of JSON vs Core schema boolean resolution (Stack Overflow)
🏁 Script executed:
# Look at the full STP document to understand context better
head -100 ./stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.mdRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 7895
🏁 Script executed:
# Check for any examples or documentation about HCO API
grep -n -A 5 -B 5 "spec.roleAggregation" ./stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.mdRepository: RedHatQE/openshift-virtualization-tests-design-docs
Length of output: 2203
Use lowercase YAML boolean (false) instead of False.
Kubernetes YAML conventions use lowercase booleans; change spec.roleAggregation.enabled: False to spec.roleAggregation.enabled: false for consistency with standard YAML 1.2 and Kubernetes documentation.
🤖 Prompt for AI Agents
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` at line 162, Change
the YAML boolean to lowercase: update the example that sets the HCO spec key
spec.roleAggregation.enabled from `False` to `false` so it conforms to
YAML/Kubernetes boolean conventions and standard YAML 1.2 formatting.
| - Unprivileged users cannot access kubevirt resources without explicit RoleBinding (when disabled): | ||
| - Explicit RoleBindings (admin, edit, view) grant access correctly | ||
| - admin = create/delete/modify VM/VMI | ||
| - edit = modify VM/VMIs but cannot create/delete |
There was a problem hiding this comment.
this one needs to be confirmed, according to the role the creation of VMs can be achieved with kubevirt.io:edit role
STP ready for QE team review while waiting for upstream merge
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md (3)
166-166: Use lowercase YAML boolean (falsenotFalse).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` at line 166, The YAML boolean in the table row for CNV-63822 uses Python-style `False`; change it to lowercase YAML boolean `false` by updating the example value `spec.roleAggregation.enabled: False` to `spec.roleAggregation.enabled: false` so the document shows valid YAML for the setting referenced in the CNV-63822 (Acceptance 1) row.
162-165: Fill in missing "Test Scenario(s)" for KubeVirt PR#16350rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 162 - 165, Add concrete "Test Scenario(s)" entries for each KubeVirt PR `#16350` row: for the row titled "RoleAggregationStrategy config should keep aggregate labels when RoleAggregationStrategy is nil" add a scenario that deploys or updates ClusterRoles with nil RoleAggregationStrategy and verifies existing aggregate labels remain on ClusterRoles and aggregated ClusterRoleBindings still grant permissions; for "RoleAggregationStrategy configuration should keep aggregate labels when RoleAggregationStrategy is AggregateToDefault" add a scenario that sets RoleAggregationStrategy to AggregateToDefault, creates expected aggregate ClusterRoles, and verifies aggregate labels are present and permissions are aggregated; for "RoleAggregationStrategy configuration should create ClusterRole without aggregate labels when RoleAggregationStrategy is Manual" add a scenario that sets strategy to Manual, creates ClusterRoles and verifies aggregate labels are not added and aggregation does not occur; and for "RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual" add a scenario that starts with AggregateToDefault producing labels, flips strategy to Manual, and verifies existing ClusterRoles have aggregate labels removed and aggregated permissions are revoked. Include steps: setup initial state, exact strategy change, resource creation or update, and assertions for labels and permission aggregation for each test description.
126-127: Capitalize "Ginkgo" (proper noun).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 126 - 127, The table entry uses a lowercase "ginkgo"; update the text "ginkgo for tier1 tests inside kubevirt repo" to use the proper noun "Ginkgo" (i.e., "Ginkgo for tier1 tests inside kubevirt repo") so the tool name is capitalized consistently.
🧹 Nitpick comments (1)
stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md (1)
9-11: Move the VEP link into the metadata table and remove the duplicate Jira field.Two issues:
The
Enhancement(s)field references only the implementation PR (#16350) but the actual enhancement/design document is the VEP athttps://github.com/kubevirt/enhancements/issues/160(currently placed as a free-floating link at line 42). The Enhancement(s) row should point to the VEP; the implementation PR can be noted as a dependency in the entry criteria section.
Feature in JiraandJira Trackingare identical links to CNV-63822 — the duplicate field adds no information. Keep one (e.g.,Jira) and drop the other.📝 Proposed metadata fix
-| **Enhancement(s)** | KubeVirt PR `#16350` (pending merge) | -| **Feature in Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | -| **Jira Tracking** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | +| **Enhancement(s)** | [KubeVirt VEP `#160`](https://github.com/kubevirt/enhancements/issues/160) | +| **Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) |Remove the free-floating VEP link at line 42 after incorporating it into the table.
Based on learnings, when an enhancement/VEP exists it should be referenced in the Enhancement(s) metadata field rather than placed outside the metadata table.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 9 - 11, Update the metadata table so the Enhancement(s) row points to the VEP URL (https://github.com/kubevirt/enhancements/issues/160) instead of the implementation PR; move the implementation PR reference (KubeVirt PR `#16350`) into the entry criteria/dependencies section and remove the free‑floating VEP link currently outside the table; also remove the duplicate Jira field by keeping a single Jira row (e.g., "Jira" linking to CNV-63822) and delete the other redundant "Feature in Jira"/"Jira Tracking" entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Line 191: The reviewers line currently lists "albarker-rh" without an @
mention; update the reviewers list string "[sig-iuo representative / `@orenc1`
`@hmeir` `@OhadRevah` albarker-rh]" to include the missing @ so it reads "...
`@OhadRevah` `@albarker-rh`]" ensuring the reviewer handle is properly mentioned.
- Line 171: The table cell header/value "Y Upgrade Testing" contains an
unintended leading "Y"; update that cell to either "Upgrade Testing" (remove the
leading "Y") or, if the intent was to indicate the Y-stream, replace it with the
explicit phrase "Y‑stream Upgrade Testing". Locate the exact string "Y Upgrade
Testing" in the table row and make the replacement so the row reads e.g.
"Upgrade Testing | Config preserved on Y upgrades (4.21.z → 4.22.0) | …" or
"Y‑stream Upgrade Testing | …" accordingly.
- Around line 50-55: The "In Scope" list has a dangling colon and misaligned
bullets—make the items starting with "Unprivileged users cannot access kubevirt
resources without explicit RoleBinding (when disabled):" and the subsequent
"Explicit RoleBindings (admin, edit, view) grant access correctly" and its three
subpoints into proper sub-bullets by keeping the colon on the parent line and
indenting the following lines two spaces so they become nested sub-items; ensure
the admin/edit/view explanations ("admin = Allows all operations...", "edit =
Allows all operations...", "view = Accepts get list and watch...") are indented
as sub-bullets under "Explicit RoleBindings (admin, edit, view) grant access
correctly" for correct Markdown rendering.
- Around line 175-179: Update the Note 1 table so the "what can do" column lists
the full capability sets for the kubevirt roles rather than only the
distinguishing action: change the admin row to state that kubevirt.io:admin
permits all kubevirt.io operations (create, get, update, delete, start/stop VMs,
etc.) and change the edit row to state that kubevirt.io:edit permits all
kubevirt.io operations except deletecollections (including
create/delete/update/start/stop VMs as applicable); reference the existing Scope
section descriptions for wording consistency and ensure the table entries
explicitly mention kubevirt.io:admin and kubevirt.io:edit to avoid ambiguity.
---
Duplicate comments:
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Line 166: The YAML boolean in the table row for CNV-63822 uses Python-style
`False`; change it to lowercase YAML boolean `false` by updating the example
value `spec.roleAggregation.enabled: False` to `spec.roleAggregation.enabled:
false` so the document shows valid YAML for the setting referenced in the
CNV-63822 (Acceptance 1) row.
- Around line 162-165: Add concrete "Test Scenario(s)" entries for each KubeVirt
PR `#16350` row: for the row titled "RoleAggregationStrategy config should keep
aggregate labels when RoleAggregationStrategy is nil" add a scenario that
deploys or updates ClusterRoles with nil RoleAggregationStrategy and verifies
existing aggregate labels remain on ClusterRoles and aggregated
ClusterRoleBindings still grant permissions; for "RoleAggregationStrategy
configuration should keep aggregate labels when RoleAggregationStrategy is
AggregateToDefault" add a scenario that sets RoleAggregationStrategy to
AggregateToDefault, creates expected aggregate ClusterRoles, and verifies
aggregate labels are present and permissions are aggregated; for
"RoleAggregationStrategy configuration should create ClusterRole without
aggregate labels when RoleAggregationStrategy is Manual" add a scenario that
sets strategy to Manual, creates ClusterRoles and verifies aggregate labels are
not added and aggregation does not occur; and for "RoleAggregationStrategy
configuration should remove aggregate labels from existing ClusterRole when
strategy changes to Manual" add a scenario that starts with AggregateToDefault
producing labels, flips strategy to Manual, and verifies existing ClusterRoles
have aggregate labels removed and aggregated permissions are revoked. Include
steps: setup initial state, exact strategy change, resource creation or update,
and assertions for labels and permission aggregation for each test description.
- Around line 126-127: The table entry uses a lowercase "ginkgo"; update the
text "ginkgo for tier1 tests inside kubevirt repo" to use the proper noun
"Ginkgo" (i.e., "Ginkgo for tier1 tests inside kubevirt repo") so the tool name
is capitalized consistently.
---
Nitpick comments:
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Around line 9-11: Update the metadata table so the Enhancement(s) row points
to the VEP URL (https://github.com/kubevirt/enhancements/issues/160) instead of
the implementation PR; move the implementation PR reference (KubeVirt PR `#16350`)
into the entry criteria/dependencies section and remove the free‑floating VEP
link currently outside the table; also remove the duplicate Jira field by
keeping a single Jira row (e.g., "Jira" linking to CNV-63822) and delete the
other redundant "Feature in Jira"/"Jira Tracking" entry.
| - Verify role aggregation can be disabled via hyperconvergeds.hco.kubevirt.io config | ||
| - Unprivileged users cannot access kubevirt resources without explicit RoleBinding (when disabled): | ||
| - Explicit RoleBindings (admin, edit, view) grant access correctly | ||
| - admin = Allows all operations on API group "kubevirt.io", including deletecollection. | ||
| - edit = Allows all operations on API group "kubevirt.io" excluding deletecollection operation. | ||
| - view = Accepts get list and watch verbs on API group "kubevirt.io", so cannot start/create/update/stop/delete VMs. |
There was a problem hiding this comment.
Fix the dangling colon and misaligned sub-bullets under the "In Scope" list.
Line 51 ends with a colon (:) implying the following lines are sub-items, but lines 52–55 are rendered as sibling top-level bullets in Markdown. They need two-space indentation to become sub-bullets of line 51.
📝 Proposed fix
-- Unprivileged users cannot access kubevirt resources without explicit RoleBinding (when disabled):
-- Explicit RoleBindings (admin, edit, view) grant access correctly
- - admin = Allows all operations on API group "kubevirt.io", including deletecollection.
- - edit = Allows all operations on API group "kubevirt.io" excluding deletecollection operation.
- - view = Accepts get list and watch verbs on API group "kubevirt.io", so cannot start/create/update/stop/delete VMs.
+- Unprivileged users cannot access kubevirt resources without explicit RoleBinding (when disabled):
+ - Explicit RoleBindings (admin, edit, view) grant access correctly
+ - admin = Allows all operations on API group "kubevirt.io", including deletecollection.
+ - edit = Allows all operations on API group "kubevirt.io" excluding deletecollection operation.
+ - view = Accepts get list and watch verbs on API group "kubevirt.io", so cannot start/create/update/stop/delete VMs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 50 - 55,
The "In Scope" list has a dangling colon and misaligned bullets—make the items
starting with "Unprivileged users cannot access kubevirt resources without
explicit RoleBinding (when disabled):" and the subsequent "Explicit RoleBindings
(admin, edit, view) grant access correctly" and its three subpoints into proper
sub-bullets by keeping the colon on the parent line and indenting the following
lines two spaces so they become nested sub-items; ensure the admin/edit/view
explanations ("admin = Allows all operations...", "edit = Allows all
operations...", "view = Accepts get list and watch...") are indented as
sub-bullets under "Explicit RoleBindings (admin, edit, view) grant access
correctly" for correct Markdown rendering.
|
|
||
| | Field | Details | | ||
| |:-----------------------|:--------------------------------------------------------| | ||
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | |
| | Field | Details | | ||
| |:-----------------------|:--------------------------------------------------------| | ||
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | | ||
| | **Feature in Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | |
There was a problem hiding this comment.
please attach the feature https://issues.redhat.com/browse/CNV-50792
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | | ||
| | **Feature in Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | | ||
| | **Jira Tracking** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | | ||
| | **QE Owner(s)** | Ramon Lobillo (@rlobillo) | |
|
|
||
| ### **II. Software Test Plan (STP)** | ||
|
|
||
| #### **1. Scope of Testing** |
There was a problem hiding this comment.
Please align with the new STP template https://github.com/RedHatQE/openshift-virtualization-tests-design-docs/blob/main/stps/stp-template/stp.md
In general for the document
|
|
||
| | Non-Goal | Rationale | PM/ Lead Agreement | | ||
| |:----------------------------------------------------------|:------------------------------------------------------------------------------|:-------------------| | ||
| | Full regression with opt-out enabled from fresh install | Deferred to post-GA manual testing if time-constrained | [ ] TBD | |
There was a problem hiding this comment.
right, removing and adding to test list.
| | Full regression with opt-out enabled from fresh install | Deferred to post-GA manual testing if time-constrained | [ ] TBD | | ||
| | External IdP compatibility (LDAP, Active Directory) | RBAC is IdP-agnostic; HTPasswd testing validates core logic | [ ] TBD | | ||
| | Multi-tenant cluster scale testing (100+ users) | RBAC overhead negligible; functional correctness sufficient at smaller scale | [ ] TBD | | ||
| | Testing kubevirt.io:migrate role aggregation | kubevirt.io:migrate has no aggregate labels (not a Kubernetes base role); already requires explicit RoleBinding regardless of strategy | [ ] TBD | |
There was a problem hiding this comment.
Please add a link to the existing tests you mentioned
| | Environment Component | Configuration | Specification Examples | | ||
| |:----------------------------------------------|:-------------------------------|:-------------------------------------------------------------------| | ||
| | **Cluster Topology** | Standard or SNO | Feature works on all topologies; multi-node preferred | | ||
| | **OCP & OpenShift Virtualization Version(s)** | OCP 4.21+ with CNV 4.22 | Target version where feature introduced | |
| | **Storage** | Any RWX storage class | ocs-storagecluster-ceph-rbd-virtualization | | ||
| | **Network** | Default (OVN-Kubernetes) | No special network requirements | | ||
| | **Required Operators** | OpenShift Virtualization | Standard CNV installation | | ||
| | **Platform** | Any supported platform | Prefer AWS or bare-metal for CI integration | |
|
|
||
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Review Requirements** | [x] | Admins can disable automatic kubevirt.io role aggregation via config | Per CNV-63822 epic acceptance criteria | |
There was a problem hiding this comment.
the stp should not be about implementation details but what is the requirement.
in this case it would be something like "allow customers to control user privileges....."
btw - the epic needs grooming.
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Review Requirements** | [x] | Admins can disable automatic kubevirt.io role aggregation via config | Per CNV-63822 epic acceptance criteria | | ||
| | **Understand Value** | [x] | Enables strict RBAC: users must explicitly get kubevirt.io permissions | Required for regulated/multi-tenant environments | |
There was a problem hiding this comment.
the value here should expalin what is the added value of this feature (without mentioing customer names)
| |:---------------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Review Requirements** | [x] | Admins can disable automatic kubevirt.io role aggregation via config | Per CNV-63822 epic acceptance criteria | | ||
| | **Understand Value** | [x] | Enables strict RBAC: users must explicitly get kubevirt.io permissions | Required for regulated/multi-tenant environments | | ||
| | **Customer Use Cases** | [x] | Regulated environments, multi-tenant clusters, security-hardened deployments | Aligns with enterprise RBAC requirements | |
There was a problem hiding this comment.
for each bullet - please add an short use case - what does "Regulated environments" mean?
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Developer Handoff/QE Kickoff** | [ ] | Pending KubeVirt PR #16350 merge; will schedule once API is confirmed | Need exact config field name and allowed values | | ||
| | **Technology Challenges** | [x] | RBAC testing requires unprivileged user (HTPasswd IdP already supported) | Using existing test infrastructure | |
There was a problem hiding this comment.
i'd go here with n/a - this is not a real challenge rather than what is needed to test the feature
|
|
||
| ### **II. Software Test Plan (STP)** | ||
|
|
||
| #### **1. Scope of Testing** |
| | CNV-63822 (Acceptance 2) | Unprivileged user blocked without RoleBinding (*Note 1) | Verify ForbiddenError when unprivileged user lacks binding | tier2 automation | P0 | | ||
| | CNV-63822 (Acceptance 3) | Explicit RoleBinding grants access (*Note 1) | Verify new user gains access after RoleBinding created | tier2 automation | P0 | | ||
| | CNV-63822 (Acceptance 4) | Feature can be disabled via config (*Note 1) | Verify new user gains access after feature disabling | tier2 automation | P0 | | ||
| | Default Behavior | Role aggregation enabled by default (Back. Comp.)| Verify default config enables automatic role aggregation | Regression | P0 | |
There was a problem hiding this comment.
this is not a specific scenaio but rather - run tier2 tests (appears already in the planned tests above)
| | CNV-63822 (Acceptance 3) | Explicit RoleBinding grants access (*Note 1) | Verify new user gains access after RoleBinding created | tier2 automation | P0 | | ||
| | CNV-63822 (Acceptance 4) | Feature can be disabled via config (*Note 1) | Verify new user gains access after feature disabling | tier2 automation | P0 | | ||
| | Default Behavior | Role aggregation enabled by default (Back. Comp.)| Verify default config enables automatic role aggregation | Regression | P0 | | ||
| | Y Upgrade Testing | Config preserved on Y upgrades (4.21.z → 4.22.0) | Test upgrade path preserves configuration and RBAC behavior | Regression | P0 | |
There was a problem hiding this comment.
i am not sure what is the use case here
is it from a version without the feature to a version with the feature? if so - what does Test upgrade path preserves configuration and RBAC behavior verify?
what are the risks and what should be tested as part of upgrade?
There was a problem hiding this comment.
this is regression testing too. I'm removing for the list.
| | Default Behavior | Role aggregation enabled by default (Back. Comp.)| Verify default config enables automatic role aggregation | Regression | P0 | | ||
| | Y Upgrade Testing | Config preserved on Y upgrades (4.21.z → 4.22.0) | Test upgrade path preserves configuration and RBAC behavior | Regression | P0 | | ||
|
|
||
| **Note 1:** Tests should create a ns with one global role assigned at a time and perform below actions to confirm functionality is correctly provided: |
There was a problem hiding this comment.
these are details that should be in the test description
you should describe end user scenarios that will later be reflected in the tests
e.g
as an admin, I want to grant users with an option to only view VMs -> Verify unpriv user can only view VM....
| **Note 2:** tier2 automation tests can be inspired on *migrate* clusterRole tests: [test_migration_rights.py](https://github.com/RedHatQE/openshift-virtualization-tests/blob/main/tests/virt/cluster/migration_and_maintenance/rbac_hardening/test_migration_rights.py) | ||
|
|
There was a problem hiding this comment.
this is not relevant in an STP
|
|
||
| ### **III. Test Scenarios & Traceability** | ||
|
|
||
| | Requirement ID | Requirement Summary | Test Scenario(s) | Test Type(s) | Priority | |
There was a problem hiding this comment.
what about virtctl options like create and expose? can a user without view permissions run virtclt fslist , virtctl credentials etc? what about storage options like virtctl vmexport?
what about other resources in the ns? anything that a user can exploit?
what about view-edit? can a user have edit but not view? what will happen in the ui?
edit - "start/stop VM" - i assume a user with edit permissions can run any action on the vm (e.g pause/ reset etc), right? should it be tested?
how does the config affect vm migration?
based on the feature - you need to test console access for view role
based on the use cases hidden in the feature (this is why i mentioned the epic is not groomed) - you need to support giving access to specific vms in a ns to user1 and to other vms to user2 ("kubevirt.io:view is automatically aggregated to the view CR, the user would have access to all the consoles in the project")
There was a problem hiding this comment.
I am replying on the email thread to address this concern.
There was a problem hiding this comment.
for visibility - i am not famialir with the roles enough to answer what is needed; please check what is needed
|
|
||
| | Field | Details | | ||
| |:-----------------------|:--------------------------------------------------------| | ||
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | |
| | Field | Details | | ||
| |:-----------------------|:--------------------------------------------------------| | ||
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | | ||
| | **Feature in Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | |
| | **Enhancement(s)** | KubeVirt PR #16350 (pending merge) | | ||
| | **Feature in Jira** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | | ||
| | **Jira Tracking** | [CNV-63822](https://issues.redhat.com/browse/CNV-63822) | | ||
| | **QE Owner(s)** | Ramon Lobillo (@rlobillo) | |
|
|
||
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Review Requirements** | [x] | Admins can disable automatic kubevirt.io role aggregation via config | Per CNV-63822 epic acceptance criteria | |
| | Check | Done | Details/Notes | Comments | | ||
| |:---------------------------------------|:-----|:--------------------------------------------------------------------------------|:------------------------------------------------------| | ||
| | **Review Requirements** | [x] | Admins can disable automatic kubevirt.io role aggregation via config | Per CNV-63822 epic acceptance criteria | | ||
| | **Understand Value** | [x] | Enables strict RBAC: users must explicitly get kubevirt.io permissions | Required for regulated/multi-tenant environments | |
| | CNV-63822 (Acceptance 2) | Unprivileged user blocked without RoleBinding (*Note 1) | Verify ForbiddenError when unprivileged user lacks binding | tier2 automation | P0 | | ||
| | CNV-63822 (Acceptance 3) | Explicit RoleBinding grants access (*Note 1) | Verify new user gains access after RoleBinding created | tier2 automation | P0 | | ||
| | CNV-63822 (Acceptance 4) | Feature can be disabled via config (*Note 1) | Verify new user gains access after feature disabling | tier2 automation | P0 | | ||
| | Default Behavior | Role aggregation enabled by default (Back. Comp.)| Verify default config enables automatic role aggregation | Regression | P0 | |
|
|
||
| ### **III. Test Scenarios & Traceability** | ||
|
|
||
| | Requirement ID | Requirement Summary | Test Scenario(s) | Test Type(s) | Priority | |
There was a problem hiding this comment.
I am replying on the email thread to address this concern.
|
|
||
| ### **II. Software Test Plan (STP)** | ||
|
|
||
| #### **1. Scope of Testing** |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md (1)
147-148: Normalize test type naming for consistency.Line 147 and Line 148 use
tier1 autowhile adjacent rows usetier1 automation. Keep one convention across the table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 147 - 148, Two table rows referencing "KubeVirt PR `#16350`" use "tier1 auto" while the rest use "tier1 automation"; update those rows (the entries with titles `RoleAggregationStrategy configuration should create ClusterRole without aggregate labels when RoleAggregationStrategy is Manual` and `RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual`) to use the consistent test type string "tier1 automation" (or vice versa if your project standard is the shorter form) so the table uses a single convention throughout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Around line 111-113: The Test Tools & Frameworks table currently has empty
entries under the headings "Test Framework", "CI/CD", and "Other Tools";
populate this table in stp.md with the chosen testing framework(s) (e.g., Unit,
Integration, E2E frameworks), the CI/CD system and pipeline steps responsible
for running tests, and any other tooling (e.g., test runners, coverage, mocking,
reporting) so QE has clear execution ownership and framework expectations before
review; update the cells corresponding to the headers "Test Framework", "CI/CD",
and "Other Tools" with concrete names and brief notes on scope/responsibility.
- Line 157: Replace the inconsistent role identifier `kubevirt:admin` with the
standardized `kubevirt.io:admin` in the table row that currently reads "As an
admin, I can disable the feature via config in hyperconverged CR | Verify config
persists once disabled and unprivileged user with admin role in a namespace can
perform kubevirt:admin action" so the traceability text matches other entries
using the `kubevirt.io:*` format; update the row text where the role string
appears (search for "kubevirt:admin") to ensure consistent role identifier usage
across the document.
- Around line 154-156: Fix the typo "unprevileged" to "unprivileged" in the
table rows that mention the roleBindings "kubevirt.io:admin",
"kubevirt.io:edit", and "kubevirt.io:view" so all occurrences in those test
description cells are corrected for spelling and searchability.
- Around line 28-29: The markdown table under the "Customer Use Cases" cell
contains raw pipe characters that break table parsing; edit the table cell in
stp.md (the row beginning with "**Customer Use Cases**") and either escape each
raw pipe as "\|" or replace the inline pipes with HTML line breaks (e.g., use
"<br>" and bullets) so the two bullets "multi-tenant clusters|different
namespaces..." and "Resources usage control|cluster admin..." render as intended
and do not split columns.
---
Nitpick comments:
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md`:
- Around line 147-148: Two table rows referencing "KubeVirt PR `#16350`" use
"tier1 auto" while the rest use "tier1 automation"; update those rows (the
entries with titles `RoleAggregationStrategy configuration should create
ClusterRole without aggregate labels when RoleAggregationStrategy is Manual` and
`RoleAggregationStrategy configuration should remove aggregate labels from
existing ClusterRole when strategy changes to Manual`) to use the consistent
test type string "tier1 automation" (or vice versa if your project standard is
the shorter form) so the table uses a single convention throughout.
| | **Customer Use Cases** | [x] | * multi-tenant clusters|different namespaces are used to allow different workloads and prevent unallowed usage of workload that the tenant is not eligible to us| | ||
| | | | * Resources usage control|cluster admin wants to get a request to allow a specific user to create VMs and Storage| |
There was a problem hiding this comment.
Escape pipe characters in the “Customer Use Cases” table cell.
Line 28 and Line 29 use raw | inside a markdown table cell, which breaks column parsing. Use \| or split into <br> bullets to keep the table render intact.
🛠️ Proposed fix
-| **Customer Use Cases** | [x] | * multi-tenant clusters|different namespaces are used to allow different workloads and prevent unallowed usage of workload that the tenant is not eligible to us|
-| | | * Resources usage control|cluster admin wants to get a request to allow a specific user to create VMs and Storage|
+| **Customer Use Cases** | [x] | - multi-tenant clusters: different namespaces are used to allow different workloads and prevent unauthorized usage |
+| | | - resources usage control: cluster admin can approve requests for specific users to create VMs and storage |📝 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.
| | **Customer Use Cases** | [x] | * multi-tenant clusters|different namespaces are used to allow different workloads and prevent unallowed usage of workload that the tenant is not eligible to us| | |
| | | | * Resources usage control|cluster admin wants to get a request to allow a specific user to create VMs and Storage| | |
| | **Customer Use Cases** | [x] | - multi-tenant clusters: different namespaces are used to allow different workloads and prevent unauthorized usage | | |
| | | | - resources usage control: cluster admin can approve requests for specific users to create VMs and storage | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 28 - 29,
The markdown table under the "Customer Use Cases" cell contains raw pipe
characters that break table parsing; edit the table cell in stp.md (the row
beginning with "**Customer Use Cases**") and either escape each raw pipe as "\|"
or replace the inline pipes with HTML line breaks (e.g., use "<br>" and bullets)
so the two bullets "multi-tenant clusters|different namespaces..." and
"Resources usage control|cluster admin..." render as intended and do not split
columns.
| | | As an admin, I can add roleBinding kubevirt.io:admin to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:admin action | tier2 automation | P0 | | ||
| | | As an admin, I can add roleBinding kubevirt.io:edit to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:edit action | tier2 automation | P0 | | ||
| | | As an admin, I can add roleBinding kubevirt.io:view to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:view action | tier2 automation | P0 | |
There was a problem hiding this comment.
Fix typo: unprevileged → unprivileged.
This appears in multiple rows and should be corrected for clarity and searchability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` around lines 154 -
156, Fix the typo "unprevileged" to "unprivileged" in the table rows that
mention the roleBindings "kubevirt.io:admin", "kubevirt.io:edit", and
"kubevirt.io:view" so all occurrences in those test description cells are
corrected for spelling and searchability.
| | | As an admin, I can add roleBinding kubevirt.io:admin to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:admin action | tier2 automation | P0 | | ||
| | | As an admin, I can add roleBinding kubevirt.io:edit to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:edit action | tier2 automation | P0 | | ||
| | | As an admin, I can add roleBinding kubevirt.io:view to unprevileged user in a namespace with feature enabled| Verify unprivileged user can perform kubevirt.io:view action | tier2 automation | P0 | | ||
| | | As an admin, I can disable the feature via config in hyperconverged CR | Verify config persists once disabled and unprivileged user with admin role in a namespace can perform kubevirt:admin action | tier2 automation | P0 | |
There was a problem hiding this comment.
Use consistent role identifier format (kubevirt.io:admin).
Line 157 uses kubevirt:admin, while other rows use kubevirt.io:*. Standardize to avoid ambiguity in traceability text.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@stps/sig-iuo/CNV-63822-role-aggregation-opt-out/stp.md` at line 157, Replace
the inconsistent role identifier `kubevirt:admin` with the standardized
`kubevirt.io:admin` in the table row that currently reads "As an admin, I can
disable the feature via config in hyperconverged CR | Verify config persists
once disabled and unprivileged user with admin role in a namespace can perform
kubevirt:admin action" so the traceability text matches other entries using the
`kubevirt.io:*` format; update the row text where the role string appears
(search for "kubevirt:admin") to ensure consistent role identifier usage across
the document.
There was a problem hiding this comment.
do you think having this nesting under stps/sig-iuo/CNV-63822-role-aggregation-opt-out is needed or just have the stp under iuo with the feature name?
| **Testing Goals** | ||
| - [P0] Verify opt-out role aggregation can be enabled via hyperconvergeds.hco.kubevirt.io config | ||
| - [P0] Unprivileged users cannot access kubevirt resources without explicit RoleBinding when feature is enabled | ||
| - [P0] Unprivileged users cannot access Virtualization view in UI without explicit RoleBinding when feature is enabled |
There was a problem hiding this comment.
ui-related testing should be under a ui stp
| | Automation Testing | Ensures test cases are automated for continuous integration and regression coverage | Y | | | ||
| | Performance Testing | Validates feature performance meets requirements (latency, throughput, resource usage) | N/A | | | ||
| | Security Testing | Verifies security requirements, RBAC, authentication, authorization, and vulnerability scanning | Y | Feature is a security enhancement | | ||
| | Usability Testing | Validates user experience, UI/UX consistency, and accessibility requirements. Does the feature require UI? If so, ensure the UI aligns with the requirements | Y | unprivileged user should not see Virtualization view on UI with feature Enabled | |
There was a problem hiding this comment.
for the ui - the expected behavior is unclear to me
also, need to test what the ui provides specific functioality for edit users - this item it to make sure it is considered and you can add a link to the ui jira (once they have one)
| | Risk Category | Specific Risk for This Feature | Mitigation Strategy | Status | | ||
| |:---------------------|:--------------------------------------------------------|:--------------------------------------------------------|:-----------| | ||
| | Timeline/Schedule | KubeVirt PR #16350 not yet merged; blocks test implementation | Monitor PR status weekly; prepare test infrastructure in parallel | [x] Active | | ||
| | Test Coverage | Cannot exhaustively test all RBAC role combinations | Test critical paths (all 4 roles); focus on acceptance criteria | [ ] | |
There was a problem hiding this comment.
i may be missing something here - what actions are there for an edit role beside editing + cannot delete for example?
|
|
||
| ### **III. Test Scenarios & Traceability** | ||
|
|
||
| | Requirement ID | Requirement Summary | Test Scenario(s) | Test Type(s) | Priority | |
There was a problem hiding this comment.
for visibility - i am not famialir with the roles enough to answer what is needed; please check what is needed
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should keep aggregate labels when RoleAggregationStrategy is AggregateToDefault` | | tier1 automation | P0 | | ||
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should create ClusterRole without aggregate labels when RoleAggregationStrategy is Manual` | | tier1 auto | P0 | | ||
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual` | | tier1 auto | P0 | | ||
| | CNV-63822 | As an admin I can enable the feature via config in hyperconverged CR | Verify config persists once enabled | tier2 automation | P0 | |
There was a problem hiding this comment.
this is an implementation thing - focus on user stories not how you achive the functionality
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should create ClusterRole without aggregate labels when RoleAggregationStrategy is Manual` | | tier1 auto | P0 | | ||
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual` | | tier1 auto | P0 | | ||
| | CNV-63822 | As an admin I can enable the feature via config in hyperconverged CR | Verify config persists once enabled | tier2 automation | P0 | | ||
| | | As an unprivileged user with admin role on a namespace, I cannot navigate into the virtualization view on openshift UI | Verify unprivileged user cannot see the view | tier2 automation | P0 | |
| #### **5. Risks** | ||
|
|
||
| | Risk Category | Specific Risk for This Feature | Mitigation Strategy | Status | | ||
| |:---------------------|:--------------------------------------------------------|:--------------------------------------------------------|:-----------| |
There was a problem hiding this comment.
what about ui development and functioanlity - is there a risk? iiuc they were not in the loop
| | KubeVirt PR #16350 | `RoleAggregationStrategy configuration should remove aggregate labels from existing ClusterRole when strategy changes to Manual` | | tier1 auto | P0 | | ||
| | CNV-63822 | As an admin I can enable the feature via config in hyperconverged CR | Verify config persists once enabled | tier2 automation | P0 | | ||
| | | As an unprivileged user with admin role on a namespace, I cannot navigate into the virtualization view on openshift UI | Verify unprivileged user cannot see the view | tier2 automation | P0 | | ||
| | | As an unprivileged user with admin role on a namespace, I cannot perform kubevirt.io:admin actions with feature enabled | Verify ForbiddenError is returned | tier2 automation | P0 | |
There was a problem hiding this comment.
think about the requirement as the user story a pm tell his customers
e.g
as an admin i want to block unprivileged users from performing any <what is defined as admin actions?>
then
verify unprivileged user cannot .....
please update the table
8 participants
STP Metadata
VEP issue: kubevirt/enhancements#160
What this PR does
STP ready for QE team review while waiting for upstream merge
Special notes for your reviewer
Summary by CodeRabbit