Skip to content

Conversation

@ellistarn
Copy link
Contributor

@ellistarn ellistarn commented Oct 21, 2025

KREP-003 introduces Decorators (a.k.a Collection Watching), an extension of Collections and ExternalRefs. We extend externalRef to support watching a collection of objects, rather than just a single object.

See the full proposal here: https://github.com/ellistarn/kro/blob/krep/docs/design/proposals/decorators.md

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 21, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ellistarn
Once this PR has been reviewed and has the lgtm label, please assign nicslatts for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 21, 2025
@ellistarn ellistarn force-pushed the krep branch 4 times, most recently from b52060c to 012afa1 Compare October 21, 2025 00:48
Copy link
Contributor

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

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

Lgtm, I think you can flatten the API a little.

// +kubebuilder:validation:Required
Kind string `json:"kind"`
// +kubebuilder:validation:Optional # <---- Mutually exclusive with Selector, NamespaceSelector
Metadata ExternalRefMetadata `json:"metadata"`
Copy link
Contributor

Choose a reason for hiding this comment

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

What if selector had an optional "Namespace" field to continue filtering labeled resources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted the full semantics to do things like opt namespaces in by label. Can you be a hair more concrete about what you're proposing? Structs?


## Proposed API and Behavior Changes

1. Introduce `Selector` and `NamespaceSelector` to `ExternalRef`
Copy link
Member

Choose a reason for hiding this comment

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

when using Selector and NamespaceSelector, there is even more reason to keep it as Reference because even Kubernetes calls it reference

Comment on lines +33 to +37
2. Make `Metadata` optional and mutually exclusive with `Selector` and `NamespaceSelector`
3. `Kind` must refer to a `ListKind` when `Metadata` is not defined (e.g. `DeploymentList`)
4. If `Metadata` is defined, `ExternalRef` refers to a single resource, otherwise it refers to a
collection of resources
5. Both `Selector` and `NamespaceSelector` are optional, and if omitted, all resources are included
Copy link
Contributor

@tjamet tjamet Oct 22, 2025

Choose a reason for hiding this comment

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

I'm unsure I get the difference between Selector and NamespaceSelector

What would be in Metadata that makes it impossible to craft a good Selector?

I smell that we would gain simplicity by having a single Selector to filter object of Kind whether it's a namespace or any other object.

There is another question, whether it is allowed, to watch resources from other namespaces (or cluster-wide for namespaced RGDs).

Maybe we can inspire from NetworkPolicies and define:

NamespaceSelector a selector to match namespace labels in which the resource should be
Selector a selector to match the resource labels

Copy link
Contributor Author

@ellistarn ellistarn Oct 22, 2025

Choose a reason for hiding this comment

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

The example from NetworkPolicies is exactly what I was thinking. The NamespaceSelector constrains what Namespaces we look at and the regular Selector constrains the individual objects by label.

namespaceSelector: # Only include namespaces with the opt-in label
  matchLabels:
    "opt-in-namespace": true
selector:
  matchLabels
    "opt-in-object": true

In this example, you would only include objects that are opted in in both their own metadata.labels, as well as the metadata.labels of the namespace. e.g.

kind: Namespace
metadata:
  labels:
    "opt-in-namespace": true
  ...
---
kind: Object
metadata:
  labels: 
    "opt-in-object": true

Copy link
Contributor

Choose a reason for hiding this comment

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

Why having metadata? for backward compatibility?

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2025
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 7, 2025
@k8s-ci-robot
Copy link
Contributor

@ellistarn: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
presubmits-build-image 789e818 link true /test presubmits-build-image
presubmits-e2e-tests 789e818 link true /test presubmits-e2e-tests
presubmits-integration-tests 789e818 link true /test presubmits-integration-tests
presubmits-verify-lint 789e818 link true /test presubmits-verify-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants