set webhooks ownerReferences to namespace #2098

novahe · 2021-04-20T16:54:30Z

Changes

set webhooks ownerReferences to namespace

/kind cleanup

Fixes #2044

Release Note

NONE

Docs

knative-prow-robot · 2021-04-20T16:54:41Z

Welcome @novahe! It looks like this is your first PR to knative/pkg 🎉

knative-prow-robot · 2021-04-20T16:54:41Z

Hi @novahe. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

codecov · 2021-04-20T16:59:23Z

Codecov Report

Merging #2098 (603d3f7) into main (b51994e) will increase coverage by 0.22%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##             main    #2098      +/-   ##
==========================================
+ Coverage   67.31%   67.54%   +0.22%     
==========================================
  Files         215      215              
  Lines        9092     9110      +18     
==========================================
+ Hits         6120     6153      +33     
+ Misses       2697     2679      -18     
- Partials      275      278       +3

Impacted Files	Coverage Δ
webhook/configmaps/configmaps.go	`86.45% <60.00%> (-1.59%)`	⬇️
webhook/resourcesemantics/defaulting/defaulting.go	`80.12% <60.00%> (-0.75%)`	⬇️
...k/resourcesemantics/validation/reconcile_config.go	`86.95% <60.00%> (-2.28%)`	⬇️
apis/duck/v1beta1/destination.go	`96.49% <0.00%> (-0.12%)`	⬇️
test/ha/ha.go	`17.39% <0.00%> (ø)`
tracing/zipkin.go	`68.42% <0.00%> (ø)`
injection/config.go	`0.00% <0.00%> (ø)`
reconciler/events.go	`100.00% <0.00%> (ø)`
reconciler/filter.go	`100.00% <0.00%> (ø)`
webhook/admission.go	`82.22% <0.00%> (ø)`
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b80a192...603d3f7. Read the comment docs.

markusthoemmes · 2021-04-21T06:47:51Z

/ok-to-test

The code looks spot on, thanks! Have we tested this is actually fixing the referenced bug? Let's also make sure the OwnerReference doesn't cause the webhook config to be garbage collected. I guess manual tests for both of those are fine.

novahe · 2021-04-21T16:33:49Z

Thank you for reviewing @markusthoemmes

In my test environment
I tested to delete the ns, webhook config can be deleted:

I deleted webhook pod twice, The webhook config has not changed

only updated once

Am I missing something? What else do I need to do?

markusthoemmes

/assign @dprotaso

Sorry for having this dangling for a while. Adding Dave to assert if this isn't breaking assumptions in K8s API.

webhook/configmaps/configmaps.go

webhook/resourcesemantics/validation/reconcile_config.go

webhook/resourcesemantics/defaulting/defaulting.go

novahe · 2021-06-07T15:32:26Z

/test pull-knative-pkg-unit-tests

novahe · 2021-06-07T15:41:51Z

I fixed it, PTAL thanks

dprotaso · 2021-06-11T01:48:10Z

🎉 tested it out and it works

/lgtm
/approve

knative-prow-robot · 2021-06-11T01:48:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, novahe

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~webhook/OWNERS~~ [dprotaso]

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

novahe · 2021-06-11T02:21:14Z

/test pull-knative-pkg-unit-tests

julz · 2021-06-11T09:18:29Z

This is going to avoid a lot of user confusion, thanks @novahe 🎉🎉

cc @omerbensaadon @csantanapr I think you will like!

This patch adds the permission to update `namespaces/finalizers`. Since knative/pkg#2098 added ownerRef refers to namespace for webhook, we need the permission. Without it, cluster which has a stricter RBAC rules gets the following error: ``` cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: ```

* Add permission to update namespace finalizer This patch adds the permission to update `namespaces/finalizers`. Since knative/pkg#2098 added ownerRef refers to namespace for webhook, we need the permission. Without it, cluster which has a stricter RBAC rules gets the following error: ``` cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on: ``` * Add kapp workaround * Add comment * Remove white space * Move kapp config into test/config/ytt/core * remove unnecessary change

(similar to knative/eventing#5501) This patch adds the permission to update `namespaces/finalizers`. Since knative/pkg#2098 added ownerRef refers to namespace for webhook, we need this permission. Without it, cluster which has a stricter RBAC rules gets the following error: ``` cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on ... ``` Signed-off-by: Pierangelo Di Pilato <[email protected]>

…#1000) (similar to knative/eventing#5501) This patch adds the permission to update `namespaces/finalizers`. Since knative/pkg#2098 added ownerRef refers to namespace for webhook, we need this permission. Without it, cluster which has a stricter RBAC rules gets the following error: ``` cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on ... ``` Signed-off-by: Pierangelo Di Pilato <[email protected]>

…knative-extensions#1000) (similar to knative/eventing#5501) This patch adds the permission to update `namespaces/finalizers`. Since knative/pkg#2098 added ownerRef refers to namespace for webhook, we need this permission. Without it, cluster which has a stricter RBAC rules gets the following error: ``` cannot set blockOwnerDeletion if an ownerReference refers to a resource you can't set finalizers on ... ``` Signed-off-by: Pierangelo Di Pilato <[email protected]>

knative-prow-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Apr 20, 2021

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 20, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 20, 2021

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 20, 2021

novahe force-pushed the setWebhookOwnerReferences branch from 1b3b751 to 20349ea Compare April 20, 2021 17:07

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 21, 2021

novahe mentioned this pull request May 27, 2021

Add novahe to the members list knative/community#637

Merged

markusthoemmes reviewed Jun 7, 2021

View reviewed changes

webhook/configmaps/configmaps.go Outdated Show resolved Hide resolved

webhook/resourcesemantics/validation/reconcile_config.go Outdated Show resolved Hide resolved

webhook/resourcesemantics/defaulting/defaulting.go Outdated Show resolved Hide resolved

knative-prow-robot assigned dprotaso Jun 7, 2021

set webhooks OwnerReferences to namespace (knative#2044)

603d3f7

novahe force-pushed the setWebhookOwnerReferences branch from 20349ea to 603d3f7 Compare June 7, 2021 15:16

novahe requested a review from markusthoemmes June 7, 2021 15:39

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 11, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 11, 2021

dprotaso mentioned this pull request Jun 11, 2021

Webhooks should get OwnerRef to namespace #2044

Closed

knative-prow-robot merged commit 8d21ce6 into knative:main Jun 11, 2021

novahe deleted the setWebhookOwnerReferences branch June 11, 2021 02:27

nak3 mentioned this pull request Jun 14, 2021

🤖 Triggering CI on branch 'release-next' after synching to upstream/main openshift/knative-serving#801

Merged

nak3 mentioned this pull request Jun 14, 2021

Add permission to update namespaces/finalizers knative/serving#11517

Merged

matzew mentioned this pull request Jun 14, 2021

Add permission to update namespaces/finalizer knative/eventing#5501

Merged

5 tasks

pierDipi mentioned this pull request Jun 15, 2021

Add permission to update namespaces/finalizer to webhook cluster role knative-extensions/eventing-kafka-broker#1000

Merged

travis-minke-sap mentioned this pull request Jul 1, 2021

Missing webhook rbac knative-extensions/eventing-kafka#751

Merged

mattmoor mentioned this pull request Jul 30, 2021

Update to latest knative.dev/pkg tektoncd/pipeline#4129

Merged

5 tasks

thesuperzapper mentioned this pull request Aug 24, 2024

fix using KNative with ArgoCD (don't set ownerReferences for webhooks) knative/serving#15483

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set webhooks ownerReferences to namespace #2098

set webhooks ownerReferences to namespace #2098

novahe commented Apr 20, 2021 •

edited

Loading

knative-prow-robot commented Apr 20, 2021

knative-prow-robot commented Apr 20, 2021

codecov bot commented Apr 20, 2021 •

edited

Loading

markusthoemmes commented Apr 21, 2021

novahe commented Apr 21, 2021

markusthoemmes left a comment

novahe commented Jun 7, 2021

novahe commented Jun 7, 2021

dprotaso commented Jun 11, 2021

knative-prow-robot commented Jun 11, 2021

novahe commented Jun 11, 2021

julz commented Jun 11, 2021

set webhooks ownerReferences to namespace #2098

set webhooks ownerReferences to namespace #2098

Conversation

novahe commented Apr 20, 2021 • edited Loading

Changes

knative-prow-robot commented Apr 20, 2021

knative-prow-robot commented Apr 20, 2021

codecov bot commented Apr 20, 2021 • edited Loading

Codecov Report

markusthoemmes commented Apr 21, 2021

novahe commented Apr 21, 2021

markusthoemmes left a comment

Choose a reason for hiding this comment

novahe commented Jun 7, 2021

novahe commented Jun 7, 2021

dprotaso commented Jun 11, 2021

knative-prow-robot commented Jun 11, 2021

novahe commented Jun 11, 2021

julz commented Jun 11, 2021

novahe commented Apr 20, 2021 •

edited

Loading

codecov bot commented Apr 20, 2021 •

edited

Loading