Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Multi-Version CRs support #835

Open
7 of 13 tasks
a-hilaly opened this issue Jun 15, 2021 · 8 comments
Open
7 of 13 tasks

Multi-Version CRs support #835

a-hilaly opened this issue Jun 15, 2021 · 8 comments
Assignees
@a-hilaly
Labels
area/multi-version Issues or PRs related to multi version support kind/enhancement Categorizes issue or PR as related to existing feature enhancements. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@a-hilaly
Copy link
Member

a-hilaly commented Jun 15, 2021 •
edited
Loading

Context

All ACK controllers start out with an Alpha API version (v1alpha1) that evolves from release to release, most likely due to updates in aws/aws-sdk-go OpenAPI Schemas or because of changes in the generator.yaml (ignored fields, renamed fields, field made secrets etc...)
Eventually most of these APIs will need to move to a more stable API ( v1beta1, v1beta2... v1...) and restricted from making breaking changes to it.

However to simplify ACK users experience when dealing with controllers/CRDs upgrades we need to keep support for multiple API versions (if not all of them) and gradually deprecate old versions when needed (see kubernetes deprecation policy)

Implementation details:

Changes to ack/code-generator
  • ack-generate should be able to generate API definitions for different ApiVersions (currently supported with --api-version flag)
  • ack-generate apis should save a copy of generator.yaml within the ApiVersions directory (apis/v*/)
  • ack-generate should save the parameters used to generate a specific ApiVersion (aws-sdk-go, ack--generate versions, ENV Vars, etc ...) see Proposal: code generation output metadata #809
  • controller flags should allow to enable webhook server and set it's host and port
  • runtime should contain a package to systematize webhook registration and listing
  • ack-generate controller should generate code that registers conversion webhooks on controller startup (probably done in main.go) Setup webhooks on controller startup code-generator#111
  • ack-generate should start using gogit to load/checkout aws-sdk-go specific versions Use go-git functionalities for common git operations code-generator#108
  • ack-generate controller should generate conversion functions by loading all aws-sdk-go version schemas and their related generator.yaml and computing the CRD "type diffs"
  • Change the scripts/build-controller.sh to call controller-gen with specific arguments when dealing with multiple versions
  • Change the scripts/release-controller.sh to include the new CRD Schemas and optionally create a CA (Certificate Authority) service
Documentation:
  • Document how code generation works for multiple version works (conversion functions, webhooks, Auth/CA service)
  • Document how does a controller support two or more API versions (data flow description + diagram)
  • Document the required steps from K8s cluster administrators to setup multi-version ACK controllers

/cc @aws-controllers-k8s/code-generator-maintainer @aws-controllers-k8s/runtime-maintainer @aws-controllers-k8s/community-maintainers
@a-hilaly a-hilaly added the kind/enhancement Categorizes issue or PR as related to existing feature enhancements. label Jun 15, 2021
@RedbackThomson
Copy link
Contributor

we need to keep support for multiple API versions

Would you mind clarifying what the Kubernetes definition of "support for" means in this context? Is it simply that we are able to convert to/from them, or are there stronger definitions about their usage?

ack-generate should start using gogit to load/checkout aws-sdk-go specific versions

Is this related to versioning, or just an aside?

create a CA (Certificate Authority) service

What purpose does a CA serve for CRDs?

Is there also some way we can ensure we do not modify a previous version of a released CRD? That is, if we've released v1alpha2 we must somehow lock v1alpha1 to a given version.

@a-hilaly
Copy link
Member Author

Would you mind clarifying what the Kubernetes definition of "support for" means in this context? Is it simply that we are able to convert to/from them, or are there stronger definitions about their usage?

Yes, simply that we will be able to convert to/from them, by always regenerating conversion functions whenever there is a CR version bump (latest version is always the "Hub" version)

Is this related to versioning, or just an aside?

Not directly related to versioning. It's gonna simplify the way we load AWS API Specifications from aws-sdk-go

What purpose does a CA serve for CRDs?

The CA will issue certificates to ensure secure communication between the kube-apiserver and a given ACK controller. The webhook server will be running within the service controller (https://github.com/kubernetes-sigs/controller-runtime/blob/master/alias.go#L103-L104)

Is there also some way we can ensure we do not modify a previous version of a released CRD?

We can do that by saving an output file containing the files checksum whenever we regenerate an API version. However conversion functions will always change whenever we update/create a new API version - so maybe we can just exclude the conversion functions when computing the checksum ?

@a-hilaly a-hilaly self-assigned this Jun 17, 2021
This was referenced Jun 25, 2021
Merged
Merged
Merged
ack-bot pushed a commit to aws-controllers-k8s/runtime that referenced this issue Jun 25, 2021
@a-hilaly
Add two controller flags to enable and configure the webhook server (#25
c8458a2 
)

Part of aws-controllers-k8s/community#835

Introduces two new controller flags:
- `--enable-webhook-server` a boolean flag instructing whether to spin a
webhook server or not. Default is `false`
- `--webhook-server-addr` a string flag used to configure the webhook
serving address. Default is `0.0.0.0:443`

This patch also run golint against `pkg/config/config.go`

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Jun 25, 2021
@a-hilaly
Copy generator.yaml to apis/version directory (#109)
81045f0 
Part of aws-controllers-k8s/community#835

Adding some logic to create copies of the `generator.yaml` within the
apis versions directories. These generator.yaml files we be reused by
the conversion webhook generator to understand the difference between
each two different API versions.

This patch Also adds a new utility function to copy files from a
source to a destination path.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@a-hilaly a-hilaly mentioned this issue Jun 28, 2021
Merged
ack-bot pushed a commit to aws-controllers-k8s/code-generator that referenced this issue Jun 30, 2021
@a-hilaly
Use go-git functionalities for common git operations (#108)
9fec5f0 
Part of aws-controllers-k8s/community#835

This patch updates all the helper functions to use `go-git` library
instead of having to call `os/exec.Command`/`os/exec.CombinedOutput` in
order to clone repository, fetch tags... These changes brings more
dependencies to our `go.mod` but in exchange it simplifies the way we
interact with git repositories and improves errors handling and
readability.

Future changes will also benefit from this patch, especially
conversation webhook generator that will need to checkout multiple
version tags.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
ryansteakley added a commit to ryansteakley/code-generator that referenced this issue Jul 1, 2021
@ryansteakley
# This is a combination of 2 commits.
2e99b5d 
# This is the 1st commit message:

add new field to pkg/generate/config.go so user can implement custom requiredFieldMissingFromReadOneInput

# This is the commit message aws-controllers-k8s#2:

Use `go-git` functionalities for common git operations (aws-controllers-k8s#108)

Part of aws-controllers-k8s/community#835

This patch updates all the helper functions to use `go-git` library
instead of having to call `os/exec.Command`/`os/exec.CombinedOutput` in
order to clone repository, fetch tags... These changes brings more
dependencies to our `go.mod` but in exchange it simplifies the way we
interact with git repositories and improves errors handling and
readability.

Future changes will also benefit from this patch, especially
conversation webhook generator that will need to checkout multiple
version tags.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
This was referenced Jul 6, 2021
Merged
Closed
RedbackThomson pushed a commit to RedbackThomson/ack-code-generator that referenced this issue Jul 8, 2021
@a-hilaly
Use go-git functionalities for common git operations (aws-controlle…
d4d6cc1 
…rs-k8s#108)

Part of aws-controllers-k8s/community#835

This patch updates all the helper functions to use `go-git` library
instead of having to call `os/exec.Command`/`os/exec.CombinedOutput` in
order to clone repository, fetch tags... These changes brings more
dependencies to our `go.mod` but in exchange it simplifies the way we
interact with git repositories and improves errors handling and
readability.

Future changes will also benefit from this patch, especially
conversation webhook generator that will need to checkout multiple
version tags.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@vijtrip2 vijtrip2 mentioned this issue Aug 25, 2021
Closed
@ack-bot ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 1, 2021
@a-hilaly
Copy link
Member Author

a-hilaly commented Dec 1, 2021

/remove-lifecycle stale

@ack-bot ack-bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 1, 2021
@jaypipes jaypipes added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 14, 2022
@muvaf
Copy link
Contributor

muvaf commented Jan 20, 2022

@a-hilaly If I understand correctly, the apiVersion is specified per-group, right? Not per-CRD? In Crossplane, we go for per-CRD to comply with Kubernetes and also CRDs in the same group usually don't have the same maturity level. Do you see that case arising in ACK as well or do you consider the whole group as a unit that will be versioned together?

@ack-bot
Copy link
Collaborator

ack-bot commented Apr 20, 2022

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

@ack-bot ack-bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 20, 2022
@vijtrip2
Copy link
Contributor

/lifecycle frozen

@ack-bot ack-bot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 20, 2022
@vijtrip2 vijtrip2 mentioned this issue Jun 27, 2022
Open
@a-hilaly a-hilaly mentioned this issue Aug 13, 2022
Open
@a-hilaly a-hilaly pinned this issue Sep 9, 2022
@a-hilaly a-hilaly moved this to In Progress in ACK Core Team Sep 9, 2022
@a-hilaly a-hilaly added priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Sep 9, 2022
@jaypipes jaypipes added the area/multi-version Issues or PRs related to multi version support label Nov 11, 2022
@jljaco
Copy link
Contributor

jljaco commented Nov 11, 2022

/remove-lifecycle stale

@jaypipes jaypipes mentioned this issue Nov 11, 2022
Open
@jljaco jljaco unpinned this issue Feb 1, 2023
@jljaco jljaco moved this from Todo to In Progress in ACK integration testing v2 Feb 16, 2023
@jljaco jljaco added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. labels Mar 14, 2023
@jljaco jljaco mentioned this issue Mar 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@a-hilaly a-hilaly

Labels
area/multi-version Issues or PRs related to multi version support kind/enhancement Categorizes issue or PR as related to existing feature enhancements. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
No open projects
ACK Core Team
Status: In Progress
Milestone
No milestone
Development

No branches or pull requests
7 participants
@jaypipes @RedbackThomson @muvaf @a-hilaly @vijtrip2 @ack-bot @jljaco