OCPCLOUD-3005: Initial implementation of CompatibilityRequirement reconciler and webhook #458
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
📝 WalkthroughWalkthroughThis pull request introduces a new CRD compatibility checker controller for OpenShift that validates CustomResourceDefinitions against compatibility requirements via webhook-based admission control. It includes controller reconciliation logic, webhook validation, status management, supporting utilities, and comprehensive test coverage. Dependency versions are updated across multiple go.mod files to align with Kubernetes 1.34.1. Changes
Sequence DiagramsequenceDiagram
participant Client as Webhook Client
participant Webhook as ValidatingWebhook
participant Reconciler as CompatibilityReconciler
participant Checker as CRDChecker
participant Status as StatusWriter
Client->>Webhook: POST /validate (CompatibilityRequirement)
Webhook->>Webhook: Parse embedded CRD YAML
Webhook->>Webhook: Validate CRD structure
Webhook->>Webhook: Build/Validate WebhookConfig
Webhook-->>Client: admission response (allow/deny)
Note over Reconciler: Reconciliation triggered
Reconciler->>Reconciler: Load CompatibilityRequirement
Reconciler->>Reconciler: Set finalizer
Reconciler->>Reconciler: Parse CRD from requirement
Reconciler->>Reconciler: Fetch current live CRD
Reconciler->>Checker: CheckCompatibilityRequirement
Checker->>Checker: Build validation registry
Checker->>Checker: Run compatibility checks
Checker-->>Reconciler: errors, warnings
Reconciler->>Status: Write status conditions
Status->>Status: Set Admitted/Compatible/Progressing
Status-->>Reconciler: status patched
Reconciler-->>Reconciler: Return result/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…ompatibilityRequirement
…alidation webhook
JoelSpeed
force-pushed
the
bootstrap-compatibility-reconciler
branch
from
1ce9e6e to
582b1d7
Compare
JoelSpeed
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@JoelSpeed: This pull request references OCPCLOUD-3005 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
JoelSpeed
force-pushed
the
bootstrap-compatibility-reconciler
branch
from
582b1d7 to
1015148
Compare
|
@JoelSpeed: This pull request references OCPCLOUD-3005 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
go.mod (1)
60-71:⚠️ Potential issue | 🟡 MinorReview k8s.io/kubernetes version; v1.34.4 is available.
The versions are all confirmed releases (k8s.io/kubernetes v1.34.1 released Sept 10, 2025; sigs.k8s.io/crdify v0.5.0 released Aug 25, 2025). However, note that v1.34.4 was released on February 10, 2026. Consider updating to the latest patch unless v1.34.1 is pinned intentionally for this release train.
🤖 Fix all issues with AI agents
In `@manifests-gen/go.mod`:
- Line 21: The go.mod entry "go.yaml.in/yaml/v2 v2.4.3" is invalid; replace or
correct it to use the canonical module path and an existing tag (e.g., change
the module to "gopkg.in/yaml.v2" and the version to "v2.4.0"), or if a different
yaml library was intended, update the module path and version to the correct
upstream name and tag so the go.mod line for the yaml dependency is valid.
In `@pkg/controllers/crdcompatibility/finalizer.go`:
- Around line 39-42: clearFinalizer currently omits the finalizers field in the
server-side apply patch so SSA treats it as “do not modify” and the finalizer is
never removed; update the apply config passed to writeFinalizer (the
CompatibilityRequirement(...) used inside clearFinalizer) to explicitly set an
empty finalizers array (e.g. call WithFinalizers() with no args or
WithFinalizers([]string{})) so the SSA patch clears the finalizer, keeping
ForceOwnership behavior intact and allowing reconcileDelete to complete.
In `@pkg/controllers/crdcompatibility/status.go`:
- Around line 36-41: The log message in reconcileState.writeStatus is incorrect:
the condition (obj.DeletionTimestamp.IsZero() &&
!slices.Contains(obj.Finalizers, finalizerName)) detects an object that is NOT
being deleted but is missing the finalizer, so update the log emitted via
logf.FromContext(ctx).Info to accurately state that status write is being
skipped because the object lacks the finalizer (reference writeStatus,
obj.DeletionTimestamp.IsZero(), slices.Contains(..., finalizerName), and
logf.FromContext(ctx).Info) so the message matches the actual condition.
In `@pkg/crdchecker/crdchecker_test.go`:
- Around line 82-93: The test assertion is inverted and uses the wrong matcher
type: replace Expect(errors).NotTo(ConsistOf(MatchError("removed field :
v1.^.spec"))) with an assertion that checks the string slice contains the
expected message; e.g. use Expect(errors).To(ContainElement("removed field :
v1.^.spec")) or Expect(errors).To(ContainElement(MatchRegexp(`removed field :
v1\.\^\.spec`))). Update the same pattern in the other failing test (the similar
assertion around the second failing block) and locate the checks in the test
that call runTest and use getCRDVersion to modify the CRD schema.
- Around line 38-43: getCRDVersion currently returns &v from a range loop which
yields a copy; change it to iterate with index and return a pointer to the
actual slice element (e.g., &crd.Spec.Versions[i]) when v.Name == version,
otherwise return nil; update function getCRDVersion to use for i := range
crd.Spec.Versions { v := &crd.Spec.Versions[i]; if v.Name == version { return v
} } to ensure callers mutate the real element.
In `@pkg/util/sync.go`:
- Around line 60-61: The loop compares condition.Type to *conditionAC.Type and
can panic if conditionAC or conditionAC.Type is nil; add a guard before
dereferencing: ensure conditionAC != nil and conditionAC.Type != nil (or handle
nil Type explicitly) before using *conditionAC.Type, mirroring the nil checks
used in matchingCondition so the comparison is safe and consistent with the
existing defensive checks.
🧹 Nitpick comments (12)
pkg/util/norequeue.go (2)
35-39: Consider handling nil error to prevent potential panic.If
erris nil, callingError()on the returned value will panic due to nil dereference on the embedded error interface.🛡️ Optional defensive fix
func NoRequeueError(err error, reason string) error { + if err == nil { + return nil + } return &noRequeueError{err, reason} }
54-62: Unexported return type limits external usability.
AsNoRequeueErroris exported but returns*noRequeueError(unexported). External callers cannot access theReasonfield without reflection. If this is intended for internal use only, this is fine. Otherwise, consider exporting the type or adding a getter method.♻️ Option 1: Export the type
-type noRequeueError struct { +type NoRequeueError struct { error Reason string }Then update the return type of
AsNoRequeueErroraccordingly.♻️ Option 2: Add a getter method
func (e *noRequeueError) GetReason() string { return e.Reason }go.mod (1)
340-344: Align controller-manager indirect version with other 0.34.1 pins.The replace block pins
k8s.io/controller-managertov0.34.1(line 24), but the indirect require showsv0.32.1(line 341). While the replace directive will override this, aligning the require list prevents confusion and maintains consistency with other k8s.io components atv0.34.1.Proposed alignment
- k8s.io/controller-manager v0.32.1 // indirect + k8s.io/controller-manager v0.34.1 // indirectpkg/util/map.go (1)
18-22: Consider distinguishing nil input from empty input behavior if this function may be JSON-serialized. Go's code review guidelines prefer nil slices as empty values, so returning nil forlen(source) == 0is idiomatic. However, if the caller JSON-marshals the result and needs[]instead ofnull, explicitly checksource == nilto preserve that distinction.♻️ Suggested change (if JSON serialization is a concern)
func SliceMap[A, B any](source []A, fn func(A) B) []B { - if len(source) == 0 { + if source == nil { return nil } result := make([]B, len(source)) for i, a := range source {pkg/crdchecker/servedversionremoval.go (1)
80-85: Deterministic ordering for removed version list.
UnsortedList()can make error strings non-deterministic; sorting avoids flaky comparisons.♻️ Suggested fix
import ( "errors" "fmt" + "sort" "strings" @@ removedVersions := oldServedVersions.Difference(newServedVersions) @@ var err error if len(removedVersions) > 0 { - err = fmt.Errorf("%w : %v", ErrRemovedServedVersions, strings.Join(removedVersions.UnsortedList(), ", ")) + removed := removedVersions.UnsortedList() + sort.Strings(removed) + err = fmt.Errorf("%w : %s", ErrRemovedServedVersions, strings.Join(removed, ", ")) }pkg/controllers/crdcompatibility/index/crdcompatibilityrequirement.go (1)
33-40: Consider handling empty CRDName in the indexer.When
Status.CRDNameis empty, the indexer returns[]string{""}, which indexes the object under an empty string key. This could cause unexpected matches when querying for objects with no CRD name set. Consider returning an empty slice to avoid indexing objects without a CRD name.♻️ Suggested improvement
func CRDByName(obj client.Object) []string { requirement, ok := obj.(*apiextensionsv1alpha1.CompatibilityRequirement) if !ok { panic(fmt.Sprintf("Expected a CompatibilityRequirement but got a %T", obj)) } + if requirement.Status.CRDName == "" { + return nil + } + return []string{requirement.Status.CRDName} }pkg/controllers/crdcompatibility/reconcile.go (2)
154-162: Sequential dependency handled via nil guards, but consider explicit sequencing for clarity.Using
errors.Joinruns all three functions regardless of prior failures. While the nil guards infetchCurrentCRDandcheckCompatibilityRequirementprevent crashes, this pattern obscures the intended sequential dependency. Consider explicit sequencing if the functions should short-circuit on failure.The current approach works due to nil guards, but explicit sequencing would make the dependencies clearer:
♻️ Alternative explicit sequencing
- err := errors.Join( - r.parseCompatibilityCRD(obj), - r.fetchCurrentCRD(ctx, logger), - r.checkCompatibilityRequirement(), - ) - - if err != nil { - return ctrl.Result{}, err - } + if err := r.parseCompatibilityCRD(obj); err != nil { + return ctrl.Result{}, err + } + + if err := r.fetchCurrentCRD(ctx, logger); err != nil { + return ctrl.Result{}, err + } + + if err := r.checkCompatibilityRequirement(); err != nil { + return ctrl.Result{}, err + }
114-120: Remove unnecessaryelseafterreturn.The
elseblock after thereturn nilstatement on line 116 is unnecessary and can be simplified.♻️ Suggested simplification
if err := r.client.Get(ctx, types.NamespacedName{Name: crdName}, currentCRD); err != nil { if apierrors.IsNotFound(err) { log.Info("CRD not found", "crdRef", crdName) return nil - } else { - return fmt.Errorf("failed to fetch CRD %s: %w", crdName, err) } + return fmt.Errorf("failed to fetch CRD %s: %w", crdName, err) }pkg/controllers/crdcompatibility/reconcile_test.go (1)
152-159: Consider extractingaddPropertyas a reusable test helper.The
addPropertyhelper is defined within the nested Context but could be useful in other tests. Consider moving it to the test helpers package if CRD schema modification is needed elsewhere.pkg/controllers/crdcompatibility/crdcompatibility_webhook.go (1)
66-69: Unnecessary double pointer inyaml.Unmarshal.On line 66,
compatibilityCRDis declared as*apiextensionsv1.CustomResourceDefinition, then line 67 passes&compatibilityCRD(a**CustomResourceDefinition) toyaml.Unmarshal. While this works because the YAML library handles dereferencing, passing the pointer directly is more idiomatic.♻️ Suggested simplification
// Parse the CRD in compatibilityCRD into a CRD object. - compatibilityCRD := &apiextensionsv1.CustomResourceDefinition{} - if err := yaml.Unmarshal([]byte(compatibilityRequirement.Spec.CompatibilitySchema.CustomResourceDefinition.Data), &compatibilityCRD); err != nil { + compatibilityCRD := &apiextensionsv1.CustomResourceDefinition{} + if err := yaml.Unmarshal([]byte(compatibilityRequirement.Spec.CompatibilitySchema.CustomResourceDefinition.Data), compatibilityCRD); err != nil { return nil, fmt.Errorf("%w: %w", errInvalidCompatibilityCRD, err) }pkg/controllers/crdcompatibility/suite_test.go (1)
257-259: Consider logging unexpected Kind values for debugging.The default case in the switch silently ignores unexpected webhook kinds. While this may be intentional for flexibility, logging a debug message could help diagnose issues when new webhook types are added to the manifest.
♻️ Suggested improvement
// Ignore unexpected kinds default: + // Optionally log for debugging: + // GinkgoLogr.Info("Ignoring unexpected kind in webhook manifest", "kind", generic.Kind) }pkg/controllers/crdcompatibility/controller.go (1)
96-120: Consider returning a requeue to handle transient list failures.When
Listfails (line 105-108), the function logs and returnsnil, meaning the CRD change won't trigger any reconciliations. While this is a common pattern and the next CRD update will retry, transient failures could delay reconciliation indefinitely if the CRD doesn't change again.This is acceptable for now since requirements are also reconciled on their own events, but worth noting for future consideration if tighter consistency guarantees are needed.
| github.com/openshift/api v0.0.0-20260205045418-81371d13d1fc | ||
| github.com/openshift/cluster-capi-operator v0.0.0-00010101000000-000000000000 | ||
| k8s.io/utils v0.0.0-20250820121507-0af2bda4dd1d | ||
| k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd manifests-gen && head -80 go.mod | tail -60Repository: openshift/cluster-capi-operator
Length of output: 2947
🏁 Script executed:
#!/bin/bash
# For k8s.io/utils - check if the commit exists
echo "=== Checking k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 ==="
# Extract commit hash from pseudo-version (last 12 chars)
curl -s "https://api.github.com/repos/kubernetes/utils/commits/bc988d571ff4" | jq -r '.sha // .message' | head -1
echo ""
echo "=== Checking k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 ==="
curl -s "https://api.github.com/repos/kubernetes/kube-openapi/commits/589584f1c912" | jq -r '.sha // .message' | head -1
echo ""
echo "=== Checking sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 ==="
curl -s "https://api.github.com/repos/kubernetes-sigs/json/commits/2d320260d730" | jq -r '.sha // .message' | head -1
echo ""
echo "=== Checking gopkg.in/evanphx/json-patch.v4 v4.13.0 ==="
curl -s "https://api.github.com/repos/evanphx/json-patch/releases/tags/v4.13.0" | jq -r '.tag_name // .message' | head -1
echo ""
echo "=== Checking go.yaml.in/yaml/v2 v2.4.3 ==="
curl -s "https://api.github.com/repos/go-yaml/yaml/releases/tags/v2.4.3" | jq -r '.tag_name // .message' | head -1Repository: openshift/cluster-capi-operator
Length of output: 1213
🌐 Web query:
k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 kubernetes go module
💡 Result:
k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 is a Go pseudo-version of the k8s.io/utils module (repo: github.com/kubernetes/utils) that pins the dependency to commit bc988d571ff4 dated Oct 2, 2025. [1][2]
To use it:
go get k8s.io/utils@v0.0.0-20251002143259-bc988d571ff4
# or equivalently
go get k8s.io/utils@bc988d571ff4On pkg.go.dev, this version appears in the module’s version list; newer pseudo-versions also exist (e.g., v0.0.0-20260108192941-914a6e750570 dated Jan 8, 2026). [1][3]
Sources: [1] [2] [3]
🌐 Web query:
k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 kubernetes release
💡 Result:
That k8s.io/kube-openapi pseudo-version (v0.0.0-20250910181357-589584f1c912) corresponds to the Kubernetes v1.35.0 release line (i.e., it’s used by Kubernetes 1.35.x components). This shows up in the SBOM for kube-controller-manager v1.35.0, which lists k8s.io/kube-openapi at exactly that version. [1]
Kubernetes v1.35.0 release date: 2025-12-17. [2]
Sources: [1] [2]
🌐 Web query:
sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 kubernetes-sigs
💡 Result:
sigs.k8s.io/json is a Go module hosted in the Kubernetes SIGs GitHub org at github.com/kubernetes-sigs/json (the import path sigs.k8s.io/json resolves to that repo). [1]
v0.0.0-20250730193827-2d320260d730 is a Go pseudo-version, meaning it points to a specific commit (2d320260d730) with the embedded timestamp 2025-07-30 19:38:27 UTC. The module version is shown as published on July 30, 2025. [1][2]
The library provides JSON decoding helpers (case-sensitive key handling, integer-preserving decoding, and stricter decoding options) on top of Go’s encoding/json. [2][3]
🌐 Web query:
gopkg.in/evanphx/json-patch.v4 v4.13.0 release
💡 Result:
gopkg.in/evanphx/json-patch.v4 v4.13.0 is the latest v4 tag, published Jan 28, 2025. [1][2]
Release note / main change: Remove github.com/pkg/errors dependency. [3]
Install: go get gopkg.in/evanphx/json-patch.v4@v4.13.0 [2]
Sources:
[1] GitHub tags page (shows v4.13.0 date) (github.com)
[2] pkg.go.dev module page (shows v4.13.0 “Published: Jan 28, 2025”) (pkg.go.dev)
[3] GitHub release v4.13.0 (release note text) (github.com)
Citations:
- 1: https://github.com/evanphx/json-patch/tags
- 2: https://pkg.go.dev/gopkg.in/evanphx/json-patch.v4?utm_source=openai
- 3: https://github.com/evanphx/json-patch/releases/tag/v4.13.0
🌐 Web query:
go.yaml.in/yaml/v2 v2.4.3 go-yaml package
💡 Result:
go.yaml.in/yaml/v2 doesn’t appear to be the canonical import path for the Go “go-yaml” library. The widely used go-yaml/yaml package uses:
- v2 import path:
gopkg.in/yaml.v2(repo:github.com/go-yaml/yaml) [1] - Latest tagged v2 release: v2.4.0 (tagged Nov 17, 2020) — there is no v2.4.3 tag in the upstream repository [2][3]
- Latest v3 tag shown: v3.0.1 (May 27, 2022) [2]
Pin to the latest upstream v2 like this:
go get gopkg.in/yaml.v2@v2.4.0If you meant a different fork/module that really has v2.4.3, paste the module path (as it appears in go.mod) and I’ll look up that specific artifact.
Fix or justify go.yaml.in/yaml/v2 v2.4.3—version does not exist.
Four of the five bumped dependencies are valid:
- ✅ k8s.io/utils v0.0.0-20251002143259-bc988d571ff4 (pseudo-version, Oct 2, 2025)
- ✅ k8s.io/kube-openapi v0.0.0-20250910181357-589584f1c912 (used in Kubernetes v1.35.0)
- ✅ sigs.k8s.io/json v0.0.0-20250730193827-2d320260d730 (pseudo-version, Jul 30, 2025)
- ✅ gopkg.in/evanphx/json-patch.v4 v4.13.0 (released Jan 28, 2025)
However, go.yaml.in/yaml/v2 v2.4.3 is invalid. The canonical go-yaml package uses import path gopkg.in/yaml.v2, and the latest v2 release is v2.4.0 (Nov 2020)—no v2.4.3 tag exists. Verify the intended module path and version, or correct to gopkg.in/yaml.v2@v2.4.0.
🤖 Prompt for AI Agents
In `@manifests-gen/go.mod` at line 21, The go.mod entry "go.yaml.in/yaml/v2
v2.4.3" is invalid; replace or correct it to use the canonical module path and
an existing tag (e.g., change the module to "gopkg.in/yaml.v2" and the version
to "v2.4.0"), or if a different yaml library was intended, update the module
path and version to the correct upstream name and tag so the go.mod line for the
yaml dependency is valid.
| func clearFinalizer(ctx context.Context, cl client.Client, obj *apiextensionsv1alpha1.CompatibilityRequirement) error { | ||
| return writeFinalizer(ctx, cl, obj, apiextensionsapplyconfig.CompatibilityRequirement(obj.GetName()). | ||
| WithUID(obj.GetUID())) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual implementation of clearFinalizer and related code
cat -n pkg/controllers/crdcompatibility/finalizer.go | head -60Repository: openshift/cluster-capi-operator
Length of output: 2341
🏁 Script executed:
# Find where WithFinalizers is defined
fd -e go | xargs grep -l "func.*WithFinalizers" | head -5Repository: openshift/cluster-capi-operator
Length of output: 487
🏁 Script executed:
# Search for how apply configs handle finalizers in this codebase
rg "WithFinalizers" -A 3 -B 3 --type goRepository: openshift/cluster-capi-operator
Length of output: 50388
🏁 Script executed:
# Check the writeFinalizer function to understand the full context
rg "writeFinalizer" -A 10 -B 3 --type goRepository: openshift/cluster-capi-operator
Length of output: 1945
🏁 Script executed:
# Look for tests of the finalizer functions
fd -e go | xargs grep -l "clearFinalizer\|setFinalizer" | head -10Repository: openshift/cluster-capi-operator
Length of output: 166
🏁 Script executed:
# Check if there are any tests in the crdcompatibility package
find pkg/controllers/crdcompatibility -name "*test*" -type fRepository: openshift/cluster-capi-operator
Length of output: 270
🏁 Script executed:
# Look for documentation or comments about SSA and finalizer removal
rg "ApplyConfigPatch|ForceOwnership" -B 5 -A 5 --type go | head -100Repository: openshift/cluster-capi-operator
Length of output: 5732
🏁 Script executed:
# Search for how to properly remove finalizers in the codebase
rg "finalizer" -i --type go -A 3 -B 3 | grep -E "(clear|remove|delete)" -A 3 -B 3 | head -50Repository: openshift/cluster-capi-operator
Length of output: 5215
🏁 Script executed:
# Look at the test files for finalizer testing
cat -n pkg/controllers/crdcompatibility/controller_test.go | head -100Repository: openshift/cluster-capi-operator
Length of output: 4356
🏁 Script executed:
# Search for tests related to finalizer removal
rg "clearFinalizer|setFinalizer" -A 10 -B 5 pkg/controllers/crdcompatibilityRepository: openshift/cluster-capi-operator
Length of output: 4076
🏁 Script executed:
# Check how the reconcile function uses these finalizer helpers
cat -n pkg/controllers/crdcompatibility/reconcile.goRepository: openshift/cluster-capi-operator
Length of output: 6734
🏁 Script executed:
# Search for how other controllers properly handle finalizer removal
rg "finalizer" --type go -B 3 -A 3 | grep -E "(WithFinalizers|RemoveFinalizer)" -B 5 -A 5 | head -80Repository: openshift/cluster-capi-operator
Length of output: 7610
clearFinalizer does not remove the finalizer via SSA as currently implemented.
With SSA, omitting the finalizers field from the patch means "don't modify this field." The ForceOwnership option takes over field ownership but does not affect fields absent from the patch. To explicitly remove finalizers owned by this field owner, you must include the field in the patch—either by calling WithFinalizers() with no arguments (to create an empty finalizers array) or by passing an explicit empty slice.
The current implementation will not actually clear the finalizer, leaving the object unable to complete deletion. This breaks the deletion reconciliation flow in reconcileDelete (line 172 of reconcile.go), which relies on clearFinalizer to remove the finalizer before the object can be garbage-collected.
🤖 Prompt for AI Agents
In `@pkg/controllers/crdcompatibility/finalizer.go` around lines 39 - 42,
clearFinalizer currently omits the finalizers field in the server-side apply
patch so SSA treats it as “do not modify” and the finalizer is never removed;
update the apply config passed to writeFinalizer (the
CompatibilityRequirement(...) used inside clearFinalizer) to explicitly set an
empty finalizers array (e.g. call WithFinalizers() with no args or
WithFinalizers([]string{})) so the SSA patch clears the finalizer, keeping
ForceOwnership behavior intact and allowing reconcileDelete to complete.
| func (r *reconcileState) writeStatus(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement, reconcileErr error) error { | ||
| // Don't write status if the object has no finalizer | ||
| if obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers, finalizerName) { | ||
| logf.FromContext(ctx).Info("Skipping status because the object is being deleted") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Log message is misleading - the condition checks for missing finalizer, not deletion.
The condition obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers, finalizerName) evaluates to true when the object is not being deleted and lacks the finalizer. However, the log message states "Skipping status because the object is being deleted," which is inaccurate.
✏️ Suggested fix
func (r *reconcileState) writeStatus(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement, reconcileErr error) error {
// Don't write status if the object has no finalizer
if obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers, finalizerName) {
- logf.FromContext(ctx).Info("Skipping status because the object is being deleted")
+ logf.FromContext(ctx).Info("Skipping status because the object has no finalizer yet")
return nil
}📝 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.
| func (r *reconcileState) writeStatus(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement, reconcileErr error) error { | |
| // Don't write status if the object has no finalizer | |
| if obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers, finalizerName) { | |
| logf.FromContext(ctx).Info("Skipping status because the object is being deleted") | |
| return nil | |
| } | |
| func (r *reconcileState) writeStatus(ctx context.Context, obj *apiextensionsv1alpha1.CompatibilityRequirement, reconcileErr error) error { | |
| // Don't write status if the object has no finalizer | |
| if obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers, finalizerName) { | |
| logf.FromContext(ctx).Info("Skipping status because the object has no finalizer yet") | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/controllers/crdcompatibility/status.go` around lines 36 - 41, The log
message in reconcileState.writeStatus is incorrect: the condition
(obj.DeletionTimestamp.IsZero() && !slices.Contains(obj.Finalizers,
finalizerName)) detects an object that is NOT being deleted but is missing the
finalizer, so update the log emitted via logf.FromContext(ctx).Info to
accurately state that status write is being skipped because the object lacks the
finalizer (reference writeStatus, obj.DeletionTimestamp.IsZero(),
slices.Contains(..., finalizerName), and logf.FromContext(ctx).Info) so the
message matches the actual condition.
| func getCRDVersion(crd *apiextensionsv1.CustomResourceDefinition, version string) *apiextensionsv1.CustomResourceDefinitionVersion { | ||
| for _, v := range crd.Spec.Versions { | ||
| if v.Name == version { | ||
| return &v | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "crdchecker_test.go" -o -name "crdchecker.go" | head -20Repository: openshift/cluster-capi-operator
Length of output: 141
🏁 Script executed:
cd $(git ls-files | grep -E "crdchecker\.(go|test\.go)$" | head -1 | xargs dirname) 2>/dev/null && pwd || echo "not found"Repository: openshift/cluster-capi-operator
Length of output: 108
🏁 Script executed:
git ls-files | grep crdcheckerRepository: openshift/cluster-capi-operator
Length of output: 176
🏁 Script executed:
cat -n pkg/crdchecker/crdchecker.go | head -80Repository: openshift/cluster-capi-operator
Length of output: 2556
🏁 Script executed:
cat -n pkg/crdchecker/crdchecker_test.go | head -100Repository: openshift/cluster-capi-operator
Length of output: 3864
🏁 Script executed:
grep -n "getCRDVersion" pkg/crdchecker/crdchecker_test.goRepository: openshift/cluster-capi-operator
Length of output: 301
🏁 Script executed:
cat -n pkg/crdchecker/crdchecker_test.go | sed -n '82,110p'Repository: openshift/cluster-capi-operator
Length of output: 1213
Fix getCRDVersion to return pointer to actual slice element.
In Go range loops, the loop variable v is a copy on the stack. Returning &v points to this stack copy, not the original element in crd.Spec.Versions. This causes mutations in test cases (lines 85-86 and 98-101) to not take effect, making the tests ineffective.
Suggested fix
func getCRDVersion(crd *apiextensionsv1.CustomResourceDefinition, version string) *apiextensionsv1.CustomResourceDefinitionVersion {
- for _, v := range crd.Spec.Versions {
- if v.Name == version {
- return &v
- }
- }
+ for i := range crd.Spec.Versions {
+ if crd.Spec.Versions[i].Name == version {
+ return &crd.Spec.Versions[i]
+ }
+ }
return nil
}📝 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.
| func getCRDVersion(crd *apiextensionsv1.CustomResourceDefinition, version string) *apiextensionsv1.CustomResourceDefinitionVersion { | |
| for _, v := range crd.Spec.Versions { | |
| if v.Name == version { | |
| return &v | |
| } | |
| } | |
| func getCRDVersion(crd *apiextensionsv1.CustomResourceDefinition, version string) *apiextensionsv1.CustomResourceDefinitionVersion { | |
| for i := range crd.Spec.Versions { | |
| if crd.Spec.Versions[i].Name == version { | |
| return &crd.Spec.Versions[i] | |
| } | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In `@pkg/crdchecker/crdchecker_test.go` around lines 38 - 43, getCRDVersion
currently returns &v from a range loop which yields a copy; change it to iterate
with index and return a pointer to the actual slice element (e.g.,
&crd.Spec.Versions[i]) when v.Name == version, otherwise return nil; update
function getCRDVersion to use for i := range crd.Spec.Versions { v :=
&crd.Spec.Versions[i]; if v.Name == version { return v } } to ensure callers
mutate the real element.
| It("should fail when a field is removed", func() { | ||
| errors, _ := runTest( | ||
| func(target *apiextensionsv1.CustomResourceDefinition) *apiextensionsv1.CustomResourceDefinition { | ||
| version := getCRDVersion(target, "v1") | ||
| delete(version.Schema.OpenAPIV3Schema.Properties, "spec") | ||
|
|
||
| return target | ||
| }, | ||
| ) | ||
|
|
||
| Expect(errors).NotTo(ConsistOf(MatchError("removed field : v1.^.spec")), "should contain an error") | ||
| }) |
There was a problem hiding this comment.
Fix assertions: inverted expectation and wrong matcher type.
errors is []string, but MatchError expects error, and NotTo negates the intended check.
✅ Suggested fix
- Expect(errors).NotTo(ConsistOf(MatchError("removed field : v1.^.spec")), "should contain an error")
+ Expect(errors).To(ContainElement(ContainSubstring("removed field : v1.^.spec")), "should contain an error")
@@
- Expect(errors).NotTo(ConsistOf(MatchError("served versions removed : v1beta1")), "should contain an error")
+ Expect(errors).To(ContainElement(ContainSubstring("served versions removed : v1beta1")), "should contain an error")Also applies to: 111-120
🤖 Prompt for AI Agents
In `@pkg/crdchecker/crdchecker_test.go` around lines 82 - 93, The test assertion
is inverted and uses the wrong matcher type: replace
Expect(errors).NotTo(ConsistOf(MatchError("removed field : v1.^.spec"))) with an
assertion that checks the string slice contains the expected message; e.g. use
Expect(errors).To(ContainElement("removed field : v1.^.spec")) or
Expect(errors).To(ContainElement(MatchRegexp(`removed field : v1\.\^\.spec`))).
Update the same pattern in the other failing test (the similar assertion around
the second failing block) and locate the checks in the test that call runTest
and use getCRDVersion to modify the CRD schema.
| for _, condition := range currentConditions { | ||
| if condition.Type == *conditionAC.Type { |
There was a problem hiding this comment.
Potential nil pointer dereference on conditionAC.Type.
Line 61 dereferences *conditionAC.Type without a nil check, while the matchingCondition closure defensively guards against nil for Status, Reason, Message, and ObservedGeneration. If Type is nil, this will panic.
Consider adding a guard clause for consistency with the defensive approach in matchingCondition:
🛡️ Proposed fix
func SetLastTransitionTimeMetaV1(now metav1.Time, currentConditions []metav1.Condition, conditionAC *metav1applyconfig.ConditionApplyConfiguration) *metav1applyconfig.ConditionApplyConfiguration {
+ if conditionAC.Type == nil {
+ return conditionAC.WithLastTransitionTime(now)
+ }
+
matchingCondition := func(condition *metav1.Condition, conditionAC *metav1applyconfig.ConditionApplyConfiguration) bool {🤖 Prompt for AI Agents
In `@pkg/util/sync.go` around lines 60 - 61, The loop compares condition.Type to
*conditionAC.Type and can panic if conditionAC or conditionAC.Type is nil; add a
guard before dereferencing: ensure conditionAC != nil and conditionAC.Type !=
nil (or handle nil Type explicitly) before using *conditionAC.Type, mirroring
the nil checks used in matchingCondition so the comparison is safe and
consistent with the existing defensive checks.
|
/test e2e-aws-capi-techpreview Quick sample E2E |
|
@JoelSpeed: 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. |
3 participants
This is currently stacked on #457, so only the last 10 commits are relevant to this PR.
This bootstraps the controller and logic for the compatibility requirement. Checking compatibility of the schema in the spec and updating the status appropriately.
Also implements the validating webhook for the CompatibilityRequirement to validate that the CRD data is valid, and that the fields we couldn't validate with openapi are validated (matchConditions, namespace selector, object selector)
Summary by CodeRabbit
Release Notes
New Features
Tests
Chores