Add storage version migration support #588
Conversation
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
📝 WalkthroughWalkthroughAdds a CRD storage-version migration facility (CRDsMigrator) with discovery-driven migration logic, integrates it into Provider, adds a standalone CLI (crds-migrator) to run updates from mappings or files, and promotes k8s.io/apiextensions-apiserver to a direct go.mod requirement. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI
participant Migrator as CRDsMigrator
participant Discovery as RESTMapper/Discovery
participant API as Kubernetes API
CLI->>Migrator: Start with CRD mapping & backoff config
Migrator->>Discovery: Discover API resources / build RESTMapper
loop per GVK
Migrator->>Discovery: Resolve CRD name from GVK
Discovery-->>Migrator: CRD name
Migrator->>API: Get CRD object
API-->>Migrator: CRD
Migrator->>Migrator: Determine storageVersion & needMigration
alt storedVersions differ
Migrator->>API: List resources of storage version (batched)
API-->>Migrator: Resource batch
loop each resource
Migrator->>API: Patch resource (noop) to trigger conversion (with retry/backoff)
API-->>Migrator: Resource patched
end
Migrator->>API: Patch CRD.Status.StoredVersions (with retry/backoff)
API-->>Migrator: Status patched
end
end
Migrator-->>CLI: Report successes/failures
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 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
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 |
![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
🧹 Nitpick comments (3)
pkg/config/crds_migrator.go (3)
11-11: Inconsistent error package usage.This file uses
github.com/crossplane/crossplane-runtime/v2/pkg/errorswhileprovider.goin the same package usesgithub.com/pkg/errors. Per coding guidelines, prefergithub.com/pkg/errorsfor consistency.♻️ Suggested change
- "github.com/crossplane/crossplane-runtime/v2/pkg/errors" + "github.com/pkg/errors"
96-116: Consider adding progress logging for large migrations.For CRDs with thousands of resources, the batch patching loop could run for a long time without feedback. Consider adding periodic progress logging (e.g., after each batch).
♻️ Optional enhancement
+ batchCount := 0 for { if err := kube.List(ctx, &resources, client.Limit(500), client.Continue(continueToken), ); err != nil { return errors.Wrapf(err, "cannot list %s", resources.GroupVersionKind().String()) } for i := range resources.Items { // apply empty patch for storage version upgrade res := resources.Items[i] if err := kube.Patch(ctx, &res, client.RawPatch(types.MergePatchType, []byte(`{}`))); err != nil { return errors.Wrapf(err, "cannot patch %s %q", crd.Spec.Names.Kind, res.GetName()) } } + batchCount++ + logr.Debug("Processed batch", "crd", crdName, "batch", batchCount, "resourcesInBatch", len(resources.Items)) continueToken = resources.GetContinue() if continueToken == "" { break } }
141-154: Consider wrapping errors with context.Per coding guidelines, wrap errors with context using patterns like
errors.Wrap(err, "context"). The bare error returns on lines 144 and 150 lose context about what operation failed.♻️ Suggested fix
func GetCRDNameFromGVK(discoveryClient discovery.DiscoveryInterface, gvk schema.GroupVersionKind) (string, error) { groupResources, err := restmapper.GetAPIGroupResources(discoveryClient) if err != nil { - return "", err + return "", errors.Wrap(err, "cannot get API group resources") } mapper := restmapper.NewDiscoveryRESTMapper(groupResources) mapping, err := mapper.RESTMapping(gvk.GroupKind(), gvk.Version) if err != nil { - return "", err + return "", errors.Wrapf(err, "cannot get REST mapping for %s", gvk.String()) } return mapping.Resource.Resource + "." + mapping.Resource.Group, nil }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
go.modpkg/config/crds_migrator.gopkg/config/provider.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Do not useanytype throughout codebase - use concrete types or type parameters instead
Use pointer types for optional fields in generated structs
Avoid type aliases in favor of explicit types
Usegithub.com/pkg/errorsfor error wrapping with context
Return errors from functions instead of panicking, except for impossible states
Wrap errors with context using patterns like:errors.Wrap(err, "cannot configure resource")
Avoid circular dependencies between packages
Files:
pkg/config/crds_migrator.gopkg/config/provider.go
pkg/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
Public API packages should be organized under
pkg/directory
Files:
pkg/config/crds_migrator.gopkg/config/provider.go
go.mod
📄 CodeRabbit inference engine (CLAUDE.md)
go.mod: Module path must begithub.com/crossplane/upjet/v2
When testing in providers, addreplace github.com/crossplane/upjet/v2 => ../upjetto provider's go.mod
Runmake modules.checkto verify go.mod/go.sum are tidy before committing
Files:
go.mod
🧬 Code graph analysis (2)
pkg/config/crds_migrator.go (1)
pkg/config/provider.go (1)
Provider(73-186)
pkg/config/provider.go (1)
pkg/config/crds_migrator.go (1)
CRDsMigrator(28-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: check-diff
🔇 Additional comments (7)
go.mod (1)
39-39: LGTM!The promotion of
k8s.io/apiextensions-apiserverto a direct dependency is appropriate since the newCRDsMigratordirectly usesextv1.CustomResourceDefinition. The version aligns correctly with other k8s.io dependencies at v0.33.0.pkg/config/provider.go (2)
182-186: LGTM!The
StorageVersionMigratorfield follows the established pattern for optional Provider fields with pointer type and clear documentation.
315-321: LGTM!The
WithStorageVersionMigratoroption follows the consistent pattern of otherProviderOptionfunctions in this file.pkg/config/crds_migrator.go (4)
27-39: LGTM!The struct and constructor are clean and follow Go conventions with an unexported field and exported constructor.
118-134: LGTM!The status update with verification is good defensive programming. Using
MergeFromfor the status patch is the correct approach.
156-171: LGTM!The function correctly identifies resources requiring migration based on
PreviousVersionsand constructs GVKs following upjet conventions.
1-1: License header year appears incorrect.The copyright year is
2026, but current date is January 2026 and this is new code. Typically, copyright years reflect when the code was written. If this was written in 2025 or earlier during development, consider using the appropriate year.⛔ Skipped due to learnings
Learnt from: CR Repo: crossplane/upjet PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-13T21:29:32.267Z Learning: Use Kubernetes-native patterns via crossplane-runtimeLearnt from: CR Repo: crossplane/upjet PR: 0 File: CLAUDE.md:0-0 Timestamp: 2026-01-13T21:29:32.266Z Learning: Applies to go.mod : Module path must be `github.com/crossplane/upjet/v2`
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func PrepareCRDsMigrator(pc *Provider) { | ||
| var gvkList []schema.GroupVersionKind | ||
| for _, r := range pc.Resources { | ||
| if len(r.PreviousVersions) != 0 { |
There was a problem hiding this comment.
note: just for heads-up, not because there is an immediate issue. This is also not actually specific to the changes in this PR.
I checked the usage of r.PreviousVersions in general. We solely rely on the provider (therefore the developer) to correctly configure it. There are no validations/cross-check around this configuration (especially with r.Version). e.g:
- it is possible to forget configuring this:
r.Version = "v1beta2"andr.PreviousVersions = [](this might be still a valid scenario though after v1beta1 is actually removed) - no check for Version and PreviousVersions are disjoint sets.
r.Version = "v1beta2"andr.PreviousVersions = ["v1beta1", "v1beta2"]possible - no check that
PreviousVersionsare behind the current Versionr.Version = "v1beta2"andr.PreviousVersions = ["v1beta3", "v1beta4"]
Just noting these since it is getting a new feature around it.
There was a problem hiding this comment.
Yes, I also considered this, and I believe that having a complete version list, etc., as a new feature, may make sense. And this must be stored automatically without manual configuration.
pkg/config/crds_migrator.go
Outdated
| origCrd := crd.DeepCopy() | ||
|
|
||
| crd.Status.StoredVersions = []string{storageVersion} | ||
| if err := kube.Status().Patch(ctx, &crd, client.MergeFrom(origCrd)); err != nil { |
There was a problem hiding this comment.
unfortunately, provider might need an extra RBAC here for patching CRDs, which is not ideal.
There was a problem hiding this comment.
Yes, that's right. I manually provided these roles to the provider ClusterRole and tested. Therefore, if we don't take this step, I believe it will be updated at some point. However, we can't validate the migration.
From an RBAC manager's perspective, granting this permission to all providers may not be a sensible approach. This is because many providers are unwilling to perform this operation. An option is to proceed without validating this part, i.e., patching the CRDs. This sounds better than granting this right to all providers.
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
|
After the latest changes, the implementation consists of two main components: 1. Library:
|
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/crdsmigrator/main.go`:
- Around line 89-95: Validate CLI flag values before constructing retryBackoff:
ensure updateRetries (used for wait.Backoff.Steps) is >0, updateDuration is >0,
updateFactor is >0, and updateJitter is within [0,1]; if any check fails return
a descriptive error (or exit) instead of building wait.Backoff. Perform these
checks right before creating retryBackoff and reference the symbols
retryBackoff, wait.Backoff, updateRetries, updateDuration, updateFactor, and
updateJitter so the code path short-circuits on invalid values and avoids
passing a bad Backoff to wait.ExponentialBackoff.
♻️ Duplicate comments (1)
pkg/config/crds_migrator.go (1)
87-101: Consider skipping when the REST mapper can’t resolve a GVK.If
RESTMappingreturns a no‑match error (e.g., CRD not installed), the current behavior aborts the entire migration run. Skipping that GVK is usually safer.k8s.io/apimachinery/pkg/api/meta NoMatchError handling with RESTMapper.RESTMapping
🧹 Nitpick comments (1)
cmd/crdsmigrator/example.yaml (1)
1-5: Prefer YAML mapping for the example file.JSON is valid YAML, but the
.yamlextension suggests a YAML mapping; switching improves readability for users copying this example.♻️ Proposed update
-{ - "locations.conditionalaccess.azuread.upbound.io":"v1beta2", - "invitations.invitations.azuread.upbound.io":"v1beta2", - "principals.serviceprincipals.azuread.upbound.io":"v1beta2" -} +locations.conditionalaccess.azuread.upbound.io: v1beta2 +invitations.invitations.azuread.upbound.io: v1beta2 +principals.serviceprincipals.azuread.upbound.io: v1beta2
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/config/crds_migrator.go`:
- Around line 147-155: The patch loop should treat NotFound as a benign case so
deleted resources don't fail migration: inside the retry.OnError callback where
kube.Patch(...) is called (affecting res, types.MergePatchType), capture the
patch error and if apierrors.IsNotFound(err) return nil to stop retrying and
continue; otherwise return the error to preserve retry behavior. Also add the
k8s.io/apimachinery/pkg/api/errors import (commonly aliased apierrors). Keep the
existing wrapping logic (errors.Wrapf) for non-NotFound failures.
- Around line 228-243: The SSAR creation currently returns an error on any
failure which aborts the run; change the error handling around kube.Create(ctx,
ssar) in crds_migrator.go so that when the creation fails with
permission-related errors (use apierrors.IsForbidden, apierrors.IsUnauthorized,
or apierrors.IsNotFound) you treat it as "permission denied" and return (false,
nil) instead of an error; for all other errors continue to wrap and return the
error as before. Ensure you reference the existing ssar variable and the
kube.Create(ctx, ssar) call when making the change.
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/config/crds_migrator.go`:
- Around line 117-119: The Debug log call in the CRD migration loop uses
logr.Debug with a single value (crdName) instead of key/value pairs; update the
call (the one referencing needMigration and crdName) to pass a descriptive key
and the crdName as its value (e.g., "crd" or "crdName") so it conforms to
crossplane-runtime logging.Logger expectations and preserves the existing
control flow (keep the continue).
Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
2 participants
Description of your changes
This PR adds storage version migration support to upjet, enabling automatic migration of CRD resources from old storage versions to new storage versions when CRD schemas are updated.
When a CRD's storage version changes (e.g., from
v1beta1tov1beta2), existing resources stored in etcd remain in the old version until they are explicitly migrated. This can cause issues during upgrades, especially when introducing breaking changes.The storage version migrator automates this migration process by:
status.storedVersionsto reflect only the new storage versionCore Implementation (
pkg/config/crds_migrator.go)CRDsMigrator: Handles storage version migration for a list of GroupVersionKindsRun(): Executes the migration process with pagination support (500 resources per batch)GetCRDNameFromGVK(): Resolves CRD names from GroupVersionKind using discovery clientPrepareCRDsMigrator(): Scans provider resources and creates a migrator for those with previous versionsProvider Integration (
pkg/config/provider.go)StorageVersionMigratorfield to theProviderstructWithStorageVersionMigrator()provider optionExample Usage
In provider configuration (
config/provider.go):In provider main (
cmd/provider/main.go):I have:
make reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR if necessary.How has this code been tested
Tested in this branch: https://github.com/sergenyalcin/provider-upjet-azuread/tree/sv-migrator. Validated the migration.
Summary by CodeRabbit
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.