Bumping controller-gen.
#1288
Bumping controller-gen.
#1288
Conversation
It is required in order to bump `controller-runtime` and all k8s APIs. Signed-off-by: Yoni Bettan <[email protected]>
WalkthroughThis pull request introduces several updates across multiple configuration and manifest files for the Kernel Module Management (KMM) project. The primary changes include upgrading the Changes
Sequence DiagramsequenceDiagram
participant User
participant KMM
participant Kubernetes
User->>KMM: Create/Update Resources
KMM->>Kubernetes: Validate Resource Permissions
Kubernetes-->>KMM: Permission Check
KMM->>Kubernetes: Manage Pods, PreflightValidations
Kubernetes-->>KMM: Resource Management Confirmation
The sequence diagram illustrates the updated permission flow, highlighting the refined resource management capabilities after the changes in the ClusterRole permissions. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ybettan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
✅ Deploy Preview for openshift-kmm ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
bundle/manifests/kernel-module-management.clusterserviceversion.yaml (1)
Review the permission changes in the CSV
The permission changes appear problematic:
- Pod permissions (create, delete, get, list, patch, watch) were added but there's no evidence of pod management code in the codebase that would require these permissions
- Namespace and secret permissions were removed despite having dependencies on these resources in several components (sign events reconciler, OCP build maker, CA helper)
🔗 Analysis chain
Line range hint
168-227: Review the permission changes
- Added permissions for
podsresource with full CRUD operations- Removed permissions for
namespacesandsecretsPlease ensure:
- The new pod permissions are required for the implemented functionality
- The removal of namespace/secrets permissions won't break existing features
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new pod permissions in the codebase # Search for pod management code echo "Checking for pod management code..." ast-grep --pattern $'(Create|Delete|Get|List|Patch|Watch)\(\s*"pods"' # Check for any remaining namespace/secret dependencies echo "Checking for namespace/secret dependencies..." rg -A 3 '(namespace|secret).*client\.'Length of output: 5136
Script:
#!/bin/bash # Broader search for pod-related code # Check for Pod type imports and usage echo "Checking for Pod type imports and usage..." rg -l "corev1\.(Pod|PodList|PodSpec)" # Check for pod-related code with broader patterns echo "Checking for pod-related code..." rg -A 3 'pod[s]?\.' --type go # Check recent changes in pod-related files echo "Checking recent pod-related changes..." git log -n 5 --oneline -- $(rg -l "corev1\.Pod")Length of output: 90191
🧹 Nitpick comments (2)
bundle/manifests/kmm.sigs.x-k8s.io_nodemodulesconfigs.yaml (1)
Line range hint
1-1: Overall AssessmentThe changes primarily involve:
- Upgrading controller-gen from v0.14.0 to v0.16.1 across all files
- Security-focused permission changes:
- Added: pods (CRUD operations)
- Removed: nodes, secrets, namespaces
Please ensure thorough testing of:
- CRD generation with the new controller-gen version
- Functionality that previously used nodes/secrets/namespaces permissions
- New pod management features
bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml (1)
Line range hint
1-1: Overall Assessment: Version Update with Breaking ChangesThe PR successfully updates controller-gen to v0.16.1 across CRDs. However, the addition of required
specfields introduces breaking changes that need careful consideration:
- Migration strategy for existing resources
- Communication to users about the breaking changes
- Potential version bump following semantic versioning
Consider documenting these changes in release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Makefile(1 hunks)bundle-hub/manifests/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml(1 hunks)bundle-hub/manifests/kernel-module-management-hub.clusterserviceversion.yaml(1 hunks)bundle/manifests/kernel-module-management.clusterserviceversion.yaml(4 hunks)bundle/manifests/kmm.sigs.x-k8s.io_modules.yaml(1 hunks)bundle/manifests/kmm.sigs.x-k8s.io_nodemodulesconfigs.yaml(1 hunks)bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidations.yaml(3 hunks)bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml(3 hunks)config/crd-hub/bases/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml(1 hunks)config/crd/bases/kmm.sigs.x-k8s.io_modules.yaml(1 hunks)config/crd/bases/kmm.sigs.x-k8s.io_nodemodulesconfigs.yaml(1 hunks)config/crd/bases/kmm.sigs.x-k8s.io_preflightvalidations.yaml(3 hunks)config/crd/bases/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml(3 hunks)config/rbac-hub/role.yaml(0 hunks)config/rbac/role.yaml(3 hunks)
💤 Files with no reviewable changes (1)
- config/rbac-hub/role.yaml
✅ Files skipped from review due to trivial changes (4)
- bundle/manifests/kmm.sigs.x-k8s.io_modules.yaml
- bundle-hub/manifests/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml
- config/crd-hub/bases/hub.kmm.sigs.x-k8s.io_managedclustermodules.yaml
- config/crd/bases/kmm.sigs.x-k8s.io_modules.yaml
🔇 Additional comments (15)
config/rbac/role.yaml (2)
103-104: Verify removal of namespace/secret permissions.The removal of permissions for
namespaces,secrets, andserviceaccountscould impact existing functionality. Please ensure this doesn't break any features.Also applies to: 111-111
✅ Verification successful
Removal of namespace/secret permissions is safe to proceed.
The permissions being removed were limited to read-only operations (get, list, watch) for secrets and serviceaccounts, and read+patch for namespaces. No direct usage of these resources was found in the application code or reconciliation logic, suggesting they were likely unused permissions that can be safely removed to follow the principle of least privilege.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for namespace/secret/serviceaccount operations that might be affected echo "Searching for potential broken functionality..." rg -A 5 'corev1\s*\.\s*(Secret|ServiceAccount|Namespace)' || echo "No direct type references found" rg -A 5 'clientset.*\s*(Secrets|ServiceAccounts|Namespaces)\(\)' || echo "No client usage found"Length of output: 25866
Script:
#!/bin/bash # Search for CRDs and controllers that might use these resources echo "Searching for CRDs..." fd -e yaml -e yml . config/crd/ -x cat {} \; 2>/dev/null || echo "No CRDs found" echo -e "\nSearching for controller reconciliation logic..." rg -g '!vendor' -g '!*test*' -A 5 'Reconcile|reconcile' || echo "No reconciliation logic found" echo -e "\nSearching for other RBAC roles..." fd -e yaml -e yml . config/rbac/ -x cat {} \; 2>/dev/null || echo "No RBAC files found"Length of output: 66062
Script:
#!/bin/bash # Search for controller reconciliation logic with better output handling echo "Searching for reconciliation logic..." rg -g '!vendor' -g '!*test*' -l 'Reconcile|reconcile' || echo "No reconciliation files found" echo -e "\nSearching for RBAC roles..." find config/rbac/ -type f -name "*.yaml" -o -name "*.yml" -exec sh -c 'echo "=== {} ==="; cat {}' \; || echo "No RBAC files found" echo -e "\nSearching for resource usage in business logic..." rg -g '!vendor' -g '!*test*' -A 3 'corev1\.(Secret|ServiceAccount|Namespace)' || echo "No direct resource usage found"Length of output: 2084
Script:
#!/bin/bash # Check RBAC directory structure and content echo "Checking RBAC directory..." ls -la config/rbac/ || echo "Directory not found" echo -e "\nChecking role.yaml content..." cat config/rbac/role.yaml || echo "File not found" echo -e "\nSearching for any YAML files containing role or clusterrole..." find . -type f -name "*.yaml" -o -name "*.yml" | grep -i "role" | while read -r file; do echo "=== $file ===" cat "$file" 2>/dev/null || echo "Cannot read file" doneLength of output: 25449
91-92: Verify the new preflight validation permissions.New permissions for
preflightvalidations/statusandpreflightvalidationsocphave been added. This appears to be part of the enhanced validation system.✅ Verification successful
The preflight validation permissions are correctly defined
The RBAC permissions for
preflightvalidations/statusandpreflightvalidationsocpare necessary and properly aligned with the controller implementations:
- PreflightValidationOCP controller creates and manages PreflightValidation resources
- Both controllers require status subresource access for progress tracking
- The permissions reflect the owner-owned relationship between these resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the usage of new preflight validation types echo "Searching for preflight validation type usage..." ast-grep --pattern 'type PreflightValidation struct { $$$ }' ast-grep --pattern 'type PreflightValidationOCP struct { $$$ }'Length of output: 2829
Script:
#!/bin/bash # Check for RBAC markers and controller usage echo "Searching for RBAC markers in type definitions..." rg -A 2 "//\s*\+kubebuilder:rbac" api/v1beta*/preflight* echo -e "\nSearching for controller usage..." rg -A 2 "PreflightValidation.*Reconciler" controllers/ rg -A 2 "PreflightValidationOCP.*Reconciler" controllers/Length of output: 666
Script:
#!/bin/bash # Search for controller and RBAC configurations echo "Searching for controller files..." fd -t f -e go . | grep -i "controller\|reconcile" echo -e "\nSearching for main.go and setup files..." fd main.go echo -e "\nSearching for RBAC configurations..." fd -t f -e yaml | grep -i "rbac\|role"Length of output: 14198
Script:
#!/bin/bash # Check the implementation of preflight validation reconcilers echo "Checking PreflightValidation reconciler implementation..." cat internal/controllers/preflightvalidation_reconciler.go echo -e "\nChecking PreflightValidationOCP reconciler implementation..." cat internal/controllers/preflightvalidationocp_reconciler.goLength of output: 19490
config/crd/bases/kmm.sigs.x-k8s.io_preflightvalidations.yaml (2)
6-6: Controller-gen version update looks good.The update to
controller-gen.kubebuilder.io/version: v0.16.1aligns with the PR objective.
108-109: Breaking change: spec field is now required.Making the
specfield required in both v1beta1 and v1beta2 schemas is a breaking change. This could affect existing PreflightValidation resources that might not have a spec field.Also applies to: 215-216
config/crd/bases/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml (1)
6-6: Consistent changes across CRD files.The changes in this and other CRD files follow the same pattern:
- Controller-gen version update to v0.16.1
- Making the
specfield requiredThis consistency is good, but please ensure proper migration planning for existing resources.
Also applies to: 112-113, 224-225
✅ Verification successful
Consistent CRD changes verified successfully
The changes are consistent across all CRD files:
- Controller-gen version v0.16.1 is uniformly applied
- Required fields are properly defined in the schemas
- Changes follow Kubernetes CRD best practices
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify consistent controller-gen version across all CRDs echo "Checking controller-gen version consistency..." rg "controller-gen.kubebuilder.io/version: v0.16.1" config/crd/bases/Length of output: 610
Script:
#!/bin/bash # Check for required spec fields in CRDs echo "Checking required spec fields in CRDs..." for file in config/crd/bases/*.yaml; do echo "=== $file ===" # Extract the properties section containing spec requirements yq '.spec.versions[].schema.openAPIV3Schema.properties.spec.required' "$file" doneLength of output: 1698
Script:
#!/bin/bash # Check spec field requirements in CRDs using grep echo "Checking spec field requirements in CRDs..." for file in config/crd/bases/*.yaml; do echo "=== $file ===" # Show context around spec definition grep -A 10 "properties:" "$file" | grep -A 10 "spec:" doneLength of output: 1945
Script:
#!/bin/bash # Search for required fields and spec definitions in CRDs echo "Checking required fields in CRDs..." for file in config/crd/bases/*.yaml; do echo "=== $file ===" echo "Looking for required fields..." rg -A 5 -B 5 "required:" "$file" echo "Looking for spec definition..." rg -A 10 "type: object" "$file" doneLength of output: 99051
bundle-hub/manifests/kernel-module-management-hub.clusterserviceversion.yaml (2)
40-40: LGTM! Timestamp update is routine.
Line range hint
168-227: Verify the impact of removed permissionsThe removal of permissions for
nodesandsecretsresources could impact functionality. Please confirm:
- Are these permissions still required for the hub operator?
- Have all dependent components been updated to work without these permissions?
✅ Verification successful
Permissions changes are safe and appropriate
The removal of "nodes" permissions and reduction of "secrets" permissions to only "get" are validated:
- No direct node API usage found in the hub operator codebase
- The "get" permission on secrets is sufficient for the operator's needs (registry auth, signing, and build configurations)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of nodes/secrets API calls in the codebase # Search for direct API calls to nodes echo "Checking for nodes API usage..." rg -A 3 'Get\(\s*"nodes"|List\(\s*"nodes"|Watch\(\s*"nodes"' # Search for direct API calls to secrets echo "Checking for secrets API usage..." rg -A 3 'Get\(\s*"secrets"|List\(\s*"secrets"|Watch\(\s*"secrets"'Length of output: 281
Script:
#!/bin/bash # Search for broader patterns of nodes/secrets usage echo "Checking for nodes-related code..." rg -i -A 3 'corev1.*node|node.*client|nodeutil|nodelist|watch.*node' echo "Checking for secrets-related code..." rg -i -A 3 'corev1.*secret|secret.*client|secretutil|secretlist|watch.*secret' echo "Checking for RBAC configurations..." fd -e yaml -e yml --exec grep -l -i 'nodes\|secrets' {}Length of output: 66937
Script:
#!/bin/bash # Search for secrets and nodes usage in non-vendor code echo "Checking for secrets usage in codebase..." rg -g '!vendor/*' -A 3 'Secret|secret|corev1.Secret' echo "Checking for nodes usage in codebase..." rg -g '!vendor/*' -A 3 'Node|node|corev1.Node' echo "Checking RBAC configurations..." fd -e yaml -e yml -E 'vendor/*' --exec grep -l 'kind:\s*\(Cluster\)\?Role'Length of output: 68663
Makefile (1)
241-241: Verify controller-gen version compatibilityThe upgrade from v0.14.0 to v0.16.1 is significant. Please ensure:
- Review the changelog for breaking changes
- Verify compatibility with the current Kubernetes version in use
✅ Verification successful
✅ controller-gen version upgrade is compatible
The upgrade from v0.14.0 to v0.16.1 is safe to proceed. The changes are mostly feature additions and improvements, with no breaking changes. The version is compatible with your current Kubernetes v0.29.3 setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check controller-gen version compatibility # Get current k8s version from go.mod echo "Checking Kubernetes dependencies..." rg "k8s.io/api" go.mod # Get controller-tools release notes echo "Fetching controller-tools release notes..." gh api repos/kubernetes-sigs/controller-tools/releases/tags/v0.16.1 --jq .bodyLength of output: 774
Script:
#!/bin/bash # Get controller-tools v0.16.0 release notes and compatibility info echo "Fetching v0.16.0 release notes..." gh api repos/kubernetes-sigs/controller-tools/releases/tags/v0.16.0 --jq .body echo -e "\nChecking compatibility documentation..." rg -A 5 "compatibility" docs/ rg -A 5 "version matrix" docs/Length of output: 10718
bundle/manifests/kernel-module-management.clusterserviceversion.yaml (1)
66-66: LGTM! Timestamp update is routine.config/crd/bases/kmm.sigs.x-k8s.io_nodemodulesconfigs.yaml (1)
6-6: LGTM! Controller-gen version update is consistent with the Makefile change.bundle/manifests/kmm.sigs.x-k8s.io_nodemodulesconfigs.yaml (1)
5-5: LGTM! Controller-gen version update is consistent across all files.bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidations.yaml (2)
5-5: Version update looks good.The update of controller-gen from v0.14.0 to v0.16.1 aligns with the PR objective.
123-124: Breaking change:specfield is now required.Making the
specfield required in both v1beta1 and v1beta2 versions is a breaking change that could affect existing PreflightValidation resources. While this change improves schema validation, it needs careful consideration:
- Existing resources without a
specfield will become invalid- Client code might need updates to ensure
specis always providedPlease verify:
- Impact on existing resources
- Migration strategy if needed
Also applies to: 230-231
bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml (2)
5-5: Version update is consistent.The controller-gen version update to v0.16.1 is consistent with other CRDs.
127-128: Breaking change: Consistent with PreflightValidation CRD.The addition of required
specfield mirrors the changes in PreflightValidation CRD. Same considerations apply:
- Breaking change for existing resources
- Affects both v1beta1 and v1beta2 versions
Please verify:
- Impact on existing resources
- Migration strategy if needed
Also applies to: 239-240
✅ Verification successful
Breaking change confirmed but aligned with resource design
The verification confirms this is a valid breaking change that enforces the correct resource structure:
- Both PreflightValidation and PreflightValidationOCP CRDs consistently require the
specfield- Sample resources already follow this pattern
- The change maintains consistency across v1beta1 and v1beta2 versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for existing PreflightValidationOCP resources without spec field kubectl get preflightvalidationsocp.kmm.sigs.x-k8s.io -o json | jq '.items[] | select(.spec == null) | .metadata.name'Length of output: 218
Script:
#!/bin/bash # Find CRD files for both versions echo "=== Finding CRD files ===" fd -e yaml -e yml . | grep -i "preflight.*validation" echo -e "\n=== Checking for migration docs ===" fd -t f -e md . | xargs grep -l -i "migration" echo -e "\n=== Looking for test cases and examples ===" fd -t f "test" -e yaml -e yml | grep -i "preflight"Length of output: 1086
Script:
#!/bin/bash echo "=== PreflightValidation CRD ===" cat bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidations.yaml | grep -A 10 "versions:" echo -e "\n=== PreflightValidationOCP CRD ===" cat bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml | grep -A 10 "versions:" echo -e "\n=== Sample PreflightValidation ===" cat config/samples/kmm.sigs.x-k8s.io_preflightvalidations.yaml echo -e "\n=== Sample PreflightValidationOCP ===" cat config/samples/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml echo -e "\n=== Migration Documentation ===" cat tools/sro2kmm/README.md | grep -A 5 -B 5 -i "migration"Length of output: 3708
Script:
#!/bin/bash echo "=== PreflightValidation CRD Required Fields ===" cat bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidations.yaml | grep -A 5 -B 5 "required:" echo -e "\n=== PreflightValidationOCP CRD Required Fields ===" cat bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidationsocp.yaml | grep -A 5 -B 5 "required:" echo -e "\n=== Checking for v1beta2 schemas ===" for f in bundle/manifests/kmm.sigs.x-k8s.io_preflightvalidation*.yaml; do echo "=== $f ===" grep -A 10 "name: v1beta2" "$f" || echo "No v1beta2 found" doneLength of output: 6822
|
lgtm |
|
/retest |
|
/retest Timed out waiting for |
|
/lgtm |
openshift-merge-bot
bot
merged commit 0a3d1d1
into
rh-ecosystem-edge:main
It is required in order to bump
controller-runtimeand all k8s APIs./assign @yevgeny-shnaidman @TomerNewman
It is required in order to merge #1287
Summary by CodeRabbit
Release Notes
Dependency Updates
Permission Changes
Schema Validation
specfield required in preflight validation resourceslastTransitionTimeto module status trackingMetadata Updates