add v2 apis by erhancagirici · Pull Request #854 · crossplane/crossplane-runtime

erhancagirici · 2025-07-08T18:20:23Z

Description of your changes

New V2 apis and removals to facilitate namespaced MRs.

v2 ManagedResourceSpec API:

writeConnectionSecretToRef drops namespace field and accepts local secret references.
providerConfigRef becomes typed and requires kind. apiGroup is intentionally left out, it is expected to be inferred by the provider code. Namespaced
deletionPolicy is removed. New resources should rely on managementPolicies to control external resource deletion.

External Secret Store support is removed
publishConnectionDetailsTo is removed from both cluster-scoped (legacy) and namespace-scoped (modern) MR specs.
ConnectionDetailsPublisherTo interface is removed.

Cross-namespace cross-resource references:
Introduced NamespacedResolutionRequest, NamespacedResolutionResponse, NamespacedMultiResolutionRequest, NamespacedMultiResolutionResponse types along with APINamespacedResolver . In the new types, references and selectors in the resolution requests can include optional namespace parameter. Otherwise, they will default to MR namespaces.

I have:

Read and followed Crossplane's contribution process.
Run earthly +reviewable to ensure this PR is ready for review.
Added or updated unit tests.
Linked a PR or a docs tracking issue to document this change.
Added backport release-x.y labels to auto-backport this PR.

Need help with this checklist? See the cheat sheet.

negz

This looks great overall - I like the direction. Thanks @erhancagirici!

negz · 2025-07-08T18:36:52Z

apis/common/v2/resource.go

+
+package v2
+
+import v1 "github.com/crossplane/crossplane-runtime/apis/common/v1"


Is there a way to avoid having the v2 package depend on v1 types?

Technically yes, but I see two ways, both having its own drawback:
most of apis/common/v1 actually has many shared constructs not particularly "v1", like ManagementPolicies, Conditions, Selector, Reference types etc.

If we duplicate them, we end up with same but different structs, which needs superfluous handling in various places.

One option can be to refactor common stuff mentioned above to its own package, but that also kind of breaks existing imports in the ecosystem.
Open to ideas here

Maybe we could:

Move the shared stuff to apis/common/common.go

Have both common/v1 and common/v2 import common

Add type aliases in the v1 package to avoid breaking folks

I think I prefer that approach. I agree that v1 is a bit of a misnomer in the first place.

The other options I considered are:

Duplicate the types in both places. I like that this decouples the two APIs, but it forces us to have two implementations of any function that uses these types, which I think is a non-starter.

Duplicate the types in both places, also add internal versions, update our functions to work on the internal versions, and generate code to translate between v1/v2 and internal. This seems like the best approach in theory, but it's a ton of work for these little utility types.

We just had an offline sync with @erhancagirici on this and the above proposal sounds good in general. We discussed some details like linter complaining about common as a package name and whether we should put a deprecation notice on v1 aliased types/consts/funcs.

At the end, we feel good about suppressing linter on package name and going with common as suggested. apis/common is there since the begining and we're fine for it as a package name. Regarding the deprecation notice, we believe it would be good not to force consumers to immediately switch to the new place to minimize the required changes for now. So, if linter complains and forces consumers to migrate, then we should better not do it now (and track with an issue to do it later). This is something @erhancagirici will check and see.

I have pushed the changes with the type aliasing approach.
One side note: I had to bump the controller-tools to 0.18.0 to consume following fixes about type aliases.

kubernetes-sigs/controller-tools#1061
kubernetes-sigs/controller-tools#1122
This bump transitively upgraded google.golang.org/protobuf to v1.36.5 which had some minor changes in the generated protobuf files (backward-compatible).

I haven't added the depreciation notices to aliased types in apis/common/v1 yet, as they cause large amount of linter warnings. I'll create a follow-up issue for this.

negz · 2025-07-08T18:40:00Z

pkg/reconciler/managed/api.go

+// UnpublishConnection is no-op since PublishConnection only creates resources
+// that will be garbage collected by Kubernetes when the managed resource is
+// deleted.
+func (a *APILocalSecretPublisher) UnpublishConnection(_ context.Context, _ resource.LocalConnectionSecretOwner, _ ConnectionDetails) error {
+	return nil
+}


iirc this'll always be a no-op now that external secret stores are gone.

No strong feelings about removing it now though - we could follow up later.

let's have this as a follow-up

negz · 2025-07-08T18:44:51Z

pkg/reconciler/managed/publisher.go

-		return false, errors.New(errSecretStoreDisabled)
+// PublishConnection calls each ConnectionPublisher.PublishConnection serially. It returns the first error it
+// encounters, if any.
+func (pc LocalPublisherChain) PublishConnection(ctx context.Context, o resource.LocalConnectionSecretOwner, c ConnectionDetails) (bool, error) {


Do we still need this with external secret stores gone? I think we'll always publish to one secret.

(Again not urgent to remove it though.)

Agreed, this was needed for the WithLocalConnectionPublishers(p ...ConnectionPublisher) reconciler option, which is already irrelevant.

A question is, the original(legacy) one of becomes irrelevant too. Should we remove this one to make our intention clear? or keep it for the sake of not causing a code changes in the (native) providers for existing legacy resources?

crossplane-runtime/pkg/reconciler/managed/reconciler.go

Lines 702 to 706 in f5f608c

func WithConnectionPublishers(p ...ConnectionPublisher) ReconcilerOption {

return func(r *Reconciler) {

r.managed.ConnectionPublisher = PublisherChain(p)

}

}

Maybe make it no-op with deprecated comment?

I'd lean toward just removing it. Either way it's going to be work for upstream provider authors. If it's gone, it's more obvious they need to do that work.

I don't feel strongly though.

I'll remove this and PublisherChain and keep an unexported version for unit testing purposes for cases we need to mock.

pkg/reconciler/managed/reconciler.go

pkg/resource/interfaces.go

negz

Great work @erhancagirici! This is a smaller diff overall than I expected, which is good. None of my comments are blocking.

apis/common/v1/resource.go

negz · 2025-07-19T02:56:03Z

pkg/reconciler/managed/policies.go

+// is enabled.
+// Deprecated: this is intended for LegacyManaged resources that had deletionPolicy
+// ModernManaged resources should use NewManagementPoliciesResolver.
+func NewLegacyManagementPoliciesResolver(managementPolicyEnabled bool, managementPolicy xpv1.ManagementPolicies, deletionPolicy xpv1.DeletionPolicy, o ...ManagementPoliciesResolverOption) ManagementPoliciesChecker {


Nit: Some of these long var names are a bit redundant. Not blocking though, and I can see it matches the existing implementation.

negz · 2025-07-19T03:01:25Z

pkg/reconciler/managed/publisher.go

-		return false, errors.New(errSecretStoreDisabled)
+// PublishConnection calls each ConnectionPublisher.PublishConnection serially. It returns the first error it
+// encounters, if any.
+func (pc LocalPublisherChain) PublishConnection(ctx context.Context, o resource.LocalConnectionSecretOwner, c ConnectionDetails) (bool, error) {


I'd lean toward just removing it. Either way it's going to be work for upstream provider authors. If it's gone, it's more obvious they need to do that work.

I don't feel strongly though.

pkg/reference/namespaced_reference.go

pkg/resource/resource.go

pkg/reference/namespaced_reference.go

negz · 2025-07-19T03:29:14Z

pkg/reconciler/managed/reconciler_modern_test.go

+
+var _ reconcile.Reconciler = &Reconciler{}
+
+func TestModernReconciler(t *testing.T) {


I do wonder whether there's diminishing returns in completely duplicating the test file. I imagine a lot of these subtest scenarios use the same code regardless of whether it's a legacy or a modern MR, right?

It seems like having two test files could be a bit of a pain - we'll need to add and update them both whenever we update the tests. On the other hand it does seem like the most defensive/conservative approach to make sure both legacy and modern MRs work with all parts of the reconciler.

As you mentioned, this was mostly for being defensive. I tried to pinpoint the "common" tests ( relying only on the "base" managed functional), though ended up duplicating most already to cover some unvisited codepaths.
We might consider following up later with a refactored test setup, that runs the tests as a matrix for legacy vs modern maybe?

negz · 2025-07-19T03:31:48Z

Officially deferring my reviewer powers to @turkenh if there's any other questions or concerns while I'm out on vacation. 😉

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

fixes deepcopy method generation for structs that has aliased types Signed-off-by: Erhan Cagirici <erhan@upbound.io>

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

turkenh · 2025-07-22T12:19:02Z

.golangci.yml

+      - linters:
+          - revive
+        text: "var-naming: avoid meaningless package names"
+        path: apis/common


Could we add a //nolint directive instead? https://github.com/crossplane/crossplane/tree/main/contributing#explain-nolint-directives

opted for this, because had to repeat this for all the new files, and nolint directives were appearing at the very top and potentially in godoc. not strongly opinionated though

turkenh

Looks good to me, thanks @erhancagirici 🙌

turkenh · 2025-07-22T12:27:24Z

pkg/reference/namespaced_reference.go

+		ns = r.from.GetNamespace()
+	}
+
+	if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels), client.InNamespace(req.Namespace)); err != nil {


Should this use ns instead of req.Namespace ?

good catch, fixed

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

negz reviewed Jul 8, 2025

View reviewed changes

erhancagirici force-pushed the ec-xpv2 branch 4 times, most recently from eb6e96e to 812d411 Compare July 15, 2025 18:50

erhancagirici marked this pull request as ready for review July 15, 2025 18:54

erhancagirici requested a review from a team as a code owner July 15, 2025 18:54

erhancagirici requested review from negz and phisco July 15, 2025 18:54

erhancagirici force-pushed the ec-xpv2 branch from 812d411 to f613617 Compare July 18, 2025 07:55

negz approved these changes Jul 19, 2025

View reviewed changes

negz assigned turkenh Jul 19, 2025

erhancagirici force-pushed the ec-xpv2 branch 3 times, most recently from 8e150cd to b069fec Compare July 21, 2025 15:20

add v2 apis

f09b0a3

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

erhancagirici force-pushed the ec-xpv2 branch from b069fec to 35f461f Compare July 21, 2025 15:25

add unit tests for v2

54f050c

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

erhancagirici force-pushed the ec-xpv2 branch from 35f461f to 54f050c Compare July 21, 2025 15:26

jbw976 added this to Crossplane Roadmap Jul 21, 2025

jbw976 moved this to In Review in Crossplane Roadmap Jul 21, 2025

erhancagirici added 4 commits July 22, 2025 13:12

refactor shared api types to common package

c056ae9

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

bump controller-tools to v1.18.0

5c98d32

fixes deepcopy method generation for structs that has aliased types Signed-off-by: Erhan Cagirici <erhan@upbound.io>

make generate: refactored common api deepcopy methods

8ba120d

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

make generate: protobuf updates after protoc-gen-go-grpc bump

1471876

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

erhancagirici force-pushed the ec-xpv2 branch from fd36255 to 1471876 Compare July 22, 2025 10:33

turkenh reviewed Jul 22, 2025

View reviewed changes

turkenh approved these changes Jul 22, 2025

View reviewed changes

use correct namespace from MR in MultiNamespacedResolution

cf256f8

Signed-off-by: Erhan Cagirici <erhan@upbound.io>

turkenh merged commit 1e3dba9 into crossplane:main Jul 22, 2025
10 checks passed

github-project-automation bot moved this from In Review to Done in Crossplane Roadmap Jul 22, 2025

ulucinar mentioned this pull request Sep 9, 2025

[v2] Remove external secret store support #842

Closed


		package v2

		import v1 "github.com/crossplane/crossplane-runtime/apis/common/v1"

	func WithConnectionPublishers(p ...ConnectionPublisher) ReconcilerOption {
	return func(r *Reconciler) {
	r.managed.ConnectionPublisher = PublisherChain(p)
	}
	}


		var _ reconcile.Reconciler = &Reconciler{}

		func TestModernReconciler(t *testing.T) {

Conversation

erhancagirici commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

Uh oh!

negz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erhancagirici Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

negz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

negz commented Jul 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erhancagirici Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turkenh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

erhancagirici commented Jul 8, 2025 •

edited

Loading

erhancagirici Jul 15, 2025 •

edited

Loading

erhancagirici Jul 22, 2025 •

edited

Loading