Base migration framework #119

ulucinar · 2022-10-24T07:13:58Z

Description of your changes

Fixes https://github.com/upbound/team-extensions/issues/29

This PR introduces the base framework for migrating from community providers to official providers.

I have:

Read and followed Crossplane's contribution process.
Run make reviewable to ensure this PR is ready for review.
Add tests for conversion functions, plan generator, etc.
Added backport release-x.y labels to auto-backport this PR if necessary.

How has this code been tested

Manually tested using the implemented sample converter and the following manifests:

apiVersion: apiextensions.crossplane.io/v1
kind: CompositeResourceDefinition
metadata:
  name: xmyresources.test.com
spec:
  claimNames:
    kind: MyResource
    plural: myresources
  group: test.com
  names:
    kind: XMyResource
    plural: xmyresources
  versions:
  - name: v1alpha1
    referenceable: true
    schema:
      openAPIV3Schema:
        properties:
          spec:
            properties:
              parameters:
                properties:
                  tagValue:
                    type: string
                required:
                - tagValue
                type: object
            required:
            - parameters
            type: object
        type: object
    served: true

---

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  labels:
    purpose: example
  name: example
spec:
  compositeTypeRef:
    apiVersion: test.com/v1alpha1
    kind: XMyResource
  resources:
    - base:
        apiVersion: ec2.aws.crossplane.io/v1beta1
        kind: VPC
        spec:
          forProvider:
            cidrBlock: "192.168.0.0/16"
            ipv6Pool: pool1
            region: "West Europe"
            tags:
              - key: key1
                value: val1
              - key: key2
                value: val2
              - key: key3
                value: val3
      name: vpc
      patches:
        - fromFieldPath: "spec.parameters.tagValue"
          toFieldPath: spec.forProvider.tags[0].value
        - fromFieldPath: "spec.parameters.tagValue"
          toFieldPath: spec.forProvider.tags[1].value
        - fromFieldPath: "spec.parameters.tagValue"
          toFieldPath: spec.forProvider.tags[2].value

---

apiVersion: test.com/v1alpha1
kind: MyResource
metadata:
  name: my-resource
  namespace: upbound-system
spec:
  compositionRef:
    name: example
  parameters:
    tagValue: demo-test

---

apiVersion: ec2.aws.crossplane.io/v1beta1
kind: VPC
metadata:
  name: test-vpc
spec:
  forProvider:
    cidrBlock: 192.168.0.0/16
    ipv6Pool: pool1
    region: West Europe
    tags:
      - key: key1
        value: val1
      - key: key2
        value: val2
      - key: key3
        value: val3
status:
  atProvider:
    vpcId: vpc-0391605a3bf7977dd

And the following sample converter for community provider VPC resources (under pkg/migration/converters/vpc_converter.go):

func resources(mg resource.Managed) ([]resource.Managed, error) {
	source := mg.(*srcv1beta1.VPC)
	target := &targetv1beta1.VPC{}
	if _, err := migration.CopyInto(source, target, targetv1beta1.VPC_GroupVersionKind, "spec.forProvider.tags"); err != nil {
		return nil, errors.Wrap(err, "failed to copy source into target")
	}
	target.Spec.ForProvider.Tags = make(map[string]*string, len(source.Spec.ForProvider.Tags))
	for _, t := range source.Spec.ForProvider.Tags {
		v := t.Value
		target.Spec.ForProvider.Tags[t.Key] = &v
	}
	return []resource.Managed{
		target,
	}, nil
}

func composedTemplates(cmp v1.ComposedTemplate, convertedBase ...*v1.ComposedTemplate) error {
	for i, cb := range convertedBase {
		for j, p := range cb.Patches {
			if p.ToFieldPath == nil || !strings.HasPrefix(*p.ToFieldPath, "spec.forProvider.tags") {
				continue
			}
			u, err := migration.FromRawExtension(cmp.Base)
			if err != nil {
				return errors.Wrap(err, "failed to convert ComposedTemplate with VPC base")
			}
			paved := fieldpath.Pave(u.Object)
			key, err := paved.GetString(strings.ReplaceAll(*p.ToFieldPath, ".value", ".key"))
			if err != nil {
				return errors.Wrap(err, "failed to get value from paved")
			}
			s := fmt.Sprintf(`spec.forProvider.tags["%s"]`, key)
			convertedBase[i].Patches[j].ToFieldPath = &s
		}
	}
	return nil
}

The converted resources are as follows:

apiVersion: apiextensions.crossplane.io/v1
kind: Composition
metadata:
  labels:
    purpose: example
  name: example
spec:
  compositeTypeRef:
    apiVersion: test.com/v1alpha1
    kind: XMyResource
  resources:
  - base:
      apiVersion: ec2.aws.upbound.io/v1beta1
      kind: VPC
      spec:
        forProvider:
          cidrBlock: 192.168.0.0/16
          region: West Europe
          tags:
            key1: val1
            key2: val2
            key3: val3
    name: vpc
    patches:
    - fromFieldPath: spec.parameters.tagValue
      toFieldPath: spec.forProvider.tags["key1"]
    - fromFieldPath: spec.parameters.tagValue
      toFieldPath: spec.forProvider.tags["key2"]
    - fromFieldPath: spec.parameters.tagValue
      toFieldPath: spec.forProvider.tags["key3"]

---

apiVersion: ec2.aws.upbound.io/v1beta1
kind: VPC
metadata:
  name: test-vpc
spec:
  forProvider:
    cidrBlock: 192.168.0.0/16
    region: West Europe
    tags:
      key1: val1
      key2: val2
      key3: val3

sergenyalcin

Thanks @ulucinar I left a few comments.

pkg/migration/plan_generator.go

pkg/migration/converter.go

muvaf

Thanks @ulucinar ! Loved how much documentation the code has! I have a couple of non-nit comments but at a high level, it looks pretty good!

muvaf · 2022-11-07T18:58:34Z

pkg/migration/interfaces.go

+	ComposedTemplates(cmp v1.ComposedTemplate, convertedBase ...*v1.ComposedTemplate) error
+}
+
+// Source is a source for reading resource manifests


Do we really need the abstractions over source and target? AFAIK, there are two main scenarios we'd like to handle;

Running against live Kubernetes cluster.

Read resources from cluster and write to the filesystem, prepare a migration plan on filesystem, execute it against the cluster.

Running against manifests stored in a Git repo, i.e. GitOps, hence filesystem.

The files are already on the filesystem, prepare a migration plan on filesystem, skip the execution since the files will be committed.

So, it feels like if we have a seperate utility to get resources from cluster to filesystem that can be turned off by a flag, then the rest of the steps are already normalized to work on the filesystem if I'm not missing something.

muvaf · 2022-11-07T19:00:42Z

pkg/migration/types.go

+	// Example: resources/a.yaml:resources/b.yaml
+	Parents string
+	// IsComposite set if the object belongs to a Composite type
+	IsComposite bool


nit: Category string, which is used in CRDs as well, as single field could help here to classify.

muvaf · 2022-11-07T19:01:38Z

pkg/migration/types.go

+// together with the associated Metadata.
+type UnstructuredWithMetadata struct {
+	Object   unstructured.Unstructured
+	Metadata Metadata


I think Metadata word is overloaded in this context. Can we have another name? Descriptor maybe?

muvaf · 2022-11-07T19:05:57Z

pkg/migration/plan_generator.go

+	pg.Plan.Spec.Steps[stepDeleteOldManaged].Name = "delete-old-managed"
+	pg.Plan.Spec.Steps[stepDeleteOldManaged].Type = StepTypeDelete
+	deletePolicy := FinalizerPolicyRemove
+	pg.Plan.Spec.Steps[stepDeleteOldManaged].Delete = &DeleteStep{


I think we should let the controller do a graceful orphaned deletion so that we stay on the normal path of execution. For example, unpublishing connection details is something done in orphan deletion where we'd not get to have.

Will check this in a future iteration after testing the behavior. Thank you!

muvaf · 2022-11-08T06:48:29Z

pkg/migration/plan_generator.go

+		annot = make(map[string]string)
+	}
+	annot[meta.AnnotationKeyReconciliationPaused] = "true"
+	u.SetAnnotations(annot)


nit: There is meta.AddAnnotations function in runtime that would help here.

muvaf · 2022-11-08T06:49:20Z

pkg/migration/plan_generator.go

+	if err := pg.target.Put(*u); err != nil {
+		return errors.Wrap(err, errResourceOutput)
+	}
+	return nil


Suggested change

if err := pg.target.Put(*u); err != nil {

return errors.Wrap(err, errResourceOutput)

}

return nil

return errors.Wrap(pg.target.Put(*u), errResourceOutput)

nit

muvaf · 2022-11-08T06:52:52Z

pkg/migration/plan_generator.go

+				}
+			}
+		default:
+			if o.Metadata.IsComposite {


nit: handling these IsComposite and IsClaim as cases in switch could make it easier to read.

muvaf · 2022-11-08T06:54:39Z

pkg/migration/converter.go

+// unstructured.Unstructured. Before the converted object is
+// returned, it's sanitized by removing certain fields
+// (like status, metadata.creationTimestamp).
+func ToUnstructured(mg any) unstructured.Unstructured {


Can we name this something like ToSanitizedUnstructured or ToSanitized? It could be misleading otherwise since there is a known ToUnstructured method in upstream libraries.

muvaf · 2022-11-08T06:56:22Z

pkg/migration/registry.go

+
+var (
+	// the default Converter registry
+	registry Registry = make(map[schema.GroupVersionKind]Converter)


nit: I'd lean towards leaving it to the consumer of this package to initialize and maintain their Registry object instead of us providing a global map unless we have to.

Makes sense. Planning to address in the next iteration.

muvaf · 2022-11-08T06:57:02Z

pkg/migration/types.go

+	Path string
+	// colon separated list of parent `Path`s for fan-ins and fan-outs
+	// Example: resources/a.yaml:resources/b.yaml
+	Parents string


I see this field isn't used anywhere. Is it a leftover?

This field is to be employed by the source and target implementations.

Upbound-CLA · 2022-11-23T20:53:43Z

All committers have signed the CLA.

muvaf

Most of my comments were non-blocking, feel free to go ahead! Thanks @ulucinar !

Signed-off-by: Alper Rifat Ulucinar <[email protected]>

pull-request-size bot added the size/L label Oct 24, 2022

ulucinar force-pushed the wearemoving branch from 802c341 to f4a2bcb Compare October 24, 2022 15:31

sergenyalcin mentioned this pull request Oct 24, 2022

Migration source and target implementations #124

Merged

3 tasks

ulucinar force-pushed the wearemoving branch from f4a2bcb to 5511b4b Compare October 25, 2022 10:21

sergenyalcin reviewed Oct 26, 2022

View reviewed changes

pkg/migration/plan_generator.go Outdated Show resolved Hide resolved

pkg/migration/converter.go Show resolved Hide resolved

pkg/migration/converter.go Show resolved Hide resolved

ulucinar force-pushed the wearemoving branch 2 times, most recently from caae7a6 to acbea16 Compare October 28, 2022 15:52

pull-request-size bot added size/XL and removed size/L labels Oct 28, 2022

ulucinar force-pushed the wearemoving branch 12 times, most recently from 8508801 to c0f5204 Compare November 2, 2022 20:40

muvaf mentioned this pull request Nov 3, 2022

Deprecating Terrajet crossplane/terrajet#308

Closed

muvaf reviewed Nov 8, 2022

View reviewed changes

muvaf approved these changes Dec 1, 2022

View reviewed changes

ulucinar added 2 commits December 12, 2022 19:20

Add migration.{Converter,Source,Target} interfaces

ac0125a

Signed-off-by: Alper Rifat Ulucinar <[email protected]>

Add plan generator

4b0a674

Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar force-pushed the wearemoving branch 3 times, most recently from 2b26008 to 81b18ac Compare December 13, 2022 22:06

ulucinar force-pushed the wearemoving branch 3 times, most recently from d1b3cc2 to af81b4a Compare December 13, 2022 22:21

Add migration tests framework and migration package tests

81c9dbe

Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar force-pushed the wearemoving branch from af81b4a to 81c9dbe Compare December 13, 2022 22:32

jeanduplessis merged commit 52337e6 into crossplane:main Dec 14, 2022

ulucinar deleted the wearemoving branch December 14, 2022 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Base migration framework #119

Base migration framework #119

ulucinar commented Oct 24, 2022 •

edited

Loading

sergenyalcin left a comment

muvaf left a comment

muvaf Nov 7, 2022

muvaf Nov 7, 2022

muvaf Nov 7, 2022

muvaf Nov 7, 2022

ulucinar Dec 13, 2022

muvaf Nov 8, 2022

ulucinar Dec 13, 2022

muvaf Nov 8, 2022

ulucinar Dec 13, 2022

muvaf Nov 8, 2022

muvaf Nov 8, 2022

ulucinar Dec 13, 2022

muvaf Nov 8, 2022

ulucinar Dec 13, 2022

muvaf Nov 8, 2022

ulucinar Dec 13, 2022

Upbound-CLA commented Nov 23, 2022 •

edited

Loading

muvaf left a comment

Base migration framework #119

Base migration framework #119

Conversation

ulucinar commented Oct 24, 2022 • edited Loading

Description of your changes

How has this code been tested

sergenyalcin left a comment

Choose a reason for hiding this comment

muvaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Upbound-CLA commented Nov 23, 2022 • edited Loading

muvaf left a comment

Choose a reason for hiding this comment

ulucinar commented Oct 24, 2022 •

edited

Loading

Upbound-CLA commented Nov 23, 2022 •

edited

Loading