Skip to content
This repository has been archived by the owner on Sep 2, 2024. It is now read-only.

Missing webhook rbac #751

Conversation

travis-minke-sap
Copy link
Contributor

@travis-minke-sap travis-minke-sap commented Jul 1, 2021

The KafkaChannel Webhook is currently broken and not performing validation/mutation/etc. The logs show the following errors...

{"level":"debug","ts":"2021-07-01T20:57:15.279Z","logger":"kafkachannel-webhook.ValidationWebhook","caller":"controller/controller.go:502","msg":"Processing from queue validation.webhook.kafka.messaging.knative.dev (depth: 0)"}
{"level":"error","ts":"2021-07-01T20:57:15.281Z","logger":"kafkachannel-webhook.ValidationWebhook","caller":"controller/controller.go:549","msg":"Reconcile error","duration":0.001437821,"error":"failed to fetch namespace: namespaces \"knative-eventing\" is forbidden: User \"system:serviceaccount:knative-eventing:kafka-webhook\" cannot get resource \"namespaces\" in API group \"\" in the namespace \"knative-eventing\"","stacktrace":"knative.dev/pkg/controller.(*Impl).handleErr\n\tknative.dev/pkg@v0.0.0-20210622173328-dd0db4b05c80/controller/controller.go:549\nknative.dev/pkg/controller.(*Impl).processNextWorkItem\n\tknative.dev/pkg@v0.0.0-20210622173328-dd0db4b05c80/controller/controller.go:532\nknative.dev/pkg/controller.(*Impl).RunContext.func3\n\tknative.dev/pkg@v0.0.0-20210622173328-dd0db4b05c80/controller/controller.go:468"}
{"level":"debug","ts":"2021-07-01T20:57:15.281Z","logger":"kafkachannel-webhook.ValidationWebhook","caller":"controller/controller.go:557","msg":"Requeuing key validation.webhook.kafka.messaging.knative.dev due to non-permanent error (depth: 0)"}

This was caused by changes in the knative.dev/pkg common webhook reconciliation logic (PR # 2098) which was pulled into the eventing-kafka repo on 6/15/21 by PR #713. The new logic needs to be able to get the knative-eventing namespace and the webhook doesn't currently have permissions to do so.

Proposed Changes

  • 🐛 Provide Webhook with get permissions for namespaces as required by new knative.dev/pkg reconcilers.

Note: Once this is approved/merged I will create a backport PR to fix the release-0.24 branch which also has the new/latest knative.dev/pkg code.

@travis-minke-sap travis-minke-sap added the bug Something isn't working label Jul 1, 2021
@google-cla google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 1, 2021
@knative-prow-robot knative-prow-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2021
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: travis-minke-sap

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:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2021
@codecov
Copy link

codecov bot commented Jul 1, 2021

Codecov Report

Merging #751 (02bf89a) into main (000dd9c) will increase coverage by 0.42%.
The diff coverage is 78.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   75.29%   75.72%   +0.42%     
==========================================
  Files         132      140       +8     
  Lines        5785     6327     +542     
==========================================
+ Hits         4356     4791     +435     
- Misses       1208     1316     +108     
+ Partials      221      220       -1     
Impacted Files Coverage Δ
...onsolidated/dispatcher/consumer_message_handler.go 0.00% <0.00%> (ø)
.../channel/distributed/controller/util/dispatcher.go 100.00% <ø> (ø)
pkg/source/adapter/adapter.go 58.42% <ø> (-1.08%) ⬇️
pkg/source/client/client.go 15.66% <0.00%> (-19.48%) ⬇️
pkg/source/reconciler/source/kafkasource.go 0.00% <0.00%> (ø)
pkg/source/mtadapter/adapter.go 47.82% <22.72%> (-27.36%) ⬇️
pkg/apis/sources/v1beta1/kafka_lifecycle.go 53.33% <50.00%> (-0.52%) ⬇️
pkg/channel/consolidated/utils/util.go 95.91% <50.00%> (-4.09%) ⬇️
pkg/source/client/offsets.go 60.00% <60.00%> (ø)
pkg/channel/consolidated/dispatcher/dispatcher.go 61.58% <66.66%> (+0.10%) ⬆️
... and 31 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 f0652fd...02bf89a. Read the comment docs.

@travis-minke-sap
Copy link
Contributor Author

/test all

@eric-sap
Copy link
Contributor

eric-sap commented Jul 2, 2021

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021
@travis-minke-sap
Copy link
Contributor Author

/test pull-knative-sandbox-eventing-kafka-unit-tests

@eric-sap
Copy link
Contributor

eric-sap commented Jul 2, 2021

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 2, 2021
@travis-minke-sap
Copy link
Contributor Author

grrrr....
/test pull-knative-sandbox-eventing-kafka-upgrade-tests

@knative-prow-robot knative-prow-robot merged commit f6cdc01 into knative-extensions:main Jul 2, 2021
@travis-minke-sap travis-minke-sap deleted the missing-webhook-rbac branch July 2, 2021 15:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bug Something isn't working cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants