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.

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

webhook: validate owned-by annotation on create, update, delete #5031

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

connorkuehl
Copy link

This annotation is already validated on the PVC current object in the Delete handler, but to be more consistent, also check the new object in the Update and Create handlers.

This annotation is already validated on the PVC current object in the
Delete handler, but to be more consistent, also check the new object
in the Update and Create handlers.

Signed-off-by: Connor Kuehl <connor.kuehl@suse.com>
Copy link
Member

@FrankYang0529 FrankYang0529 left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

Copy link
Contributor

@ibrokethecloud ibrokethecloud left a comment

Choose a reason for hiding this comment

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

lgtm. thanks.

@w13915984028
Copy link
Member

w13915984028 commented Jan 31, 2024

Wait a minute:

For VM related PVCs, the owned-by is trully set; but there are some PVCs used by applications like rancher-monitoring, vcluster, the owned-by is not set.

In v1.2.1 and later, rancher-monitoring is an addon, if we enable it on the fly, it will create the PVCs and they are checked by the Harvester webhook.

harvester112:~ # kubectl get pvc vm66-disk-0-3pmjd -oyaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    harvesterhci.io/imageId: default/image-wzkml
    harvesterhci.io/owned-by: '[{"schema":"kubevirt.io.virtualmachine","refs":["default/vm66"]}]'
    pv.kubernetes.io/bind-completed: "yes"
    pv.kubernetes.io/bound-by-controller: "yes"
    volume.beta.kubernetes.io/storage-provisioner: driver.longhorn.io
    volume.kubernetes.io/storage-provisioner: driver.longhorn.io
  creationTimestamp: "2023-05-16T10:02:41Z"
harvester112:~ # kubectl get pvc prometheus-rancher-monitoring-prometheus-db-prometheus-rancher-monitoring-prometheus-0 -n cattle-monitoring-system -oyaml
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  annotations:
    pv.kubernetes.io/bind-completed: "yes"
    pv.kubernetes.io/bound-by-controller: "yes"
    volume.beta.kubernetes.io/storage-provisioner: driver.longhorn.io
    volume.kubernetes.io/storage-provisioner: driver.longhorn.io
  creationTimestamp: "2023-03-07T13:29:05Z"

@w13915984028
Copy link
Member

Should we add another annotation like managed-by harvesterhci.io, to combine with owned-by?

In Harvester, user may also use helm to deploy other charts (we know some customers are doing so), those related PVCs will have no such annotations.

@connorkuehl
Copy link
Author

In v1.2.1 and later, rancher-monitoring is an addon, if we enable it on the fly, it will create the PVCs and they are checked by the Harvester webhook.

The webhook doesn't require that harvesterhci.io/owned-by is present, it only requires that if the annotation is there that it has the correct structure. It won't reject Rancher's PVCs.

@w13915984028
Copy link
Member

Got it, great, thanks.

@bk201 bk201 merged commit 3972ddd into harvester:master Feb 1, 2024
5 checks passed
@bk201
Copy link
Member

bk201 commented Feb 1, 2024

@Mergifyio backport v1.2

Copy link

mergify bot commented Feb 1, 2024

backport v1.2

✅ Backports have been created

@bk201
Copy link
Member

bk201 commented Feb 1, 2024

@Mergifyio backport v1.1

Copy link

mergify bot commented Feb 1, 2024

backport v1.1

✅ Backports have been created

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.

None yet

5 participants