Create a ServiceAccount for Broker Ingress by Harwayne · Pull Request #1241 · knative/eventing

Harwayne · 2019-05-15T23:13:31Z

Helps with #704, as this SA will be needed to get ConfigMap reading permissions.

Proposed Changes

Use the Service Account eventing-broker-ingress, rather than default on Broker Ingress Pods.
Create the Service Account via the namespace annotation.
Add the instructions to create the Service Account manually for those that don't use the annotation.

Release Note

If you are using a Broker in a namespace not annotated with eventing injection, then you will need to follow the [manual setup instructions](https://github.com/knative/eventing/tree/master/docs/broker#manual-setup) to create the required ServiceAccount and RoleBinding. 

Note that failing to do so will not cause immediately noticable failures. Only after a subsequent PR starts watching ConfigMaps will the failure begin to manifest, in the form of crash looping Broker Ingress Pods.

…or Broker Ingress.

knative-prow-robot · 2019-05-15T23:13:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Harwayne

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Harwayne]

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

Harwayne · 2019-05-20T16:56:56Z

Ping.

nachocano · 2019-05-20T20:17:47Z

 	brokerCreated             = "BrokerCreated"
-	serviceAccountCreated     = "BrokerFilterServiceAccountCreated"
-	serviceAccountRBACCreated = "BrokerFilterServiceAccountRBACCreated"
+	serviceAccountCreated     = "BrokerServiceAccountCreated"


might be better to have the 4 events: BrokerFilterXXX and BrokerIngressXXX, and you pass them as args when you call the method?
But I'm ok either way, if you want to leave it as is, it's fine.

I guess with the message you are printing the name of the service account or role binding. We can leave it as is...

nachocano · 2019-05-20T20:28:17Z


 In order to setup a `Broker` manually, we must first create the required
-`ServiceAccount` and give it the proper RBAC permissions. This setup is required
+`ServiceAccount`s and give them the proper RBAC permissions. This setup is required


Now that you are editing this file... can you fix the cosmetic:

the implmentation may change at any time, absolutely no guarantees are made about the implmentation

Also, inside Implementation, Namespace, you will have to add the steps of creating the eventing-broker-filter service account, and the RBAC.

Now that is fresh, there is a broker_trigger.md file in knative/docs, with this manual steps as well. It'd be great if you can create a follow up PR on docs with this same changes...

Made the changes here.

Once this gets in, I'll immediately open a PR on docs to update it their too.

Maybe we should just delete this file? And rely on docs only?

Yes, I think it'd be better to remove this one. Just check if it has the same content. I think the docs one doesn't have the implementation, but I think it's fine to skip that section.

OK. In a follow up PR (after I submit the changes to docs), I'll pair this down to a link to docs and implementation details.

Sounds good!

knative-metrics-robot · 2019-05-20T21:41:53Z

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	Do not exist	79.8%

nachocano · 2019-05-20T22:02:15Z

/lgtm

* pr#1408 * pr#1398 * service/samples: add .dockerignore to samples with local build disruption risk (#1392) * service/samples: add .dockerignore to samples with risk of local build interference * serving/samples: .dockerignore readme text update * Update docs/serving/samples/hello-world/helloworld-csharp/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-nodejs/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-python/README.md Co-Authored-By: RichieEscarez <rescarez@google.com> * Update docs/serving/samples/hello-world/helloworld-php/README.md Co-Authored-By: Evan Anderson <evan.k.anderson@gmail.com> * Fix the wrong information in grpc example (#1380) * pr#1363 * Fix samples path (#1357) * Copy the updates from knative/eventing#1241. (#1355) * add reference to custom DNS config (#1327) * add reference to custom DNS config * Update docs/install/getting-started-knative-app.md Co-Authored-By: Sam O'Dell <31352624+samodell@users.noreply.github.com> * fix relative link * Fix PR#1202 * Document serving tag resolution (#1260) Our controller does resolution of tags to digests, which has been a source of confusion. This documents the fact that we do it, why we do it, and how to configure the controller to work around common issues.

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Harwayne added 2 commits May 15, 2019 15:00

Namespace reconciler creates a service account and adds RoleBinding f…

4f74fcf

…or Broker Ingress.

Make a single function for reconcileBrokerServiceAccountAndRBAC.

e3aedf6

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 15, 2019

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2019

knative-prow-robot requested review from grantr and matzew May 15, 2019 23:13

Add the Ingress SA creation to the 'manual' test.

8464d25

nachocano reviewed May 20, 2019

View reviewed changes

Comment thread pkg/reconciler/namespace/namespace.go Outdated

nachocano reviewed May 20, 2019

View reviewed changes

Comment thread pkg/reconciler/namespace/namespace.go Outdated

nachocano reviewed May 20, 2019

View reviewed changes

Comment thread pkg/reconciler/namespace/namespace.go Outdated

nachocano reviewed May 20, 2019

View reviewed changes

Comment thread pkg/reconciler/namespace/namespace.go Outdated

PR comments.

fcdced0

knative-prow-robot assigned nachocano May 20, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2019

knative-prow-robot merged commit 3f8ecf9 into knative:master May 20, 2019

Harwayne added a commit to Harwayne/knative-docs that referenced this pull request May 20, 2019

Copy the updates from knative/eventing#1241.

907baba

Harwayne mentioned this pull request May 20, 2019

Copy the updates from knative/eventing#1241. knative/docs#1355

Merged

knative-prow-robot pushed a commit to knative/docs that referenced this pull request May 21, 2019

Copy the updates from knative/eventing#1241. (#1355)

b2bbbb0

Harwayne mentioned this pull request May 21, 2019

Remove the redundant Broker and Trigger documentation. #1257

Merged

Harwayne deleted the ingress-sa branch May 29, 2019 17:37

RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Jun 6, 2019

Copy the updates from knative/eventing#1241. (knative#1355)

02b5864

RichieEscarez pushed a commit to RichieEscarez/docs that referenced this pull request Jun 12, 2019

Copy the updates from knative/eventing#1241. (knative#1355)

448ad40

nachocano mentioned this pull request Oct 29, 2019

Adding nachocano to approvers #2118

Merged

markusthoemmes pushed a commit to markusthoemmes/knative-eventing that referenced this pull request May 17, 2021

💄 using logger.info instead of header_text (knative#1240) (knative#1241)

45c8823

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>

Conversation

Harwayne commented May 15, 2019

Proposed Changes

Uh oh!

knative-prow-robot commented May 15, 2019

Uh oh!

Harwayne commented May 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-metrics-robot commented May 20, 2019

Uh oh!

nachocano commented May 20, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants