Make CRD version configurable #266

ulucinar · 2022-01-17T17:45:12Z

Fixes #1089
Adds config.ResourceConfig.APIVersion to be able to
override the generated version for a CRD

Issue #, if available: #1089

Description of changes:
This PR proposes a new configuration field named APIVersion (JSON-serialized name api_version) in config.ResourceConfig that allows overriding versions of specific CRDs generated by the Crossplane codegen pipeline. If a specific version string is not configured for a CRD-type, the default version specified with the --version command-line argument is used for backwards-compatibility. An example configuration overriding the version for the CloudFront Distribution resource is as follows:

resources:
  "Distribution":
    api_versions:
    - name: "v1beta1"
      storage: true
ignore:
  resource_names:
    - FieldLevelEncryptionProfile
    - Invalidation
    - KeyGroup
    - OriginRequestPolicy
    - PublicKey
    - StreamingDistribution
    - RealtimeLogConfig
    - MonitoringSubscription
    - FieldLevelEncryptionConfig
  field_paths:
    - DistributionConfig.CallerReference
    - Origins.Quantity
    - OriginAccessIdentityConfig.CallerReference

This PR now introduces a backward-incompatible behavior change in the Crossplane pipeline:
Because the generator configuration is per API group (service) and because we may now have resources belonging to different API versions in a single group, this PR changes the default path of the generator-config.yaml file from:
apis/<API group>/<API version>/generator-config.yaml
to
apis/<API group>/generator-config.yaml

An example invocation ack-generate crossplane cloudfront --output <output path> --generator-config-path generator-config.yaml using the above configuration yields the following file structure:

❯ tree
.
├── apis
│   └── cloudfront
│       ├── v1alpha1
│       │   ├── zz_cache_policy.go
│       │   ├── zz_cloud_front_origin_access_identity.go                                                                                [0/941]
│       │   ├── zz_doc.go
│       │   ├── zz_enums.go
│       │   ├── zz_function.go
│       │   ├── zz_groupversion_info.go
│       │   ├── zz_response_headers_policy.go
│       │   └── zz_types.go
│       └── v1beta1
│           ├── zz_distribution.go
│           ├── zz_doc.go
│           ├── zz_enums.go
│           ├── zz_groupversion_info.go
│           └── zz_types.go
├── generator-config.yaml
├── go.mod
├── go.sum
└── pkg
    └── controller
        └── cloudfront
            ├── cachepolicy
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── cloudfrontoriginaccessidentity
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── distribution
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── function
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            └── responseheaderspolicy
                ├── zz_controller.go
                └── zz_conversions.go

12 directories, 26 files

Without the resources configuration block, all resources are under v1alpha1 version as expected:

❯ tree .
.
├── apis                                                                                                                               [0/1036]
│   └── cloudfront
│       └── v1alpha1
│           ├── zz_cache_policy.go
│           ├── zz_cloud_front_origin_access_identity.go
│           ├── zz_distribution.go
│           ├── zz_doc.go
│           ├── zz_enums.go
│           ├── zz_function.go
│           ├── zz_groupversion_info.go
│           ├── zz_response_headers_policy.go
│           └── zz_types.go
├── generator-config.yaml
├── go.mod
├── go.sum
└── pkg
    └── controller
        └── cloudfront
            ├── cachepolicy
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── cloudfrontoriginaccessidentity
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── distribution
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            ├── function
            │   ├── zz_controller.go
            │   └── zz_conversions.go
            └── responseheaderspolicy
                ├── zz_controller.go
                └── zz_conversions.go

11 directories, 22 files

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

- Fixes #1089 - Adds config.ResourceConfig.APIVersion to be able to override the generated version for a CRD Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ack-bot · 2022-01-17T17:45:21Z

Hi @ulucinar. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vijtrip2 · 2022-01-18T19:01:19Z

/ok-to-test

vijtrip2 · 2022-01-18T19:14:59Z

/approve

Looks good but I will let @jaypipes approve on new GeneratorConfig field name.
And I will also let @a-hilaly to approve as he is working on supporting multi-version CRs for ACK controllers.

a-hilaly · 2022-01-19T11:47:59Z

Looks like this change is only impacting ackgenerate crossplane and doesn't touch any other components. I'm fine with merging this if it's solving crossplane issue and satisfies their versioning criterias. However ACK and crossplane CRD versioning philosophies will take different paths.
I pinged @ulucinar and @muvaf on slack to chat about this subject
/hold for now

ulucinar · 2022-01-19T20:18:02Z

Hi @a-hilaly,
Thank you for taking a look at this PR. I saw some existing work around supporting multiple versions for the generated CRDs, and I think you are referring to that work. This implementation follows @muvaf's suggestion here. Let's get together as you suggested to discuss the path we would like to take. My understanding is that in the Crossplane pipeline, for the first phase, we (currently) need to be able to control the API version for the generated CRDs with per-resource configuration, and multiple versions per CRD is not currently in the scope of #1089. Let's discuss when @muvaf is back.

a-hilaly · 2022-01-20T12:02:48Z

Thanks @ulucinar ! Meeting scheduled tomorrow with Upbound folks @ulucinar and @muvaf.
Unholding the PR
/unhold

ack-bot · 2022-01-20T12:10:00Z

@a-hilaly: The /retest command does not accept any targets.
The following commands are available to trigger jobs:

/test unit-test
/test s3-olm-test
/test dynamodb-controller-test
/test ecr-controller-test
/test s3-controller-test

Use /test all to run all jobs.

In response to this:

/retest all

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

a-hilaly · 2022-01-20T12:10:15Z

/test all

a-hilaly

LGTM! left one nit comment

a-hilaly · 2022-01-20T12:11:49Z

pkg/generate/crossplane/crossplane.go

 	)

 	metaVars := m.MetaVars()
+	detectedCRDVersions := make(map[string]struct{})


nit: s/struct{}/bool/

Resolved. Thank you.

a-hilaly · 2022-01-20T12:24:45Z

/test s3-controller-test

- Allow multiple CRD versions to be configured for the generated CRDs Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar · 2022-01-24T22:18:35Z

@muvaf, as detailed in the description of this PR, we will need to move generator-config.yamls to unversioned API group roots. And for resources with API version configurations, we will generate CRDs at the storage API version.

- Consume aws-controllers-k8s/code-generator#266 that implements support for specifying API versions of the generated CRDs. - Move generator-config.yaml files to unversioned API group parent folders, because an API Group may now contain multiple versions. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

a-hilaly · 2022-01-26T18:08:30Z

/test all

- Consume aws-controllers-k8s/code-generator#266 that implements support for specifying API versions of the generated CRDs. - Move generator-config.yaml files to unversioned API group parent folders, because an API Group may now contain multiple versions. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

- Till aws-controllers-k8s/code-generator#266 is merged. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

a-hilaly · 2022-01-28T14:50:16Z

Spoken with @muvaf and @ulucinar about this PR. Will make changes in #122. Merging to unblock crossplane folks for now.
/lgtm

ack-bot · 2022-01-28T14:50:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: A-Hilaly, ulucinar, vijtrip2

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [A-Hilaly,vijtrip2]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ulucinar · 2022-01-28T14:58:21Z

Thank you @a-hilaly, very much appreciated!

- Consume aws-controllers-k8s/code-generator#266 that implements support for specifying API versions of the generated CRDs. - Move generator-config.yaml files to unversioned API group parent folders, because an API Group may now contain multiple versions. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

Make CRD version configurable

1d2320f

- Fixes #1089 - Adds config.ResourceConfig.APIVersion to be able to override the generated version for a CRD Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ack-bot requested review from RedbackThomson and vijtrip2 January 17, 2022 17:45

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 17, 2022

ack-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 18, 2022

ack-bot added the approved label Jan 18, 2022

vijtrip2 requested review from a-hilaly and jaypipes January 18, 2022 19:15

ack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2022

ack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2022

a-hilaly approved these changes Jan 20, 2022

View reviewed changes

config.ResourceConfig.APIVersions is now a slice

62aa766

- Allow multiple CRD versions to be configured for the generated CRDs Signed-off-by: Alper Rifat Ulucinar <[email protected]>

ulucinar force-pushed the fix-1089 branch from 427bad8 to 62aa766 Compare January 24, 2022 18:05

ulucinar mentioned this pull request Jan 26, 2022

Move versioned generator configs to API group folders crossplane-contrib/provider-aws#1096

Merged

2 tasks

ulucinar added a commit to ulucinar/provider-aws that referenced this pull request Jan 28, 2022

Consume ACK code-generator temporarily from external repo

07b0125

- Till aws-controllers-k8s/code-generator#266 is merged. Signed-off-by: Alper Rifat Ulucinar <[email protected]>

a-hilaly approved these changes Jan 28, 2022

View reviewed changes

ack-bot assigned a-hilaly Jan 28, 2022

ack-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2022

ack-bot merged commit 3f6e80a into aws-controllers-k8s:main Jan 28, 2022

ulucinar deleted the fix-1089 branch January 28, 2022 14:57

muvaf mentioned this pull request Jan 28, 2022

Ability to provide API version as resource-specific configuration aws-controllers-k8s/community#1089

Closed

Make CRD version configurable #266

Make CRD version configurable #266

Uh oh!

Conversation

ulucinar commented Jan 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Jan 17, 2022

Uh oh!

vijtrip2 commented Jan 18, 2022

Uh oh!

vijtrip2 commented Jan 18, 2022

Uh oh!

a-hilaly commented Jan 19, 2022

Uh oh!

ulucinar commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly commented Jan 20, 2022

Uh oh!

ack-bot commented Jan 20, 2022

Uh oh!

a-hilaly commented Jan 20, 2022

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

a-hilaly Jan 20, 2022

Choose a reason for hiding this comment

Uh oh!

ulucinar Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Jan 20, 2022

Uh oh!

ulucinar commented Jan 24, 2022

Uh oh!

a-hilaly commented Jan 26, 2022

Uh oh!

a-hilaly commented Jan 28, 2022

Uh oh!

ack-bot commented Jan 28, 2022

Uh oh!

ulucinar commented Jan 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ulucinar commented Jan 17, 2022 •

edited

Loading

ulucinar commented Jan 19, 2022 •

edited

Loading