Repository navigation
Conversation
c675d78 to
d9afbba
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Admission race and bypass paths can still permit scope changes, while migration failures can advance to source cleanup before histories are copied.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds persistent APIExport scope history and admission enforcement to prevent previously served resources from changing between cluster and namespace scope.
Changes:
- Introduces the APIExportHistory API and generated SDK support.
- Adds history reconciliation, admission validation, and migration handling.
- Adds unit and end-to-end coverage.
File summaries
| File | Description |
|---|---|
test/e2e/reconciler/apiexport/apiexporthistory_test.go |
Tests scope-change rejection end to end. |
staging/src/github.com/kcp-dev/sdk/openapi/zz_generated.openapi.go |
Adds generated OpenAPI definitions. |
staging/src/github.com/kcp-dev/sdk/client/listers/apis/v1alpha2/expansion_generated.go |
Adds lister expansion interfaces. |
staging/src/github.com/kcp-dev/sdk/client/listers/apis/v1alpha2/apiexporthistory.go |
Adds generated history listers. |
staging/src/github.com/kcp-dev/sdk/client/informers/externalversions/generic.go |
Registers generic history informers. |
staging/src/github.com/kcp-dev/sdk/client/informers/externalversions/apis/v1alpha2/interface.go |
Exposes history informers. |
staging/src/github.com/kcp-dev/sdk/client/informers/externalversions/apis/v1alpha2/apiexporthistory.go |
Implements generated history informers. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/generated_expansion.go |
Adds client expansion interface. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/fake/fake_apis_client.go |
Exposes the fake history client. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/fake/fake_apiexporthistory.go |
Implements the fake history client. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apis_client.go |
Exposes the typed history client. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apiexporthistory.go |
Implements the typed history client. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/generated_expansion.go |
Adds cluster-client expansion. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apis_client.go |
Exposes fake cluster history clients. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apiexporthistory.go |
Implements fake cluster history clients. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apis_client.go |
Exposes the cluster history client. |
staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apiexporthistory.go |
Implements the cluster history client. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go |
Registers history apply configurations. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/resourcehistory.go |
Adds resource-history apply support. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistorystatus.go |
Adds status apply support. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryspec.go |
Adds spec apply support. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryref.go |
Adds reference apply support. |
staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistory.go |
Adds resource apply support. |
staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.model_name.go |
Adds generated model names. |
staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.deepcopy.go |
Adds generated deep-copy methods. |
staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/types_apiexporthistory.go |
Defines the history API types. |
staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/register.go |
Registers history API types. |
pkg/server/server.go |
Installs the history controller. |
pkg/server/controllers.go |
Configures controller informers and indexes. |
pkg/reconciler/migration/logicalclustermigration/reconciler.go |
Integrates history migration. |
pkg/reconciler/migration/logicalclustermigration/apiexporthistories.go |
Copies histories between shards. |
pkg/reconciler/apis/apiexporthistory/apiexporthistory_reconcile.go |
Records and cleans up scope history. |
pkg/reconciler/apis/apiexporthistory/apiexporthistory_reconcile_test.go |
Tests history reconciliation. |
pkg/reconciler/apis/apiexporthistory/apiexporthistory_controller.go |
Implements the history controller. |
pkg/indexers/apiexport.go |
Adds history-to-export indexing. |
pkg/admission/apiexport/history_admission_test.go |
Tests history admission behavior. |
pkg/admission/apiexport/admission.go |
Rejects incompatible scope changes. |
config/system-crds/bootstrap.go |
Bootstraps the history CRD. |
config/crds/apis.kcp.io_apiexporthistories.yaml |
Defines the history CRD. |
Review details
Files not reviewed (16)
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.deepcopy.go: Generated file
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.model_name.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryref.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryspec.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistorystatus.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/resourcehistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/generated_expansion.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/fake/fake_apiexporthistory.go: Generated file
- Files reviewed: 23/39 changed files
- Comments generated: 5
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d9afbba to
c66dcee
Compare
There was a problem hiding this comment.
🟡 Changes recommended
History migration uses an incompatible endpoint, and admission still permits a remove-and-readd race.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (14)
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.deepcopy.go: Generated file
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.model_name.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryref.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryspec.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistorystatus.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/resourcehistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/generated_expansion.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- Files reviewed: 27/41 changed files
- Comments generated: 2
- Review effort level: Balanced
c66dcee to
ec57994
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Admission and informer races can still allow prohibited scope changes or leave histories permanently unpopulated.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (15)
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.deepcopy.go: Generated file
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.model_name.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryref.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryspec.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistorystatus.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/resourcehistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/generated_expansion.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apis_client.go: Generated file
Suppressed comments (2)
pkg/admission/apiexport/admission.go:230
- Comparing only the immediately previous APIExport does not close the missing-history race. Before the asynchronous controller records the initial scope, a client can first remove the resource and then re-add the same GVR with the opposite scope; on the second update the history is absent and the old object has no resource, so
recordedis empty and the change is admitted. The initial baseline must be established synchronously, or updates/removals must be rejected until the history is available; this two-update sequence should also be covered by a test.
if old != nil {
for gr, scope := range e.servedScopes(clusterName, old) {
if _, ok := recorded[gr]; !ok {
recorded[gr] = scope
}
}
pkg/admission/apiexport/admission.go:240
- Treating a cache miss as permission to continue creates another bypass even when history already exists. A provider can create an opposite-scope schema and immediately update the APIExport before the schema informer observes it; this
NotFoundbranch admits the update, and the history controller later preserves the old scope rather than repairing the now-conflicting export. For a resource whose declared group/name already appears inrecorded, reject or retry while its schema cannot be resolved (or use a live read).
for i, resource := range ae.Spec.Resources {
ars, err := e.apiResourceSchemaLister.Cluster(clusterName).Get(resource.Schema)
if apierrors.IsNotFound(err) {
continue
- Files reviewed: 25/40 changed files
- Comments generated: 2
- Review effort level: Balanced
ec57994 to
a793552
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Shard-local histories are not preserved through logical-cluster migration, allowing prior scope constraints to be lost.
Get a fresh assessment by requesting another Copilot review.
Review details
Files not reviewed (15)
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.deepcopy.go: Generated file
- staging/src/github.com/kcp-dev/sdk/apis/apis/v1alpha2/zz_generated.model_name.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryref.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistoryspec.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/apiexporthistorystatus.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/apis/v1alpha2/resourcehistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/fake/apis_client.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/apis/v1alpha2/generated_expansion.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apiexporthistory.go: Generated file
- staging/src/github.com/kcp-dev/sdk/client/clientset/versioned/typed/apis/v1alpha2/apis_client.go: Generated file
- Files reviewed: 25/40 changed files
- Comments generated: 1
- Review effort level: Balanced
a793552 to
d024dfc
Compare
mjudeikis
left a comment
There was a problem hiding this comment.
First rould of review. Some changes will tune the implementation a bit
d024dfc to
9917564
Compare
|
/retest |
mjudeikis
left a comment
There was a problem hiding this comment.
few comment for mysefl to check
| old := &apisv1alpha1.APIExport{} | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, old); err != nil { | ||
| return nil, fmt.Errorf("failed to convert unstructured to APIExport: %w", err) | ||
| } | ||
| v2 := new(apisv1alpha2.APIExport) | ||
| if err := apisv1alpha2.Convert_v1alpha1_APIExport_To_v1alpha2_APIExport(old, v2, nil); err != nil { | ||
| return nil, fmt.Errorf("failed to convert v1alpha1 APIExport to v1alpha2: %w", err) | ||
| } | ||
| return v2, nil | ||
| case apisv1alpha2.SchemeGroupVersion.Version: | ||
| old := &apisv1alpha2.APIExport{} | ||
| if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.Object, old); err != nil { | ||
| return nil, fmt.Errorf("failed to convert unstructured to APIExport: %w", err) | ||
| } |
There was a problem hiding this comment.
TODO: check if need converted here.
There was a problem hiding this comment.
Checked: no conversion needed here. For an UPDATE the old object is handed to admission in the request version, so a.GetKind().GroupVersion() applies to both old and new; the v1alpha1 branch above already converts and Convert_v1alpha1_APIExport_To_v1alpha2_APIExport folds in the overhanging ResourceSchemasAnnotation. Resolving.
| }) | ||
|
|
||
| _, _ = apiResourceSchemaInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ | ||
| AddFunc: func(obj interface{}) { c.enqueueAPIResourceSchema(obj) }, |
There was a problem hiding this comment.
TODO: check if we can do noop schema update (change labels)
There was a problem hiding this comment.
Checked: APIResourceSchema spec is fully immutable on update (ValidateAPIResourceSchemaUpdate), so a labels-only update cannot change scope and AddFunc alone is sufficient. Resolving.
mjudeikis
left a comment
There was a problem hiding this comment.
Second round. Overall shape is good now (admin group, hashed name, no spec, no migration). Two things I think need fixing before this merges: the stale-history case on APIExport delete+recreate, and the lister-miss skip in APIExport admission. Details inline.
|
@neolit123 did a manual review and AI-assisted, as this one is a big one :/ few comments before merging |
9917564 to
460eb65
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A failed status write during history rebinding can permanently retain a deleted export’s scopes and block valid updates.
2 open issues
2 resolved since last review
12 files not reviewed
staging/src/github.com/kcp-dev/sdk/apis/admin/v1alpha1/zz_generated.deepcopy.go: Generated filestaging/src/github.com/kcp-dev/sdk/apis/admin/v1alpha1/zz_generated.model_name.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/applyconfiguration/admin/v1alpha1/apiexporthistory.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/applyconfiguration/admin/v1alpha1/apiexporthistorystatus.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/applyconfiguration/admin/v1alpha1/resourcehistory.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/applyconfiguration/utils.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/clientset.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/clientset.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/fake/clientset.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/fake/register.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/scheme/register.go: Generated filestaging/src/github.com/kcp-dev/sdk/client/clientset/versioned/cluster/typed/admin/v1alpha1/admin_client.go: Generated file
Review effort: Balanced
460eb65 to
d8636c4
Compare
|
/hold |
As discovered earlier if the scope of a resource changed from cluster to namespaced, and this resource is re-introduced in the same APIExport, existing consumer CR instances can cause some confusion in kcp. Add a new API that tracks the APIExport resource scopes, and block on admission if the scope of a previously known resource changed. If the provider wants to workaround this problem, they need to either remove the APIExport or introduce a different GVR. Signed-off-by: Lubomir I. Ivanov <neolit123@gmail.com>
d8636c4 to
b8dec06
Compare
|
[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 |
|
had to rebase the PR. /retest |
|
/retest |


Summary
As discovered earlier if the scope of a resource changed from cluster to namespaced, and this resource is re-introduced in the same APIExport, existing consumer CR instances can cause some confusion in kcp (high MEM/CPU usage).
Add a new API that tracks the APIExport resource scopes, and block on admission if the scope of a previously known resource changed.
If the provider wants to workaround this problem, they need to either remove the APIExport or introduce a different GVR.
slack thread:
https://kubernetes.slack.com/archives/C09C7UP1VLM/p1789467382474599
What Type of PR Is This?
/kind bug feature
Related Issue(s)
Fixes #4330
Release Notes