feat: add SchemaUpdatePolicy for updating BoundSchemas

[cnvergence]

Summary

Adds a SchemaUpdatePolicy field that controls whether BoundSchemas are kept in sync with their source CRDs after initial creation. This enables continuous CRD schema propagation from provider to consumer when the policy is set to Always.
Currently, we will keep the default as it is, but we might progress it to Always when stabilised.
Using configurable interval to reconcile APIServiceExportRequest currently, we might migrate to properly set up watches in the future, we need to support different schema sources.

What Type of PR Is This?

/kind feature

Related Issue(s)

Fixes #301

Release Notes

Added SchemaUpdatePolicy for updating BoundSchemas.

[mjudeikis]

@mjudeikis-bot please review this

[mjudeikis-bot]

Thanks for the PR! Overall solid approach — using a SHA256 hash of BoundSchemaSpec to detect drift is clean and avoids spurious updates. A few observations:

Correctness / Design:

1. 30-second requeue interval — requeuing every 30s for all SchemaUpdatePolicyAlways resources could create significant controller load in clusters with many bound schemas. Consider making the interval configurable (e.g., a controller flag like --schema-sync-interval) or defaulting to something longer (e.g., 5 min). Alternatively, a Watch on the source CRD would be more event-driven.

2. Policy asymmetry — APIServiceExportRequest.Spec.SchemaUpdatePolicy is marked immutable via CEL validation (self == oldSelf), but APIServiceExportTemplate.Spec.SchemaUpdatePolicy has no such constraint. If a template policy is changed after requests are already created, those requests remain locked to the old policy. Is this intentional? Worth documenting explicitly.

3. export == nil path — in the old code, if export == nil { return nil } was an early-exit guard. In the new code, when export == nil, owner-reference setting is skipped (wrapped in if export != nil), but the hash/update logic still runs. This seems intentional, but worth adding a comment explaining why we allow schema sync even without an export reference.

Minor:

4. Missing tests — no test files appear in this diff. The SchemaUpdatePolicyAlways path (hash mismatch → update, hash match → no-op, requeue behavior) should have unit/integration test coverage.

5. BoundSchemasSpecHash change — switching from hashing the full schema object to just schema.Spec is a correctness fix (metadata churn no longer triggers spurious hash changes). Good catch, though the TODO comment removal is a bit abrupt — was issue #297 actually resolved by this change?

6. Annotation key constant — kubebindv1alpha2.SourceSpecHashAnnotationKey is used but not defined in this diff. Presumably it exists elsewhere; just make sure it has a clear doc comment.

Overall the implementation is clean. Main concern is the 30s hard-coded requeue — that could become a scalability issue worth addressing before merge.

[cnvergence]

@mjudeikis-bot please review this

[mjudeikis-bot]

Code Review

Thanks for the PR! The overall design is solid — using a SHA-256 spec-hash annotation to gate updates is a clean, efficient approach. A few observations below.

---

✅ Positives

• Hash fix in BoundSchemasSpecHash: Switching from encoding the whole schema object to encoding only schema.Spec is an important correctness fix. Previously, updating the hash annotation would itself change the hash on the next pass, causing an infinite update loop. Good catch.
• Immutability via CEL: Locking schemaUpdatePolicy on the APIServiceExportRequest via x-kubernetes-validations while keeping it mutable on the template is exactly the right design — lets providers change the default for new bindings without affecting existing ones.
• --schema-sync-interval flag: Sensible default of 5m with full flag exposure.

---

🟡 Issues / Questions

1. First reconcile with Always on existing schemas will always trigger an update

The hash annotation (kube-bind.io/source-spec-hash) is only written to BoundSchemas at creation time. Existing schemas that predate this PR (or were created with Never) won't have the annotation. When policy is Always, the comparison:

if existing.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] != hash {

will always be true on the first reconcile (empty string vs computed hash), unconditionally overwriting the spec. This could be surprising — consider treating a missing annotation as "write annotation only, no spec overwrite" on first encounter, or at least document this as intentional migration behaviour.

2. Behaviour change when export == nil

The old code returned early when existing != nil && export == nil. The new code continues with schema sync in that case. The comment explains the motivation, but this is a silent behaviour change for the Never policy path too (it's a no-op if needsUpdate stays false, so likely safe). A test case for export == nil, policy=Never would confirm nothing regresses.

3. Unconditional RequeueAfter when policy is Always

if apiServiceExportRequest.Spec.SchemaUpdatePolicy == kubebindv1alpha2.SchemaUpdatePolicyAlways {
    return ctrl.Result{RequeueAfter: r.reconciler.schemaSyncInterval}, nil
}

This requeues every cycle regardless of whether ensureBoundSchemas made any changes. In clusters with many Always-policy requests this is steady controller churn. Not a blocker, but worth a comment.

4. No unit tests for the new hash-comparison logic

Given the subtleties above, table-driven unit tests for createOrUpdateBoundSchema covering at minimum:

• existing has annotation, hash matches → no update
• existing has annotation, hash differs → update
• existing missing annotation → first-reconcile behaviour
• export == nil, policy=Never → no update

5. Minor: SchemaUpdatePolicy type placement

The type is defined in apiserviceexporttemplate_types.go but is shared with the request types. Consider moving it to a shared file (e.g. types.go) in the same package to make the dependency explicit.

---

Overall this is a well-structured feature. Addressing point 1 (or documenting it) and adding tests would make it merge-ready.

[mjudeikis-bot]

Thanks for the PR! Overall this is a well-structured addition with a clean API design. A few observations:

Positives:

• Good use of CEL self == oldSelf to lock in SchemaUpdatePolicy at binding time on APIServiceExportRequest, while keeping the template field mutable — that is the right trade-off.
• Hash-based drift detection (BoundSchemaSpecHash) is efficient, and the needsUpdate flag correctly batches owner-ref + spec updates into a single API call.
• The fix to BoundSchemasSpecHash (encoding schema.Spec instead of the full object) is a nice correctness improvement, could previously cause spurious hash mismatches.
• Default Never preserves backward-compatibility.

Concerns:

1. Orphaned BoundSchemas when export == nil (behavior change): Previously when export == nil and a BoundSchema already existed, the function returned early (if export == nil { return nil }). Now the function continues and can create new BoundSchemas even when export == nil, meaning newly created BoundSchemas will have no owner reference. These will not be garbage-collected if the APIServiceExportRequest is deleted. Consider guarding the create path: skip creation when export == nil (or add an explicit owner-ref-less-creation comment explaining the intent).

2. No lower-bound validation on SchemaSyncInterval: A zero or near-zero value would cause a reconciler storm. Consider adding a minimum (e.g., 10s) in Complete() / option validation.

3. Missing tests: There are no tests visible in the diff for the SchemaUpdatePolicyAlways reconciliation path (hash comparison, update triggering, requeue behaviour) or for the new BoundSchemaSpecHash helper. Given this is a non-trivial controller behaviour change, unit tests here would be valuable before merge.

4. Minor nit: The removed // https://github.com/kube-bind/kube-bind/issues/297 To fix. comment — worth noting in the PR description that this is intentionally resolved by the spec-only hashing fix.

Happy to discuss any of these further. Good work overall!

[mjudeikis-bot]

Follow-up review on the latest commit — two targeted improvements noted:

1. Owner-reference check (IsControlledBy guard)

Switching from the compare-length approach to metav1.IsControlledBy(existing, export) before calling SetControllerReference is strictly better: it is semantically accurate (checks the actual controller ref, not just count) and avoids an unnecessary mutation call when the ref is already set. ✅

2. SchemaSyncInterval lower-bound validation

Adding if options.SchemaSyncInterval < 10*time.Second { return error } in Validate() directly addresses my earlier concern about a zero/near-zero interval causing a reconciler storm. Good. ✅

Both changes address feedback from the previous reviews. The remaining open items from prior reviews (missing unit tests for the hash-comparison paths, and the first-reconcile annotation bootstrap behaviour for pre-existing schemas) are still outstanding — those would be the last things needed before merge.

[mjudeikis-bot]

Code Review — feat: add SchemaUpdatePolicy for updating BoundSchemas

Overall this is a well-structured addition. The hash-based drift detection approach is clean, the CEL immutability rule on schemaUpdatePolicy at bind time is exactly right, and the backward-compatible Never default ensures no surprises for existing users. A few things worth addressing before merge:

---

🔴 Correctness / Potential Regression

export == nil path in createOrUpdateBoundSchema
The old code returned nil immediately when export == nil. The new code intentionally continues to run spec sync even without an export, reasoning that BoundSchemas should be ready before the export is created. This is fine for SchemaUpdatePolicy=Always, but for SchemaUpdatePolicy=Never it means we'd attempt updateBoundSchema (if needsUpdate=true) without an owner. In practice needsUpdate will be false for Never+export==nil today — but the path is fragile. Consider an explicit early-return or at minimum a comment to prevent a future regression here.

---

🟡 Design / Clarity

desired is mutated before the hash is computed
The isolation scope rewrite (desired.Spec.Scope = ClusterScoped) now happens at the top of createOrUpdateBoundSchema, before BoundSchemaSpecHash is called. This is semantically correct (hash should reflect stored spec), but it silently mutates the caller's struct. Consider cloning desired.Spec at the function entry or adding a comment that this mutation is intentional.

RequeueAfter block is uncommented

if apiServiceExportRequest.Spec.SchemaUpdatePolicy == SchemaUpdatePolicyAlways {
    return ctrl.Result{RequeueAfter: r.reconciler.schemaSyncInterval}, nil
}

This is the intentional polling loop — worth a one-liner comment (e.g. // Polling loop: re-reconcile periodically to detect source CRD changes) so future maintainers don't mistake it for dead code or a missing error path.

---

🟡 Missing Test Coverage

No unit tests are visible in the diff for the new policy logic. At minimum the following cases should be covered:

• SchemaUpdatePolicy=Never: spec of existing BoundSchema is NOT updated even when source hash differs.
• SchemaUpdatePolicy=Always + matching hash: no write is performed.
• SchemaUpdatePolicy=Always + hash mismatch: spec is updated and annotation is refreshed.
• export == nil + both policies (to lock in the intended behavior).

---

🟢 Nits

• The // https://github.com/kube-bind/kube-bind/issues/297 To fix. comment removal is a nice cleanup — if that issue is resolved by this PR, close it or mention it in the PR description.
• --schema-sync-interval default of 5m and minimum of 10s are sensible choices. A note about reconciliation load at short intervals in the flag help text would be helpful for operators.
• CRD YAMLs, KCP APIResourceSchema, and Helm chart all consistently updated. ✅

---

Summary: Feature logic is sound. Address the export==nil/policy=Never path clarity and add unit tests before merging. Happy to re-review once those are in.