Skip to content

bug: verify existing ownership of CRD managed by ResourceGraphDefinition to prevent conflict #562

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
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

bug: verify existing ownership of CRD managed by ResourceGraphDefinition to prevent conflict #562

wants to merge 3 commits into from

Conversation

a-buck
Copy link

@a-buck a-buck commented May 19, 2025

Hi 👋 This PR intends to fix #554. This is my first first PR to the project - please don't hesitate to give me feedback.

This PR adds a check to verify ownership of the CRD created by a ResourceGraphDefinition, and returns an error if the CRD already exists (not managed by KRO), or exists and is managed by a different ResourceGraphDefinition.

Example: Given a CustomResourceDefinition that pre-exists and is managed by another ResourceGraphDefinition

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
  labels:
    kro.run/kro-version: devel
    kro.run/owned: "true"
    kro.run/resource-graph-definition-id: e44f64c5-6f4c-4442-9824-73cf3ba5e162
    kro.run/resource-graph-definition-name: noop2
  name: noops.kro.run
# rest of resource trimmed for brevity

If another ResourceGraphDefinition is created that also attempts to manage this CRD, an error will be surfaced in the status. I trimmed some fields for brevity

apiVersion: kro.run/v1alpha1
kind: ResourceGraphDefinition
metadata:
  annotations:
  finalizers:
  - kro.run/finalizer
  generation: 1
  name: noop
spec:
  resources: []
  schema:
    apiVersion: v1alpha1
    group: kro.run
    kind: NoOp
    spec: {}
    status: {}
status:
  conditions:
  - lastTransitionTime: "2025-05-19T17:01:17Z"
    message: Directed Acyclic Graph is synced
    reason: ""
    status: "True"
    type: GraphVerified
  - lastTransitionTime: "2025-05-19T17:01:17Z"
    message: Custom Resource Definition is synced
    reason: 'conflict detected: CRD noops.kro.run has ownership by another ResourceGraphDefinition'
    status: "False"
    type: CustomResourceDefinitionSynced
  - lastTransitionTime: "2025-05-19T17:01:17Z"
    message: micro controller is ready
    reason: CRD not-synced
    status: Unknown
    type: ReconcilerReady
  state: Inactive

@a-buck a-buck marked this pull request as ready for review May 19, 2025 17:23
@a-buck a-buck force-pushed the verify-no-conflict branch from d92d623 to 87caeae Compare May 23, 2025 10:11
@barney-s barney-s self-assigned this May 23, 2025
@barney-s barney-s requested a review from a-hilaly May 23, 2025 17:13
@barney-s
Copy link
Contributor

Thankyou for the PR. Would you please rebase to latest main. ?

a-hilaly
a-hilaly reviewed Aug 18, 2025
View reviewed changes
pkg/metadata/labels.go
Comment on lines +58 to +62
aOwnerName := a.Labels[ResourceGraphDefinitionNameLabel]
aOwnerID := a.Labels[ResourceGraphDefinitionIDLabel]

bOwnerName := b.Labels[ResourceGraphDefinitionNameLabel]
bOwnerID := b.Labels[ResourceGraphDefinitionIDLabel]
Copy link
Member

Choose a reason for hiding this comment

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

Can we also check for the existence of these labels, for example aOwnerName, ok := .... I believe If those labels don't exist we should return false or an error saying labels are missing. Also can we have some unit tests for it?

Copy link
Author

Choose a reason for hiding this comment

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

Apologies for the delay in responding. I added a test cases including when there are no ownership labels. Is that sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

Yep that's great, tahnks!

@k8s-triage-robot
Copy link

Unknown CLA label state. Rechecking for CLA labels.

Send feedback to sig-contributor-experience at kubernetes/community.

/check-cla
/easycla

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 12, 2025
@a-hilaly
Copy link
Member

ping @a-buck any chance you can take a look at this?

@a-buck
bug: verify existing ownership of CRD managed by ResourceGraphDefinit…
36c2cf1 
…ion to prevent conflict

this prevents 2 ResourceGraphDefinitions fighting to manage the same CRD

Signed-off-by: abuck <abuck@spotify.com>
@a-buck
fix: tear down CRDs inbetween tests to avoid unintended conflicts
5bb081b 
Signed-off-by: abuck <abuck@spotify.com>
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Sep 15, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: a-buck
Once this PR has been reviewed and has the lgtm label, please ask for approval from barney-s. 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 cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 15, 2025
@a-buck
labels test case
13bc019 
Signed-off-by: abuck <abuck@spotify.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 15, 2025
a-hilaly
a-hilaly reviewed Sep 16, 2025
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Neat - thank you @a-buck! I left a few comments below

pkg/metadata/labels.go
return ok && booleanFromString(v)
}

// HasMatchingKROOwner returns true if resources have the same RGD owner
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you document that the caller is reponsible of making sure that kro labels exist? otherwise we can just delegate this existance check to this function.

pkg/client/crd.go
})
}

func (w *CRDWrapper) verifyNoConflict(existingCRD *v1.CustomResourceDefinition, newCRD v1.CustomResourceDefinition) error {
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/verifyNoConflict/verifyOwnership/

Copy link
Member

Choose a reason for hiding this comment

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

can you please add a comment/godoc explaining why we need to check this

@a-hilaly
Copy link
Member

cc @a-buck are you still working on this?

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

@a-hilaly a-hilaly a-hilaly left review comments

Assignees

@barney-s barney-s

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Conflict when 2 RGDs define the same Schema Kind
5 participants
@a-buck @barney-s @k8s-triage-robot @a-hilaly @k8s-ci-robot