Update the updateServerConfig function to pass in the cert value diectly #3337

Leo6Leo · 2023-09-13T18:53:47Z

Proposed Changes

Now:

PemKeyCertOptions keyCertOptions = new PemKeyCertOptions()
            .setCertValue(Buffer.buffer(java.nio.file.Files.readString(tlsCrtFile.toPath())))
            .setKeyValue(Buffer.buffer(java.nio.file.Files.readString(tlsKeyFile.toPath())));

Past:

PemKeyCertOptions keyCertOptions =
                        new PemKeyCertOptions().setKeyPath(tlsKeyFile.getPath()).setCertPath(tlsCrtFile.getPath());

Release Note

Docs

…ctly

knative-prow · 2023-09-13T18:53:50Z

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-09-13T18:59:48Z

Codecov Report

Merging #3337 (e7b6346) into main (7e86a37) will increase coverage by 3.14%.
Report is 6 commits behind head on main.
The diff coverage is 72.72%.

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

@@             Coverage Diff              @@
##               main    #3337      +/-   ##
============================================
+ Coverage     58.49%   61.63%   +3.14%     
- Complexity        0      768     +768     
============================================
  Files            91      182      +91     
  Lines          9233    12333    +3100     
  Branches          0      268     +268     
============================================
+ Hits           5401     7602    +2201     
- Misses         3403     4135     +732     
- Partials        429      596     +167

Flag	Coverage Δ
java-unittests	`70.90% <72.72%> (?)`

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

Files Changed	Coverage Δ
...g/kafka/broker/receiver/impl/ReceiverVerticle.java	`78.78% <72.72%> (ø)`

... and 92 files with indirect coverage changes

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

Cali0707 · 2023-09-14T15:50:40Z

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

-                    });
+            try {
+                // Update SSL configuration by passing the new value of the certificate and key
+                // Have to use value instead of path here otherwise the changes won't be applied


I'm curious, do you know why this is the case @Leo6Leo?

I am not sure whether problem is here or not.
https://github.com/eclipse-vertx/vert.x/blob/6954961d9b723ed245f91b6dedb41b9b09e991bd/src/main/java/io/vertx/core/net/impl/SSLHelper.java#L182C11-L182C71

Because the line if (prev.succeeded() && prev.result().options.equals(options)) { is checking whether the options are the same. If we are using the path, obviously the options doesn't get changed because the file path is the same. And so it doesn't get updated?

That's my guess. Wdyt @Cali0707 @pierDipi

That seems accurate, can you report that to the Vertx team https://github.com/eclipse-vertx/vert.x/issues/new/choose?

eclipse-vertx/vert.x#4867

Issue reported @pierDipi

pierDipi

Nice findings @Leo6Leo

/lgtm
/approve

Left one nit comment to be consistent with the style of the other code which often use "final var"

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

pierDipi · 2023-09-20T09:06:45Z

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java

-                    });
+            try {
+                // Update SSL configuration by passing the new value of the certificate and key
+                // Have to use value instead of path here otherwise the changes won't be applied


That seems accurate, can you report that to the Vertx team https://github.com/eclipse-vertx/vert.x/issues/new/choose?

pierDipi · 2023-09-20T09:10:11Z

/retest-required

…roker/receiver/impl/ReceiverVerticle.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Leo6Leo · 2023-09-20T17:30:45Z

/retest-required

knative-prow · 2023-09-20T17:54:03Z

@Leo6Leo: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
reconciler-tests-keda_eventing-kafka-broker_main	`ea4028e`	link	false	`/test reconciler-tests-keda`
reconciler-tests-loom_eventing-kafka-broker_main	`ea4028e`	link	false	`/test reconciler-tests-loom`
reconciler-tests-namespaced-broker-loom_eventing-kafka-broker_main	`ea4028e`	link	false	`/test reconciler-tests-namespaced-broker-loom`

Your PR dashboard.

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. I understand the commands that are listed here.

Leo6Leo · 2023-09-21T19:35:28Z

/retest-required
/cc @Cali0707

Cali0707

/lgtm

knative-prow · 2023-09-21T19:49:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, Leo6Leo, pierDipi

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

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

Leo6Leo · 2023-09-22T14:56:41Z

/cherry-pick release-1.11

knative-prow-robot · 2023-09-22T14:57:24Z

@Leo6Leo: new pull request created: #3358

In response to this:

/cherry-pick release-1.11

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.

Update the updateServerConfig function to pass in the cert value dire…

fe09b97

…ctly

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 Sep 13, 2023

knative-prow bot requested review from odacremolbap and pierDipi September 13, 2023 18:53

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/data-plane labels Sep 13, 2023

Update the implementation of receiverVerticle

092b83e

Leo6Leo marked this pull request as ready for review September 13, 2023 22:37

knative-prow bot requested review from Cali0707 and lionelvillard September 13, 2023 22:38

Leo6Leo changed the title ~~[WIP] Update the updateServerConfig function to pass in the cert value diectly~~ Update the updateServerConfig function to pass in the cert value diectly Sep 13, 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 Sep 13, 2023

pierDipi reviewed Sep 14, 2023

View reviewed changes

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Outdated Show resolved Hide resolved

pierDipi reviewed Sep 14, 2023

View reviewed changes

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Outdated Show resolved Hide resolved

pierDipi reviewed Sep 14, 2023

View reviewed changes

...receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Outdated Show resolved Hide resolved

Fix the comments

e7b6346

Leo6Leo requested a review from pierDipi September 14, 2023 15:32

Leo6Leo mentioned this pull request Sep 14, 2023

E2E tests for broker: TLS key pair rotation #3287

Merged

10 tasks

Cali0707 reviewed Sep 14, 2023

View reviewed changes

pierDipi approved these changes Sep 20, 2023

View reviewed changes

knative-prow bot assigned pierDipi Sep 20, 2023

knative-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 20, 2023

Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/b…

ea4028e

…roker/receiver/impl/ReceiverVerticle.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2023

Leo6Leo mentioned this pull request Sep 20, 2023

updateSSLOptions doesn't update when the updated secret remain at the same path eclipse-vertx/vert.x#4867

Closed

knative-prow bot requested a review from Cali0707 September 21, 2023 19:35

Cali0707 approved these changes Sep 21, 2023

View reviewed changes

knative-prow bot assigned Cali0707 Sep 21, 2023

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

knative-prow bot merged commit 0502268 into knative-extensions:main Sep 21, 2023

knative-prow-robot mentioned this pull request Sep 22, 2023

[release-1.11] Update the updateServerConfig function to pass in the cert value diectly #3358

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the updateServerConfig function to pass in the cert value diectly #3337

Update the updateServerConfig function to pass in the cert value diectly #3337

Leo6Leo commented Sep 13, 2023

knative-prow bot commented Sep 13, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

Cali0707 Sep 14, 2023

Leo6Leo Sep 14, 2023

pierDipi Sep 20, 2023

Leo6Leo Sep 20, 2023

pierDipi left a comment

pierDipi Sep 20, 2023

pierDipi commented Sep 20, 2023

Leo6Leo commented Sep 20, 2023

knative-prow bot commented Sep 20, 2023 •

edited

Loading

Leo6Leo commented Sep 21, 2023

Cali0707 left a comment

knative-prow bot commented Sep 21, 2023

Leo6Leo commented Sep 22, 2023

knative-prow-robot commented Sep 22, 2023

Update the updateServerConfig function to pass in the cert value diectly #3337

Update the updateServerConfig function to pass in the cert value diectly #3337

Conversation

Leo6Leo commented Sep 13, 2023

Proposed Changes

knative-prow bot commented Sep 13, 2023

codecov bot commented Sep 13, 2023 • edited Loading

Codecov Report

Cali0707 Sep 14, 2023

Choose a reason for hiding this comment

Leo6Leo Sep 14, 2023

Choose a reason for hiding this comment

pierDipi Sep 20, 2023

Choose a reason for hiding this comment

Leo6Leo Sep 20, 2023

Choose a reason for hiding this comment

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi Sep 20, 2023

Choose a reason for hiding this comment

pierDipi commented Sep 20, 2023

Leo6Leo commented Sep 20, 2023

knative-prow bot commented Sep 20, 2023 • edited Loading

Leo6Leo commented Sep 21, 2023

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Sep 21, 2023

Leo6Leo commented Sep 22, 2023

knative-prow-robot commented Sep 22, 2023

codecov bot commented Sep 13, 2023 •

edited

Loading

knative-prow bot commented Sep 20, 2023 •

edited

Loading