-
Notifications
You must be signed in to change notification settings - Fork 1.4k
✨ feat: add experimental clusterctl migrate command
#12843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,189 @@ | ||||||
| /* | ||||||
| Copyright 2025 The Kubernetes Authors. | ||||||
|
|
||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| you may not use this file except in compliance with the License. | ||||||
| You may obtain a copy of the License at | ||||||
|
|
||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||
|
|
||||||
| Unless required by applicable law or agreed to in writing, software | ||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| See the License for the specific language governing permissions and | ||||||
| limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package cmd | ||||||
|
|
||||||
| import ( | ||||||
| "fmt" | ||||||
| "io" | ||||||
| "os" | ||||||
| "strings" | ||||||
|
|
||||||
| "github.com/pkg/errors" | ||||||
| "github.com/spf13/cobra" | ||||||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||||||
|
|
||||||
| clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" | ||||||
| "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/migrate" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The migrate command should not be implemented in an internal packages. All clusterctl commands should be implemented as a single method in the clusterctl Client interface so they can be used both from the cmd as well as from users leveraging to clusterctl as a library This also implies that most of the content of runMigrate must be moved into the method above. The single method in the clustectl library can then use subcomponents from client subpacages. |
||||||
| "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme" | ||||||
| ) | ||||||
|
|
||||||
| type migrateOptions struct { | ||||||
| output string | ||||||
| toVersion string | ||||||
| } | ||||||
|
|
||||||
| var migrateOpts = &migrateOptions{} | ||||||
|
|
||||||
| var supportedTargetVersions = []string{ | ||||||
| clusterv1.GroupVersion.Version, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't there a programmatic way to enumerate these instead of requiring the OWNERS to manually add a new version here? e.g. core k8s types manage that in the internal types for a given group, but in CAPI the layout is different.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TBH I don't think it is necessary to automate managing this list, I would prefer to avoid unnecessary complexity
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The list should be defined only once, and it should be defined into the library, not at the cmd level |
||||||
| } | ||||||
|
|
||||||
| var migrateCmd = &cobra.Command{ | ||||||
| Use: "migrate [SOURCE]", | ||||||
| Short: "EXPERIMENTAL: Migrate cluster.x-k8s.io resources between API versions", | ||||||
| Long: `EXPERIMENTAL: Migrate cluster.x-k8s.io resources between API versions. | ||||||
|
Comment on lines
+45
to
+48
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we should position this command as "clusterctl config migrate" instead of "clusterctl migrate", for a ca reasons:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wdyt is this enough to make clear that we are just converting YAML files? I saw so much confusion already about how apiVersions work that I wonder if that command will make some folks think they have to run the migration when bumping to v1beta2. Especially as we have components like CRD migrator in CAPI Should we rather use conversion instead of migration? (It's using our conversion and not our migration code, also apiserver calls this conversion)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. clusterctl convert yaml also works for me (e.g we already have clusterctl generate yaml)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
|
|
||||||
| This command is EXPERIMENTAL and may be removed in a future release! | ||||||
|
|
||||||
| Scope and limitations: | ||||||
| - Only cluster.x-k8s.io resources are converted | ||||||
| - Other CAPI API groups are passed through unchanged | ||||||
| - ClusterClass patches are not migrated | ||||||
| - Field order may change and comments will be removed in output | ||||||
| - API version references are dropped during conversion (except ClusterClass and external | ||||||
| remediation references) | ||||||
|
|
||||||
| Examples: | ||||||
| # Migrate from file to stdout | ||||||
| clusterctl migrate cluster.yaml | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would migrate from new to old be supported? |
||||||
|
|
||||||
| # Migrate from stdin to stdout | ||||||
| cat cluster.yaml | clusterctl migrate | ||||||
|
|
||||||
| # Explicitly specify target <VERSION> | ||||||
| clusterctl migrate cluster.yaml --to-version <VERSION> --output migrated-cluster.yaml`, | ||||||
|
|
||||||
| Args: cobra.MaximumNArgs(1), | ||||||
| RunE: func(_ *cobra.Command, args []string) error { | ||||||
| return runMigrate(args) | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| func init() { | ||||||
| migrateCmd.Flags().StringVarP(&migrateOpts.output, "output", "o", "", "Output file path (default: stdout)") | ||||||
| migrateCmd.Flags().StringVar(&migrateOpts.toVersion, "to-version", clusterv1.GroupVersion.Version, fmt.Sprintf("Target API version for migration (supported: %s)", strings.Join(supportedTargetVersions, ", "))) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| RootCmd.AddCommand(migrateCmd) | ||||||
| } | ||||||
|
|
||||||
| func isSupportedTargetVersion(version string) bool { | ||||||
| for _, v := range supportedTargetVersions { | ||||||
| if v == version { | ||||||
| return true | ||||||
| } | ||||||
| } | ||||||
| return false | ||||||
| } | ||||||
|
|
||||||
| func runMigrate(args []string) error { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of the code in this func must be moved to the client interface method, all except:
|
||||||
| if !isSupportedTargetVersion(migrateOpts.toVersion) { | ||||||
| return errors.Errorf("invalid --to-version value %q: supported versions are %s", migrateOpts.toVersion, strings.Join(supportedTargetVersions, ", ")) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| } | ||||||
|
|
||||||
| fmt.Fprint(os.Stderr, "WARNING: This command is EXPERIMENTAL and may be removed in a future release!") | ||||||
|
|
||||||
| var input io.Reader | ||||||
| var inputName string | ||||||
|
|
||||||
| if len(args) == 0 { | ||||||
| input = os.Stdin | ||||||
| inputName = "stdin" | ||||||
| } else { | ||||||
| sourceFile := args[0] | ||||||
| // #nosec G304 | ||||||
| // command accepts user-provided file path by design | ||||||
| file, err := os.Open(sourceFile) | ||||||
| if err != nil { | ||||||
| return errors.Wrapf(err, "failed to open input file %q", sourceFile) | ||||||
| } | ||||||
| defer file.Close() | ||||||
| input = file | ||||||
| inputName = sourceFile | ||||||
| } | ||||||
|
|
||||||
| // Determine output destination | ||||||
| var output io.Writer | ||||||
| var outputFile *os.File | ||||||
| var err error | ||||||
|
|
||||||
| if migrateOpts.output == "" { | ||||||
| output = os.Stdout | ||||||
| } else { | ||||||
| outputFile, err = os.Create(migrateOpts.output) | ||||||
| if err != nil { | ||||||
| return errors.Wrapf(err, "failed to create output file %q", migrateOpts.output) | ||||||
| } | ||||||
| defer outputFile.Close() | ||||||
| output = outputFile | ||||||
| } | ||||||
|
|
||||||
| // Create migration engine components | ||||||
| parser := migrate.NewYAMLParser(scheme.Scheme) | ||||||
|
|
||||||
| targetGV := schema.GroupVersion{ | ||||||
| Group: clusterv1.GroupVersion.Group, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is ok, but it hardcodes the group to the group of an imported version package. |
||||||
| Version: migrateOpts.toVersion, | ||||||
| } | ||||||
|
|
||||||
| converter, err := migrate.NewConverter(targetGV) | ||||||
| if err != nil { | ||||||
| return errors.Wrap(err, "failed to create converter") | ||||||
| } | ||||||
|
|
||||||
| engine, err := migrate.NewEngine(parser, converter) | ||||||
| if err != nil { | ||||||
| return errors.Wrap(err, "failed to create migration engine") | ||||||
| } | ||||||
|
|
||||||
| opts := migrate.MigrationOptions{ | ||||||
| Input: input, | ||||||
| Output: output, | ||||||
| Errors: os.Stderr, | ||||||
| ToVersion: migrateOpts.toVersion, | ||||||
| } | ||||||
|
|
||||||
| result, err := engine.Migrate(opts) | ||||||
| if err != nil { | ||||||
| return errors.Wrap(err, "migration failed") | ||||||
| } | ||||||
|
|
||||||
| if result.TotalResources > 0 { | ||||||
| fmt.Fprintf(os.Stderr, "\nMigration completed:\n") | ||||||
| fmt.Fprintf(os.Stderr, " Total resources processed: %d\n", result.TotalResources) | ||||||
| fmt.Fprintf(os.Stderr, " Resources converted: %d\n", result.ConvertedCount) | ||||||
| fmt.Fprintf(os.Stderr, " Resources skipped: %d\n", result.SkippedCount) | ||||||
|
|
||||||
| if result.ErrorCount > 0 { | ||||||
| fmt.Fprintf(os.Stderr, " Resources with errors: %d\n", result.ErrorCount) | ||||||
| } | ||||||
|
|
||||||
| if len(result.Warnings) > 0 { | ||||||
| fmt.Fprintf(os.Stderr, " Warnings: %d\n", len(result.Warnings)) | ||||||
| } | ||||||
|
|
||||||
| fmt.Fprintf(os.Stderr, "\nSource: %s\n", inputName) | ||||||
| if migrateOpts.output != "" { | ||||||
| fmt.Fprintf(os.Stderr, "Output: %s\n", migrateOpts.output) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if result.ErrorCount > 0 { | ||||||
| return errors.Errorf("migration completed with %d errors", result.ErrorCount) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would that ever be reached? |
||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,166 @@ | ||||||
| /* | ||||||
| Copyright 2025 The Kubernetes Authors. | ||||||
|
|
||||||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||||||
| you may not use this file except in compliance with the License. | ||||||
| You may obtain a copy of the License at | ||||||
|
|
||||||
| http://www.apache.org/licenses/LICENSE-2.0 | ||||||
|
|
||||||
| Unless required by applicable law or agreed to in writing, software | ||||||
| distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| See the License for the specific language governing permissions and | ||||||
| limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package migrate | ||||||
|
|
||||||
| import ( | ||||||
| "fmt" | ||||||
|
|
||||||
| "github.com/pkg/errors" | ||||||
| "k8s.io/apimachinery/pkg/runtime" | ||||||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||||||
| "sigs.k8s.io/controller-runtime/pkg/conversion" | ||||||
|
|
||||||
| clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2" | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the cluster-api GVKs being source or targets must be opaque to the converter as a backend. these can be defined as GV and GVKs passed to the Engine (Migrator) and the Converter. |
||||||
| "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme" | ||||||
| ) | ||||||
|
|
||||||
| // Converter handles conversion of individual CAPI resources between API versions. | ||||||
| type Converter struct { | ||||||
| scheme *runtime.Scheme | ||||||
| targetGV schema.GroupVersion | ||||||
| targetGVKMap gvkConversionMap | ||||||
| } | ||||||
|
|
||||||
| // gvkConversionMap caches conversions from a source GroupVersionKind to its target GroupVersionKind. | ||||||
| type gvkConversionMap map[schema.GroupVersionKind]schema.GroupVersionKind | ||||||
|
|
||||||
| // ConversionResult represents the outcome of converting a single resource. | ||||||
| type ConversionResult struct { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also in this case, either we convert or we fail hard. |
||||||
| Object runtime.Object | ||||||
| // Converted indicates whether the object was actually converted | ||||||
| Converted bool | ||||||
| Error error | ||||||
| Warnings []string | ||||||
|
Comment on lines
+43
to
+47
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if you are adding a go comment for one of the fields best to add for all of them. |
||||||
| } | ||||||
|
|
||||||
| // NewConverter creates a new resource converter using the clusterctl scheme. | ||||||
| func NewConverter(targetGV schema.GroupVersion) (*Converter, error) { | ||||||
| return &Converter{ | ||||||
| scheme: scheme.Scheme, | ||||||
| targetGV: targetGV, | ||||||
| targetGVKMap: make(gvkConversionMap), | ||||||
| }, nil | ||||||
| } | ||||||
|
|
||||||
| // ConvertResource converts a single resource to the target version. | ||||||
| // Returns the converted object, or the original if no conversion is needed. | ||||||
| func (c *Converter) ConvertResource(info ResourceInfo, obj runtime.Object) ConversionResult { | ||||||
| gvk := info.GroupVersionKind | ||||||
|
|
||||||
| if gvk.Group == clusterv1.GroupVersion.Group && gvk.Version == c.targetGV.Version { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I wrong or this should never happen considering we are calling func this only when dealing with ResourceTypeCoreV1Beta1 objects? If this is the case, either drop or return an error in this case |
||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Warnings: []string{fmt.Sprintf("Resource %s/%s is already at version %s", gvk.Kind, info.Name, c.targetGV.Version)}, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| if gvk.Group != clusterv1.GroupVersion.Group { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I wrong or this should never happen considering we are calling func this only when dealing with ResourceTypeCoreV1Beta1 objects? |
||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Warnings: []string{fmt.Sprintf("Skipping non-%s resource: %s", clusterv1.GroupVersion.Group, gvk.String())}, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| targetGVK, err := c.getTargetGVK(gvk) | ||||||
| if err != nil { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I wrong or this should never happen considering we are calling func this only when dealing with ResourceTypeCoreV1Beta1 objects? |
||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Error: errors.Wrapf(err, "failed to determine target GVK for %s", gvk.String()), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Check if the object is already typed | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add dots at the end of comment sentences consistently across the diff. some don't have them. |
||||||
| // If it's typed and implements conversion.Convertible, use the custom ConvertTo method | ||||||
| if convertible, ok := obj.(conversion.Convertible); ok { | ||||||
| // Create a new instance of the target type | ||||||
| targetObj, err := c.scheme.New(targetGVK) | ||||||
| if err != nil { | ||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Error: errors.Wrapf(err, "failed to create target object for %s", targetGVK.String()), | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
to be less confusing which input caused this. |
||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Check if the target object is a Hub | ||||||
| if hub, ok := targetObj.(conversion.Hub); ok { | ||||||
| if err := convertible.ConvertTo(hub); err != nil { | ||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Error: errors.Wrapf(err, "failed to convert %s from %s to %s", gvk.Kind, gvk.Version, c.targetGV.Version), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Ensure the GVK is set on the converted object | ||||||
| hubObj := hub.(runtime.Object) | ||||||
| hubObj.GetObjectKind().SetGroupVersionKind(targetGVK) | ||||||
|
|
||||||
| return ConversionResult{ | ||||||
| Object: hubObj, | ||||||
| Converted: true, | ||||||
| Error: nil, | ||||||
| Warnings: nil, | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Use scheme-based conversion for all remaining cases | ||||||
| convertedObj, err := c.scheme.ConvertToVersion(obj, targetGVK.GroupVersion()) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would say we should fail hard if we are dealing with a core object that does not support convertible |
||||||
| if err != nil { | ||||||
| return ConversionResult{ | ||||||
| Object: obj, | ||||||
| Converted: false, | ||||||
| Error: errors.Wrapf(err, "failed to convert %s from %s to %s", gvk.Kind, gvk.Version, c.targetGV.Version), | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| return ConversionResult{ | ||||||
| Object: convertedObj, | ||||||
| Converted: true, | ||||||
| Error: nil, | ||||||
| Warnings: nil, | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // getTargetGVK returns the target GroupVersionKind for a given source GVK. | ||||||
| func (c *Converter) getTargetGVK(sourceGVK schema.GroupVersionKind) (schema.GroupVersionKind, error) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can probably be entirely dropped as soon as we move supportedTargetVersions into this package |
||||||
| // Check cache first | ||||||
| if targetGVK, ok := c.targetGVKMap[sourceGVK]; ok { | ||||||
| return targetGVK, nil | ||||||
| } | ||||||
|
|
||||||
| // Create target GVK with same kind but target version | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| targetGVK := schema.GroupVersionKind{ | ||||||
| Group: c.targetGV.Group, | ||||||
| Version: c.targetGV.Version, | ||||||
| Kind: sourceGVK.Kind, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this assumes that that a Kind exists in both the source and target GV. is there are better way to manage this? |
||||||
| } | ||||||
|
|
||||||
| // Verify the target type exists in the scheme | ||||||
| if !c.scheme.Recognizes(targetGVK) { | ||||||
| return schema.GroupVersionKind{}, errors.Errorf("target GVK %s not recognized by scheme", targetGVK.String()) | ||||||
| } | ||||||
|
|
||||||
| // Cache for future use | ||||||
| c.targetGVKMap[sourceGVK] = targetGVK | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note, the converter can be done slightly differently, but this is not a blocker. |
||||||
|
|
||||||
| return targetGVK, nil | ||||||
| } | ||||||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add new line between these two.
k8s vs non-k8s sources.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently, linter is not happy with the suggested change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's odd. linters normally allow you to define as many 'groups' of imports as you like.