Add TLS support for mt-broker-filter by creydr · Pull Request #6940 · knative/eventing

creydr · 2023-05-10T09:06:32Z

Currently includes 98a98cd as a cherry-pick from #6983.

Proposed Changes

🎁 Add TLS support for mt-broker-filter

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Add TLS support for mt-broker-filter

knative-prow · 2023-05-10T09:06:35Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-05-16T07:03:51Z

Codecov Report

Patch coverage: 73.27% and project coverage change: +0.16 🎉

Comparison is base (65f4b1c) 79.51% compared to head (e757372) 79.67%.

❗ Current head e757372 differs from pull request most recent head e860f9a. Consider uploading reports for the commit e860f9a to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6940      +/-   ##
==========================================
+ Coverage   79.51%   79.67%   +0.16%     
==========================================
  Files         246      247       +1     
  Lines       13070    12973      -97     
==========================================
- Hits        10392    10336      -56     
+ Misses       2159     2126      -33     
+ Partials      519      511       -8

Impacted Files	Coverage Δ
pkg/broker/filter/server_manager.go	`0.00% <0.00%> (ø)`
pkg/eventingtls/eventingtls.go	`21.17% <0.00%> (ø)`
pkg/kncloudevents/message_sender.go	`91.66% <ø> (ø)`
pkg/adapter/v2/main_message.go	`50.00% <71.42%> (ø)`
...iler/inmemorychannel/dispatcher/inmemorychannel.go	`83.46% <72.72%> (+11.75%)`	⬆️
pkg/channel/message_dispatcher.go	`75.00% <88.23%> (ø)`
pkg/broker/filter/filter_handler.go	`76.69% <100.00%> (+1.18%)`	⬆️
pkg/channel/fanout/fanout_message_handler.go	`87.23% <100.00%> (ø)`
...nnelfanout/multi_channel_fanout_message_handler.go	`84.31% <100.00%> (-2.58%)`	⬇️
pkg/kncloudevents/utils.go	`70.00% <100.00%> (ø)`
... and 1 more

... and 24 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/broker/filter/server_manager.go

creydr · 2023-05-31T05:42:06Z

/retest

creydr · 2023-05-31T19:55:18Z

Need changes from #6988
/hold

pierDipi

Looks good overall:

a few missing copyright headers on YAMLs
q: where do we call "CloseIdleConnections" on various HTTP(S) clients?

test/rekt/features/trigger/feature.go

creydr · 2023-06-01T12:44:56Z

a few missing copyright headers on YAMLs

👍 Added them

q: where do we call "CloseIdleConnections" on various HTTP(S) clients?

I didn't change much logic there IIRC. We could (and probably should) add some garbage collection to the kncloudevents library, to remove old clients, and could also include something to close idling connections. But I would see this in a separate PR.

test/rekt/features/trigger/feature.go

creydr · 2023-06-12T09:00:38Z

/unhold
as #6988 merged

creydr · 2023-06-12T11:19:07Z

/test upgrade-tests

pkg/broker/filter/server_manager.go

pierDipi · 2023-06-12T11:49:44Z

/approve

knative-prow · 2023-06-12T11:49:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, pierDipi

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~~ [pierDipi]

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

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

creydr · 2023-06-12T13:42:40Z

/test upgrade-tests

creydr · 2023-06-12T14:18:22Z

/test reconciler-tests

pierDipi · 2023-06-12T14:55:46Z

/lgtm

pierDipi · 2023-06-12T14:56:06Z

Thanks @creydr !

Fixes knative#6877 Currently includes 98a98cd as a cherry-pick from knative#6983. ## Proposed Changes - 🎁 Add TLS support for mt-broker-filter ### Pre-review Checklist - [ ] **At least 80% unit test coverage** - [ ] **E2E tests** for any new behavior - [ ] **Docs PR** for any user-facing impact - [ ] **Spec PR** for any new API feature - [ ] **Conformance test** for any change to the spec **Release Note** ```release-note Add TLS support for mt-broker-filter ``` --------- Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2023

knative-prow bot requested review from matzew and odacremolbap May 10, 2023 09:06

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 10, 2023

creydr force-pushed the add-tls-support-to-mt-broker-filter branch 5 times, most recently from 5881605 to 5e21a99 Compare May 16, 2023 06:52

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 6c36d11 to 78a7726 Compare May 25, 2023 15:28

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 25, 2023

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 78a7726 to e757372 Compare May 25, 2023 15:55

creydr marked this pull request as ready for review May 25, 2023 20:01

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2023

vishal-chdhry reviewed May 28, 2023

View reviewed changes

pkg/broker/filter/server_manager.go Outdated Show resolved Hide resolved

knative-prow bot requested a review from pierDipi May 28, 2023 13:37

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from e757372 to 0165e16 Compare May 30, 2023 14:35

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 30, 2023

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2023

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 0165e16 to 87feb4f Compare May 31, 2023 05:57

knative-prow bot added the area/test-and-release Test infrastructure, tests or release label May 31, 2023

creydr changed the title ~~[WIP] Add TLS support for mt-broker-filter~~ Add TLS support for mt-broker-filter May 31, 2023

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2023

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 31, 2023

pierDipi reviewed Jun 1, 2023

View reviewed changes

test/rekt/features/trigger/feature.go Outdated Show resolved Hide resolved

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 8b526d5 to 14d9802 Compare June 1, 2023 11:23

creydr added 5 commits June 12, 2023 08:51

Add permissions for mt-broker-filter to read tls secret

22e085a

Add https handler for mt-broker-filter

07e0028

Log only warning if TLS secret is not found

32a2c78

Update filter handler to use CloudEventsRequest

108379b

Register client handlers in filter-handler

bed32ef

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 14d9802 to 7ff797a Compare June 12, 2023 06:51

pierDipi reviewed Jun 12, 2023

View reviewed changes

test/rekt/features/trigger/feature.go Outdated Show resolved Hide resolved

Add e2e test for trigger with TLS subscriber

eeb120d

creydr force-pushed the add-tls-support-to-mt-broker-filter branch from 7ff797a to eeb120d Compare June 12, 2023 08:54

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 12, 2023

creydr requested a review from pierDipi June 12, 2023 10:26

pierDipi reviewed Jun 12, 2023

View reviewed changes

pkg/broker/filter/server_manager.go Outdated Show resolved Hide resolved

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2023

Remove comment

e860f9a

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

knative-prow bot assigned pierDipi Jun 12, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 12, 2023

knative-prow bot merged commit 631f4ec into knative:main Jun 12, 2023

Comments

Conversation

creydr commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

knative-prow bot commented May 10, 2023

Uh oh!

codecov bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

creydr commented May 31, 2023

Uh oh!

creydr commented May 31, 2023

Uh oh!

pierDipi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

creydr commented Jun 1, 2023

Uh oh!

Uh oh!

creydr commented Jun 12, 2023

Uh oh!

creydr commented Jun 12, 2023

Uh oh!

Uh oh!

pierDipi commented Jun 12, 2023

Uh oh!

knative-prow bot commented Jun 12, 2023

Uh oh!

creydr commented Jun 12, 2023

Uh oh!

creydr commented Jun 12, 2023

Uh oh!

pierDipi commented Jun 12, 2023

Uh oh!

pierDipi commented Jun 12, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

creydr commented May 10, 2023 •

edited

Loading

codecov bot commented May 16, 2023 •

edited

Loading