Skip to content

Generate namespaced managed resources#1753

Merged
negz merged 40 commits intocrossplane-contrib:crossplane-v2from
jbw976:negz-spatial
Mar 25, 2025
Merged

Generate namespaced managed resources#1753
negz merged 40 commits intocrossplane-contrib:crossplane-v2from
jbw976:negz-spatial

Conversation

@jbw976
Copy link
Copy Markdown
Member

@jbw976 jbw976 commented Mar 23, 2025

Description of your changes

This PR continues the work from draft PR #1689 that @negz started. It essentially updates this provider to start generating namespaced managed resources in addition to cluster scoped ones, which will be featured heavily in the current proposal of Crossplane v2.

This should not be taken as a final product. This is a practical update to this provider as a proof of concept for what we think a provider could look like in Crossplane v2. There are many short cuts taken, it is not production ready, and we are very open to feedback to change directions or improve the architecture.

The work in this PR should be merged into the new crossplane-v2 branch, which is based on commit 1ecd9cb, the latest commit when we started this work. We will need to rebase this effort in the future to include latest changes from main, but that should not block this PR.

I have:

  • Read and followed Crossplane's contribution process.
  • Run make reviewable to ensure this PR is ready for review.

General steps this PR has taken

The following list describes the general process that was taken to update this provider to support namespaced MRs:

  • update to latest upjet version that supports namespaced resources
  • duplicate content into both cluster and namespaced copies and make some minor updates for api groups and import paths
    • config, apis, controllers
  • remove all legacy conversion/migration logic, because only the latest version needs to be supported
  • update the provider main.go template to setup both cluster and namespaced apis and controllers
  • update code generation comment markers to run on both cluster and namespaced types
  • update the generator cmd to init both cluster and namespaced config to pass to code gen pipeline
  • manually copy and update the handful of manual api and controller files to namespaced dirs

How has this code been tested

Besides generating, building and publishing successfully, this code has also been tested manually by installing the config, s3, rds, and ec2 packages into a control plane, then creating both cluster scoped and namespaced S3 buckets. Ex.:

❯ { head -n 1; grep '^buckets '; } < <(kubectl api-resources)
NAME                                        SHORTNAMES   APIVERSION                            NAMESPACED   KIND
buckets                                                  s3.aws.upbound.io/v1beta2             false        Bucket
buckets                                                  s3.m.aws.upbound.io/v1beta1           true         Bucket

❯ k get managed -A
NAMESPACE   NAME                                               SYNCED   READY   EXTERNAL-NAME             AGE
            bucket.s3.aws.upbound.io/crossplane-bucket-tmm54   True     True    crossplane-bucket-tmm54   21s

NAMESPACE   NAME                                                 SYNCED   READY   EXTERNAL-NAME             AGE
xp          bucket.s3.m.aws.upbound.io/crossplane-bucket-mhv6k   True     True    crossplane-bucket-mhv6k   18s

negz and others added 30 commits February 26, 2025 19:45
Signed-off-by: Nic Cope <nicc@rk0n.org>
It doesn't fit well in apis given it also generates controllers,
commands, etc.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Ideally the code generation configuration wouldn't depend on generated
code. That should be fixed in a future commit.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Using:

sed -i -e 's/scope=Cluster/scope=Namespaced/' $(find . -name '*.go'|xargs)

Signed-off-by: Nic Cope <nicc@rk0n.org>
Unchanged, for now.

Signed-off-by: Nic Cope <nicc@rk0n.org>
This should make the namespaced APIs and controllers actually namespaced

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
The controller import package is now configurable.

Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Nic Cope <nicc@rk0n.org>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
…o pass to upjet pipeline

Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
…lly cased in singleton list handling

These specific controllers were handled specially in the singleton list conversion code,
since that has now been removed, their versions are bumped during code gen. Not entirely
sure this is a good thing just yet, but we'll see.

Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
* refactor GetProvider so that xpprovider.GetProvider() is only
  called once at runtime and then reused for both cluster and
  namespaced
* update provider main.go template to use both cluster and namespaced
  config during start up. This is important as both of their setup,
  conversions, etc. need to be honored

Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
The CRD file names don't conflict with each other because they have
their full group names embedded in the file name. When the kustomize
tooling runs over the CRDs, it's easier to process them all in one single
directory, so we don't end up with wierd duplicates.

Signed-off-by: Jared Watts <jbw976@gmail.com>
This isn't an ideal name because it doesn't exactly match the same pattern
of other groups, e.g. s3.m.aws.upbound.io, but it is a quick way to make
publishing work for the family/config provider. That provider is special
cased to take all aws.* CRDs. Updating the group for namespaced core/config
CRDs to aws.m.upbound.io means they are also included in the family/config
provider along with the cluster scoped aws.upbound.io CRDs. We should likely
revisit this with more time to maybe update the up xpkg batch tooling.

Signed-off-by: Jared Watts <jbw976@gmail.com>
…tory

Signed-off-by: Jared Watts <jbw976@gmail.com>
jbw976 added 2 commits March 21, 2025 12:11
…atial

Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
jbw976 added 2 commits March 24, 2025 11:34
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 24, 2025

Trying to fix the linter errors now, but that has been a bit tough due to the long cycles it takes to run, find issues, repeat :)

I don't have a solution for the failed check-examples test yet, looks like the examples dir is populated somewhat manually while testing a new resource, starting with the manifests from the examples-generated dir. I wonder if we can do something simple here...

jbw976 added 2 commits March 24, 2025 12:39
… vars

Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 24, 2025

Just pushed a few commits that give me a clean make lint locally, c'mon CI! 🙏

// Package v1alpha1 contains the core resources of the aws provider.
// +kubebuilder:object:generate=true
// +groupName=m.aws.upbound.io
// +groupName=aws.m.upbound.io
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we switch everything to use aws.m.upbound.io?

I think that actually makes the most sense - since `m.upbound.io is the least specific part of the domain.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, we can do that! that actually makes more sense for the root "config" group, as the xpkg batch assumes all of the CRDs for that group start with aws.*. Let's see how code gen like this update... 🙏

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now complete @negz, all the namespaced CRDs consistently use *.aws.m.upbound.io and I manually built/tested with the s3 provider 👍

go 1.23.6

replace github.com/crossplane/upjet => github.com/negz/upjet v0.0.0-20250227033622-bf0ba4f0de9d
replace github.com/crossplane/upjet => github.com/negz/upjet v0.0.0-20250320063135-520f63c242c6
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding a note that we shouldn't merge this until we switch this to the crossplane/upjet crossplane-v2 branch after crossplane/upjet#480 is merged.

@negz
Copy link
Copy Markdown
Member

negz commented Mar 24, 2025

@jbw976 why did you end up with config/namespaced/common/ and config/cluster/common`?

When we chatted with @ulucinar and @sergenyalcin I think we said we'd:

  • Put most config only in common
  • (Only) extend common with cluster-specific overrides in cluster
  • (Only) extend common with namespace-specific overrides in namespaced

Is that still the plan long term?

Copy link
Copy Markdown
Member

@negz negz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I focused on the hand-written commits in my review, and this looks like a good starting point. I'd like to get it merged so we can build an artifact and iterate.

@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 25, 2025

Is that still the plan long term?

Yes I think that is still the long term plan! I haven't tried any config consolidation into a common config yet in the interest of time.

why did you end up with config/namespaced/common/ and config/cluster/common`?

This is an artifact of there previously being a config/common that got moved down into config/cluster/common along with all the other previously existing config getting moved into a cluster subdir, and then copied over to config/namespaced/common as well. The code in that dir imports some types from apis, so as of now I still have the separate cluster and namespaced copies that import their corresponding api types.

There's not a ton of code in those common config dirs, so I reckon when we try to do a REAL common config dir it should be easy enough to make it work. Just haven't done any of that yet 😁

jbw976 added 2 commits March 24, 2025 17:46
Signed-off-by: Jared Watts <jbw976@gmail.com>
Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 25, 2025

I'm not sure what's going on with the linter check on this PR, make lint is passing consistently for me in ~14 mins, but it's timing out here after 2 hours. I suppose there is some local caching that I'm benefiting from on my laptop, as well as some extra CPU cores.

From my laptop:

❯ make lint
19:24:44 [ .. ] Running golangci-lint with the analysis cache building phase.
...
19:38:16 [ OK ] golangci-lint
19:38:16 [ .. ] Untagging source files.
19:38:18 [ OK ] Untagging source files.

I wonder if we need to continue investing in the strategy already in place from #1194, beyond what I've already done in this PR, to maybe filter out via build tags all the namespaced generated code, so we're not doing ~2x the linting? Or give the lint job a larger runner?

On main, it looks like lint already takes 1.5 hours, e.g. this recent run. Maybe a larger runner isn't a bad idea in general 😂

Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 25, 2025

yay lint passed in 1h40m with a larger runner, so it's not fundamentally broken, just resource constrained. Not an ideal long term solution, but perhaps enough to merge this and iterate 😂

Only unsolved item for the PR feedback and status checks is now check-examples.

We don't currently have manually curated examples for namespaced
resources, so for now we will exclude them for this examples check.

Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976
Copy link
Copy Markdown
Member Author

jbw976 commented Mar 25, 2025

@negz, we got a green run here!! take a quick look again if you'd like at the latest manual commits to integrate all your feedback, to get linting passing by using a larger runner, and to exclude namespaced examples for now.

I am tracking all these shortcuts and will open issues later, don't worry about that! 😇

@negz negz merged commit 9d6587d into crossplane-contrib:crossplane-v2 Mar 25, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants