Support TLS in Trigger and Channel reconciler by pierDipi · Pull Request #6988 · knative/eventing

pierDipi · 2023-05-29T11:27:33Z

This is looking at making the control plane wiring and resolving for the
triggers Eventing TLS aware.

To support Eventing TLS in trigger reconciler, we need to make sure that
the underlying channel is properly propagating the Status and that the
subscription URI is set to the https endpoint when the TLS feature is
enabled (permissive or strict)

Individual commits might contain more details on the why of certain
changes.

Proposed Changes

Support TLS in Trigger reconciler
Set CA certs fields for channel, subscription, broker and trigger resources related to subscriber, reply and dead letter sink

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

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

This will reduce and, at the same time, fix the direct access to DeadLetterSinkURI field which is not correct in the TLS case. Such wrong direct access assumes a certain structure of the DeliveryStatus type increasing coupling between components. Moving forward we can use: - NewDeliveryStatusFromAddressable - NewDestinationFromDeliveryStatus that encapsulate the common pattern of: - creating DeliveryStatus from an Addressable returned by the Resolver - creating a Destination from a DeliveryStatus (for reconcilers that wiring other resources) Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow · 2023-05-29T11:27:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

codecov · 2023-05-29T13:31:34Z

Codecov Report

Patch coverage: 77.67% and project coverage change: -0.11 ⚠️

Comparison is base (59118a0) 79.61% compared to head (e68bcaf) 79.51%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6988      +/-   ##
==========================================
- Coverage   79.61%   79.51%   -0.11%     
==========================================
  Files         246      246              
  Lines       12991    13070      +79     
==========================================
+ Hits        10343    10392      +49     
- Misses       2131     2159      +28     
- Partials      517      519       +2

Impacted Files	Coverage Δ
pkg/apis/duck/v1/delivery_types.go	`73.07% <0.00%> (-21.93%)`	⬇️
pkg/reconciler/channel/channel.go	`70.00% <0.00%> (ø)`
pkg/apis/messaging/v1/channel_lifecycle.go	`81.03% <75.00%> (ø)`
pkg/reconciler/broker/trigger/trigger.go	`82.71% <79.06%> (-2.22%)`	⬇️
...g/apis/messaging/v1/in_memory_channel_lifecycle.go	`74.60% <87.50%> (ø)`
pkg/apis/eventing/v1/broker_lifecycle.go	`100.00% <100.00%> (ø)`
pkg/apis/eventing/v1/test_helper.go	`92.30% <100.00%> (ø)`
pkg/reconciler/broker/broker.go	`77.35% <100.00%> (-0.32%)`	⬇️
pkg/reconciler/broker/controller.go	`87.83% <100.00%> (+1.47%)`	⬆️
pkg/reconciler/broker/resources/subscription.go	`100.00% <100.00%> (ø)`
... and 3 more

... and 16 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.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-05-31T10:34:20Z

/test upgrade-tests

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-05-31T12:38:46Z

/cc @creydr @vishal-chdhry

pierDipi · 2023-05-31T13:09:02Z

/test unit-tests

matzew · 2023-05-31T13:14:57Z

pkg/apis/eventing/v1beta1/eventtype_lifecycle_test.go

 			got := ets.GetTopLevelCondition().Status
 			if test.wantConditionStatus != got {
-				t.Errorf("unexpected readiness: want %v, got %v", test.wantConditionStatus, got)
+				t.Errorf("unexpected readiness: want %v, got %v\n%+v", test.wantConditionStatus, got, ets)


wanna apply to v1b2 as well?

creydr · 2023-05-31T18:40:40Z

pkg/apis/messaging/v1/in_memory_channel_lifecycle.go

+func (imcs *InMemoryChannelStatus) MarkDeadLetterSinkResolvedSucceeded(deadLetterSinkURI eventingduck.DeliveryStatus) {
+	imcs.DeliveryStatus = deadLetterSinkURI


Suggested change

func (imcs *InMemoryChannelStatus) MarkDeadLetterSinkResolvedSucceeded(deadLetterSinkURI eventingduck.DeliveryStatus) {

imcs.DeliveryStatus = deadLetterSinkURI

func (imcs *InMemoryChannelStatus) MarkDeadLetterSinkResolvedSucceeded(deadLetterSink eventingduck.DeliveryStatus) {

imcs.DeliveryStatus = deadLetterSink

creydr · 2023-05-31T19:12:15Z

pkg/reconciler/subscription/subscription.go


-		logging.FromContext(ctx).Debugw("Resolved deadLetterSink", zap.String("deadLetterSinkURI", deadLetterSinkURI.String()))
-		subscription.Status.PhysicalSubscription.DeadLetterSinkURI = deadLetterSinkURI
+		logging.FromContext(ctx).Debugw("Resolved deadLetterSink", zap.String("deadLetterSinkURI", deadLetterSinkAddr.URL.String()))


nit: WDYT about logging the whole DLS object (including the ca certs)? Just to have this information and it's on debug level anyhow...

creydr · 2023-05-31T19:14:30Z

pkg/reconciler/testing/v1/channel.go

+func WithChannelStatusDLSURI(dlsURI eventingduckv1.DeliveryStatus) ChannelOption {
 	return func(c *eventingv1.Channel) {
 		c.Status.MarkDeadLetterSinkResolvedSucceeded(dlsURI)


Suggested change

func WithChannelStatusDLSURI(dlsURI eventingduckv1.DeliveryStatus) ChannelOption {

return func(c *eventingv1.Channel) {

c.Status.MarkDeadLetterSinkResolvedSucceeded(dlsURI)

func WithChannelStatusDLSURI(dls eventingduckv1.DeliveryStatus) ChannelOption {

return func(c *eventingv1.Channel) {

c.Status.MarkDeadLetterSinkResolvedSucceeded(dls)

creydr · 2023-05-31T19:16:04Z

pkg/reconciler/testing/v1/inmemorychannel.go

 }

-func WithInMemoryChannelStatusDLSURI(dlsURI *apis.URL) InMemoryChannelOption {
+func WithInMemoryChannelStatusDLS(dlsURI *duckv1.Addressable) InMemoryChannelOption {


Suggested change

func WithInMemoryChannelStatusDLS(dlsURI *duckv1.Addressable) InMemoryChannelOption {

func WithInMemoryChannelStatusDLS(dls *duckv1.Addressable) InMemoryChannelOption {

creydr · 2023-05-31T19:16:27Z

pkg/reconciler/testing/v1/inmemorychannel.go

+		if dlsURI == nil {
+			imc.Status.MarkDeadLetterSinkNotConfigured()
+			return
+		}
+		imc.Status.MarkDeadLetterSinkResolvedSucceeded(eventingv1.NewDeliveryStatusFromAddressable(dlsURI))


Suggested change

if dlsURI == nil {

imc.Status.MarkDeadLetterSinkNotConfigured()

return

}

imc.Status.MarkDeadLetterSinkResolvedSucceeded(eventingv1.NewDeliveryStatusFromAddressable(dlsURI))

if dls == nil {

imc.Status.MarkDeadLetterSinkNotConfigured()

return

}

imc.Status.MarkDeadLetterSinkResolvedSucceeded(eventingv1.NewDeliveryStatusFromAddressable(dls))

creydr · 2023-05-31T19:22:14Z

pkg/reconciler/testing/v1/subscription.go

 }

-func WithSubscriptionDeadLetterSinkURI(uri *apis.URL) SubscriptionOption {
+func WithSubscriptionDeadLetterSinkURI(uri *duckv1.Addressable) SubscriptionOption {


Shouldn't we rename the function (and its parameters), as we don't take an URI anymore?

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi · 2023-06-01T10:04:14Z

@matzew @creydr I think I covered your comments, PTAL

pierDipi · 2023-06-01T10:16:32Z

/test unit-tests

creydr

Looks good so far.

/lgtm

Putting it on hold, to give others a bit time to review too. Feel free to unhold when you're fine
/hold

vishal-chdhry · 2023-06-01T13:18:05Z

/lgtm

pierDipi · 2023-06-05T09:36:00Z

/unhold

This is looking at making the control plane wiring and resolving for the triggers Eventing TLS aware. To support Eventing TLS in trigger reconciler, we need to make sure that the underlying channel is properly propagating the Status and that the subscription URI is set to the https endpoint when the TLS feature is enabled (permissive or strict) Individual commits might contain more details on the why of certain changes.  ## Proposed Changes  - Support TLS in Trigger reconciler - Set CA certs fields for channel, subscription, broker and trigger resources related to subscriber, reply and dead letter sink ### 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  --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi added 9 commits May 29, 2023 12:05

Add DeadLetterSinkCACerts to DeliveryStatus

d121431

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Make trigger reconciler TLS aware

d867f4e

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Make subscription reconciler TLS aware

3868d6f

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Make IMC controller TLS aware

ce044cc

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Initialize feature flag store in Broker reconciler

39684ed

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Adapt Channel lifecycle methods TLS changes

4158a97

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Refactor Broker and Channel tests to be TLS aware

8c1bb9a

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Add various CA certs fields to Broker and Channel resources

b057e28

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.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 29, 2023

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 29, 2023

knative-prow bot requested review from aslom and evankanderson May 29, 2023 11:27

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

pierDipi added 3 commits May 29, 2023 13:45

Update codegen

79380d5

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Additional CA certs fields on CRDs schema

f399e75

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Fix unit tests

9df57b0

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi self-assigned this May 29, 2023

pierDipi changed the title ~~[WIP] Support TLS in Trigger reconciler~~ Support TLS in Trigger reconciler 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

pierDipi changed the title ~~Support TLS in Trigger reconciler~~ [WIP] Support TLS in Trigger reconciler May 31, 2023

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 31, 2023

pierDipi changed the title ~~[WIP] Support TLS in Trigger reconciler~~ [WIP] Support TLS in Trigger and Channel reconciler May 31, 2023

pierDipi added 2 commits May 31, 2023 11:36

Add trigger tests

a61b3b4

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Add Channel dead letter sink tests

175dc12

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 31, 2023

Refactoring + rename + format Go code

1fecd25

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

pierDipi changed the title ~~[WIP] Support TLS in Trigger and Channel reconciler~~ Support TLS in Trigger and Channel reconciler 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 requested review from creydr and vishal-chdhry May 31, 2023 12:38

matzew reviewed May 31, 2023

View reviewed changes

creydr reviewed May 31, 2023

View reviewed changes

creydr mentioned this pull request May 31, 2023

Add TLS support for mt-broker-filter #6940

Merged

5 tasks

pierDipi added 2 commits June 1, 2023 12:02

Renaming

1d461ee

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

Add full ET to test error in v1b2

e68bcaf

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

creydr reviewed Jun 1, 2023

View reviewed changes

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

knative-prow bot assigned creydr Jun 1, 2023

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

knative-prow bot assigned vishal-chdhry Jun 1, 2023

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 5, 2023

knative-prow bot merged commit dfb2243 into knative:main Jun 5, 2023

		func (imcs *InMemoryChannelStatus) MarkDeadLetterSinkResolvedSucceeded(deadLetterSinkURI eventingduck.DeliveryStatus) {
		imcs.DeliveryStatus = deadLetterSinkURI

	func WithInMemoryChannelStatusDLS(dlsURI *duckv1.Addressable) InMemoryChannelOption {
	func WithInMemoryChannelStatusDLS(dls *duckv1.Addressable) InMemoryChannelOption {

Conversation

pierDipi commented May 29, 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 29, 2023

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

pierDipi commented May 31, 2023

Uh oh!

pierDipi commented May 31, 2023

Uh oh!

pierDipi commented May 31, 2023

Uh oh!

matzew May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

creydr May 31, 2023

Choose a reason for hiding this comment

Uh oh!

pierDipi commented Jun 1, 2023

Uh oh!

pierDipi commented Jun 1, 2023

Uh oh!

creydr left a comment

Choose a reason for hiding this comment

Uh oh!

vishal-chdhry commented Jun 1, 2023

Uh oh!

pierDipi commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pierDipi commented May 29, 2023 •

edited

Loading

codecov bot commented May 29, 2023 •

edited

Loading