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

Use shared observability config in broker data plane #1426

Merged
merged 4 commits into from
Jun 19, 2019

Conversation

grantr
Copy link
Contributor

@grantr grantr commented Jun 18, 2019

Fixes #1416

Proposed Changes

  • Update ingress and filter pods to watch for observability ConfigMaps in the system namespace (normally knative-eventing) instead of the Broker's namespace. This removes the ability to create per-namespace configs, but we think that use case is much less likely than the cluster-wide config case.
  • Add the SYSTEM_NAMESPACE env var to ingress and filter podspecs to inform knative/pkg where the system namespace is.
  • When creating a default Broker via injection, add a RoleBinding in the system namespace allowing ConfigMap read from the ingress and filter service accounts in the Broker's namespace. Users creating non-injected Brokers will need to create this RoleBinding manually (and all other RBAC objects).

Release Note

All Broker ingress and filter pods now retrieve observability config from cluster-wide ConfigMaps (normally in the `knative-eventing` namespace) instead of per-namespace ConfigMaps.

Needed to watch the shared config configmaps in knative-eventing.
Broker data plane pods live in the Broker namespace, but observability
config lives in the system namespace (knative-eventing). When
auto-creating a default Broker and its RBAC, also create a RoleBinding
allowing read access to the shared config.

The existing permission to read configmaps in the Broker's namespace is
retained.
Switch to watching the shared configmaps instead of per-namespace
configmaps. This will be easier to manage for the operator and more
predictable for the user.
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 18, 2019
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 18, 2019
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2019
@grantr
Copy link
Contributor Author

grantr commented Jun 18, 2019

/milestone v0.7.0

@grantr grantr added this to the v0.7.0 milestone Jun 18, 2019
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/namespace/namespace.go 77.3% 76.2% -1.1

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

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2019
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: grantr, n3wscott

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 merged commit 6acbe3e into knative:master Jun 19, 2019
@grantr grantr deleted the shared-observability-config branch June 19, 2019 18:38
grantr added a commit to grantr/docs that referenced this pull request Jun 19, 2019
knative-prow-robot pushed a commit to knative/docs that referenced this pull request Jun 20, 2019
* Add new RoleBindings to broker manual setup

These were added in knative/eventing#1426.

* Remove commands added by accident
matzew added a commit to matzew/eventing that referenced this pull request Sep 29, 2021
Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Co-authored-by: Knative Prow Robot <knative-prow-robot@google.com>
Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. 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.

[BUG] Broker tracing requires a config map in the broker's namespace
5 participants