use the composite prober with the channel #3252

Rahul-Kumar-prog · 2023-07-31T12:02:00Z

Proposed Changes

Update the channel controller to use the prober with tls if the transport encryption flag is set to strict.

Release Note

The Channel now uses a tls-enabled prober if the transport encryption flag is set to strict.

Docs

knative-prow · 2023-07-31T12:02:10Z

Hi @Rahul-Kumar-prog. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-07-31T12:11:49Z

Codecov Report

Merging #3252 (8aa2a7f) into main (e7017ef) will decrease coverage by 3.20%.
Report is 2 commits behind head on main.
The diff coverage is 45.28%.

@@             Coverage Diff              @@
##               main    #3252      +/-   ##
============================================
- Coverage     61.54%   58.34%   -3.20%     
============================================
  Files           181       91      -90     
  Lines         12322     9279    -3043     
  Branches        265        0     -265     
============================================
- Hits           7583     5414    -2169     
+ Misses         4142     3431     -711     
+ Partials        597      434     -163

Flag	Coverage Δ
java-unittests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
control-plane/pkg/reconciler/channel/channel.go	`69.73% <100.00%> (+0.62%)`	⬆️
...ntrol-plane/pkg/reconciler/channel/v2/channelv2.go	`71.52% <100.00%> (+0.56%)`	⬆️
...ol-plane/pkg/reconciler/channel/v2/controllerv2.go	`78.02% <22.22%> (-14.09%)`	⬇️
control-plane/pkg/reconciler/channel/controller.go	`78.18% <21.05%> (-12.25%)`	⬇️

... and 91 files with indirect coverage changes

Cali0707

This is a good start @Rahul-Kumar-prog! Can you look into why your unit tests are failing on this PR:

# knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel_test [knative.dev/eventing-kafka-broker/control-plane/pkg/reconciler/channel.test]
Error: control-plane/pkg/reconciler/channel/channel_test.go:2134:23: cannot use proberMock (variable of type prober.Prober) as prober.NewProber value in struct literal: prober.Prober does not implement prober.NewProber (wrong type for method Probe)
		have Probe("context".Context, prober.Addressable, prober.Status) prober.Status
		want Probe("context".Context, prober.NewAddressable, prober.Status) prober.Status

It looks to me like we need to update the unit tests as well, similar to what I did in https://github.com/knative-sandbox/eventing-kafka-broker/pull/3175/files in the broker_test.go file.

Rahul-Kumar-prog · 2023-07-31T13:29:36Z

@Cali0707 How can I identify this failing cause ?

Cali0707 · 2023-07-31T13:31:46Z

@Rahul-Kumar-prog look at the changes I made to the broker_test.go file in https://github.com/knative-sandbox/eventing-kafka-broker/pull/3175/files

Rahul-Kumar-prog · 2023-07-31T13:52:09Z

@Cali0707 Do I have to make changes in all the files that you have changed? Or similarly? Because you make changes in 15 file for the same kind of issue?

Cali0707 · 2023-07-31T14:01:49Z

@Rahul-Kumar-prog you just have to follow the changes I made to broker_test.go in that PR in channel_test.go here.

Rahul-Kumar-prog · 2023-08-02T09:04:09Z

@Cali0707 @pierDipi have a look.

Cali0707 · 2023-08-02T13:53:46Z

/ok-to-test

Cali0707

@Rahul-Kumar-prog thanks for your work so far!

If you look at the issue, it also mentions implementing this for channel v2. Do you think you could make the same changes you have made here for channel v2?

Rahul-Kumar-prog · 2023-08-02T14:26:36Z

Ohh i am so sorry i forget about that one sorry.

defiantly i can do. Thank you for letting me know.

Cali0707 · 2023-08-02T14:55:48Z

/retest-required

Cali0707

Thanks for adding the channelv2 changes @Rahul-Kumar-prog !

control-plane/pkg/reconciler/channel/v2/controllerv2.go

Cali0707 · 2023-08-02T16:49:56Z

/retest-required

Cali0707

@Rahul-Kumar-prog sorry I missed this earlier, but you need to add the address into the Addresses field for all of the duckv1.AddressStatus fields you are creating yourself.

control-plane/pkg/reconciler/channel/channel.go

control-plane/pkg/reconciler/channel/v2/channelv2.go

Cali0707 · 2023-10-04T18:51:17Z

@Rahul-Kumar-prog can you rebase this PR to have the newest version of the changes on main? I think that will likely fix your failing tests, as there have been a few bug fixes on the prober since you opened this PR

Rahul-Kumar-prog · 2023-10-07T17:40:35Z

@Cali0707 can you please retest ?

Cali0707

@Rahul-Kumar-prog I found a few more places in your code which are causing some of the failing tests, if you make the changes I suggested more (hopefully all) of the tests will pass. The question I had for @pierDipi won't block any current tests from passing, but may affect whether this works when we add tls tests for the channel

control-plane/pkg/reconciler/channel/controller.go

control-plane/pkg/reconciler/channel/v2/controllerv2.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

pierDipi · 2023-10-11T12:41:55Z

/retest-required

Cali0707 · 2023-10-11T14:02:27Z

/retest-required

Cali0707

@Rahul-Kumar-prog @pierDipi per earlier conversation, these changes will set the port correctly for the case when we want to probe with TLS

control-plane/pkg/reconciler/channel/controller.go

control-plane/pkg/reconciler/channel/v2/controllerv2.go

Co-authored-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2023-10-12T15:42:52Z

Btw @Rahul-Kumar-prog, you normally don't need to merge the main branch into your PR unless you:

want to use new code from the main branch
Have a merge conflict

Otherwise, Prow will handle that for you once the PR is approved

Rahul-Kumar-prog · 2023-10-12T15:47:28Z

@Cali0707 thank you for telling I was not aware of that.

Cali0707 · 2023-10-12T17:10:27Z

/retest-required

Cali0707 · 2023-10-12T20:31:12Z

/retest-required

Cali0707

/lgtm
/approve
/hold in case @pierDipi wants any changes

knative-prow · 2023-10-12T20:33:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, Rahul-Kumar-prog

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:

~~OWNERS~~ [Cali0707]

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

Cali0707 · 2023-10-12T21:57:44Z

/retest-required

pierDipi · 2023-10-13T13:12:59Z

/test channel-integration-tests-ssl

pierDipi · 2023-10-13T13:57:17Z

/unhold

* using the composite prober for the channel * edit channel_test.go file * change channel_test.go * done changes in V2 channel * edit in controllerv2.go * edit addresses * 2nd edit addresses * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> --------- Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Calum Murray <cmurray@redhat.com>

#3406 (#3497) * use the composite prober with the channel (#3252) * using the composite prober for the channel * edit channel_test.go file * change channel_test.go * done changes in V2 channel * edit in controllerv2.go * edit addresses * 2nd edit addresses * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> --------- Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Calum Murray <cmurray@redhat.com> * E2E tests for channel: TLS key pair rotation (#3406) * Save work progress * Expose the TLS port * Adding the logger to see what is happening * Java - Adding the debugging information * Adding the path to the contract * Comment out the certificate rotation test portion * Resolve the source certificate not found issue * Fix the issue in the test * Update control-plane/pkg/prober/prober.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/resources/service.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java Co-authored-by: Calum Murray <cmurray@redhat.com> * Fix the inconsistent varable name * Fix the failed build issue * Remove the logger * Run formatting * Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Co-authored-by: Calum Murray <cmurray@redhat.com> * Remove the logger * Code gen * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Remove the uncessary code * Fix the failing reconciler tests due to the missing newly added filed in the test * Format fix * Instead of using channel service name, we directly use channel name for Path * Instead of using channel service name, we directly use channel name for Path --------- Co-authored-by: Calum Murray <cmurray@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Update the function name --------- Co-authored-by: Rahul kumar <68837569+Rahul-Kumar-prog@users.noreply.github.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Calum Murray <cmurray@redhat.com>

knative-extensions#3406 (knative-extensions#3497) * use the composite prober with the channel (knative-extensions#3252) * using the composite prober for the channel * edit channel_test.go file * change channel_test.go * done changes in V2 channel * edit in controllerv2.go * edit addresses * 2nd edit addresses * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> * Update prober pod port for TLS Co-authored-by: Calum Murray <cmurray@redhat.com> --------- Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Calum Murray <cmurray@redhat.com> * E2E tests for channel: TLS key pair rotation (knative-extensions#3406) * Save work progress * Expose the TLS port * Adding the logger to see what is happening * Java - Adding the debugging information * Adding the path to the contract * Comment out the certificate rotation test portion * Resolve the source certificate not found issue * Fix the issue in the test * Update control-plane/pkg/prober/prober.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update control-plane/pkg/reconciler/channel/resources/service.go Co-authored-by: Calum Murray <cmurray@redhat.com> * Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/IngressProducerReconcilableStore.java Co-authored-by: Calum Murray <cmurray@redhat.com> * Fix the inconsistent varable name * Fix the failed build issue * Remove the logger * Run formatting * Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Co-authored-by: Calum Murray <cmurray@redhat.com> * Remove the logger * Code gen * Update control-plane/pkg/reconciler/channel/channel.go Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Remove the uncessary code * Fix the failing reconciler tests due to the missing newly added filed in the test * Format fix * Instead of using channel service name, we directly use channel name for Path * Instead of using channel service name, we directly use channel name for Path --------- Co-authored-by: Calum Murray <cmurray@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Update the function name --------- Co-authored-by: Rahul kumar <68837569+Rahul-Kumar-prog@users.noreply.github.com> Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> Co-authored-by: Calum Murray <cmurray@redhat.com>

using the composite prober for the channel

0b4f237

knative-prow bot requested review from lionelvillard and pierDipi July 31, 2023 12:02

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2023

knative-prow bot added the area/control-plane label Jul 31, 2023

Cali0707 suggested changes Jul 31, 2023

View reviewed changes

knative-prow bot assigned Cali0707 Jul 31, 2023

Rahul-Kumar-prog added 2 commits August 2, 2023 13:43

edit channel_test.go file

a7a7d6a

change channel_test.go

0b18141

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2023

Cali0707 reviewed Aug 2, 2023

View reviewed changes

done changes in V2 channel

67271f4

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 2, 2023

Cali0707 suggested changes Aug 2, 2023

View reviewed changes

control-plane/pkg/reconciler/channel/v2/controllerv2.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/channel/v2/controllerv2.go Outdated Show resolved Hide resolved

edit in controllerv2.go

a407f84

Cali0707 suggested changes Aug 2, 2023

View reviewed changes

control-plane/pkg/reconciler/channel/channel.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/channel/v2/channelv2.go Outdated Show resolved Hide resolved

Merge branch 'knative-extensions:main' into Rahulkumar3188

94ade90

Cali0707 suggested changes Oct 10, 2023

View reviewed changes

control-plane/pkg/reconciler/channel/controller.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/channel/v2/controllerv2.go Outdated Show resolved Hide resolved

pierDipi and others added 2 commits October 11, 2023 13:19

Update prober pod port

9da5edc

Co-authored-by: Calum Murray <cmurray@redhat.com>

Update prober pod port

a2b1cb5

Co-authored-by: Calum Murray <cmurray@redhat.com>

Cali0707 suggested changes Oct 11, 2023

View reviewed changes

control-plane/pkg/reconciler/channel/controller.go Outdated Show resolved Hide resolved

control-plane/pkg/reconciler/channel/v2/controllerv2.go Outdated Show resolved Hide resolved

pierDipi and others added 3 commits October 12, 2023 16:45

Update prober pod port for TLS

1e9f84f

Co-authored-by: Calum Murray <cmurray@redhat.com>

Update prober pod port for TLS

da70a43

Co-authored-by: Calum Murray <cmurray@redhat.com>

Merge branch 'knative-extensions:main' into Rahulkumar3188

8aa2a7f

Cali0707 approved these changes Oct 12, 2023

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 12, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 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 Oct 13, 2023

knative-prow bot merged commit 13e4936 into knative-extensions:main Oct 13, 2023

Cali0707 mentioned this pull request Oct 13, 2023

Eventing TLS: E2E tests for Channel TLS key pair rotation #3374

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use the composite prober with the channel #3252

use the composite prober with the channel #3252

Rahul-Kumar-prog commented Jul 31, 2023 •

edited

Loading

knative-prow bot commented Jul 31, 2023

codecov bot commented Jul 31, 2023 •

edited

Loading

Cali0707 left a comment •

edited

Loading

Rahul-Kumar-prog commented Jul 31, 2023

Cali0707 commented Jul 31, 2023

Rahul-Kumar-prog commented Jul 31, 2023

Cali0707 commented Jul 31, 2023

Rahul-Kumar-prog commented Aug 2, 2023 •

edited

Loading

Cali0707 commented Aug 2, 2023

Cali0707 left a comment •

edited

Loading

Rahul-Kumar-prog commented Aug 2, 2023

Cali0707 commented Aug 2, 2023

Cali0707 left a comment

Cali0707 commented Aug 2, 2023

Cali0707 left a comment

Cali0707 commented Oct 4, 2023

Rahul-Kumar-prog commented Oct 7, 2023 •

edited

Loading

Cali0707 left a comment

pierDipi commented Oct 11, 2023

Cali0707 commented Oct 11, 2023

Cali0707 left a comment

Cali0707 commented Oct 12, 2023 •

edited

Loading

Rahul-Kumar-prog commented Oct 12, 2023 •

edited

Loading

Cali0707 commented Oct 12, 2023

Cali0707 commented Oct 12, 2023

Cali0707 left a comment

knative-prow bot commented Oct 12, 2023

Cali0707 commented Oct 12, 2023

pierDipi commented Oct 13, 2023

pierDipi commented Oct 13, 2023

use the composite prober with the channel #3252

use the composite prober with the channel #3252

Conversation

Rahul-Kumar-prog commented Jul 31, 2023 • edited Loading

Proposed Changes

knative-prow bot commented Jul 31, 2023

codecov bot commented Jul 31, 2023 • edited Loading

Codecov Report

Cali0707 left a comment • edited Loading

Choose a reason for hiding this comment

Rahul-Kumar-prog commented Jul 31, 2023

Cali0707 commented Jul 31, 2023

Rahul-Kumar-prog commented Jul 31, 2023

Cali0707 commented Jul 31, 2023

Rahul-Kumar-prog commented Aug 2, 2023 • edited Loading

Cali0707 commented Aug 2, 2023

Cali0707 left a comment • edited Loading

Choose a reason for hiding this comment

Rahul-Kumar-prog commented Aug 2, 2023

Cali0707 commented Aug 2, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 commented Aug 2, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 commented Oct 4, 2023

Rahul-Kumar-prog commented Oct 7, 2023 • edited Loading

Cali0707 left a comment

Choose a reason for hiding this comment

pierDipi commented Oct 11, 2023

Cali0707 commented Oct 11, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 commented Oct 12, 2023 • edited Loading

Rahul-Kumar-prog commented Oct 12, 2023 • edited Loading

Cali0707 commented Oct 12, 2023

Cali0707 commented Oct 12, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 12, 2023

Cali0707 commented Oct 12, 2023

pierDipi commented Oct 13, 2023

pierDipi commented Oct 13, 2023

Rahul-Kumar-prog commented Jul 31, 2023 •

edited

Loading

codecov bot commented Jul 31, 2023 •

edited

Loading

Cali0707 left a comment •

edited

Loading

Rahul-Kumar-prog commented Aug 2, 2023 •

edited

Loading

Cali0707 left a comment •

edited

Loading

Rahul-Kumar-prog commented Oct 7, 2023 •

edited

Loading

Cali0707 commented Oct 12, 2023 •

edited

Loading

Rahul-Kumar-prog commented Oct 12, 2023 •

edited

Loading