Keypair rotation #3208

Leo6Leo · 2023-07-11T20:34:18Z

Fixes #3164

Proposed Changes

Set up the watcher that listen to the changes in the mounted secret volume are detected by the watcher
Use updateSSLOptions to update the server SSL options when the changes in the mounted secret volume are detected by the watcher

Task List

Bump up Quarkus to 3.1.2.Final PR: Bump Quarkus to 3.1.2 Final #3183
Bump up Vert.x to V4.4.2 PR: Bump Vertx to 4.4.3 #3201
Support Rotating key pair PR: Keypair rotation #3208

Additional Note

Pier suggested to bump up to 3.2.0.Final, the issue Bump quarkus from 3.1.2.Final to 3.2.0.Final #3199

Release Note

SSL options change when the mounted secret file changed

Docs

knative-prow · 2023-07-11T20:34:20Z

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

Leo6Leo · 2023-07-11T20:39:59Z

Today I was trying to directly modify the .key file by entering the bash of the pod kubectl exec -it kafka-sink-receiver-698ff747b4-75sg6 --namespace=knative-eventing -- /bin/sh, but it doesn't work with a lot of different attempts. :((

@Cali0707 suggested me to write some unit tests, so I will write some unit test to make sure my function works as expected tomorrow! :))

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

pierDipi · 2023-07-12T08:52:39Z

Today I was trying to directly modify the .key file by entering the bash of the pod kubectl exec -it kafka-sink-receiver-698ff747b4-75sg6 --namespace=knative-eventing -- /bin/sh, but it doesn't work with a lot of different attempts. :((

Yes, our container filesystems are usually read-only, so you can't do that, see
https://cert-manager.io/docs/usage/certificate/, section: "Actions that will trigger a rotation of the private key"

@Cali0707 suggested me to write some unit tests, so I will write some unit test to make sure my function works as expected tomorrow! :))

codecov · 2023-07-12T15:36:55Z

Codecov Report

Merging #3208 (7039365) into main (05694b6) will decrease coverage by 16.19%.
The diff coverage is 75.80%.

@@              Coverage Diff              @@
##               main    #3208       +/-   ##
=============================================
- Coverage     79.84%   63.65%   -16.19%     
- Complexity      763      774       +11     
=============================================
  Files            78      170       +92     
  Lines          2744    11915     +9171     
  Branches        246      250        +4     
=============================================
+ Hits           2191     7585     +5394     
- Misses          401     3754     +3353     
- Partials        152      576      +424

Flag	Coverage Δ
java-unittests	`80.14% <75.80%> (+0.29%)`	⬆️

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

Impacted Files	Coverage Δ
...eventing/kafka/broker/core/file/SecretWatcher.java	`71.42% <71.42%> (ø)`
...g/kafka/broker/receiver/impl/ReceiverVerticle.java	`87.62% <78.78%> (+19.05%)`	⬆️
.../broker/receiver/main/ReceiverVerticleFactory.java	`78.57% <100.00%> (+1.64%)`	⬆️

... and 92 files with indirect coverage changes

Leo6Leo · 2023-07-12T22:02:04Z

@pierDipi @creydr I have switched to filesystem watcher, and by writing a small test, I can see that when the file changed, the update function will be called. I am wondering that am I on the right track?

OpenJDK 64-Bit Server VM warning: Sharing is only supported for boot loader classes because bootstrap classpath has been appended
18:02:26.782 [vert.x-eventloop-thread-1] INFO  dev.knative.eventing.kafka.broker.receiver.impl.ReceiverVerticle - Starting SecretWatcher
18:02:26.791 [Thread-2] INFO  dev.knative.eventing.kafka.broker.core.file.SecretWatcher - Detected change to secret tls.crt

pierDipi

Thanks @Leo6Leo !

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

data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/SecretWatcher.java

…r/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Leo6Leo · 2023-07-19T15:36:48Z

Should I try to implement this approach I mentioned here? #3208 (comment) @pierDipi

pierDipi · 2023-07-19T15:42:03Z

Should I try to implement this approach I mentioned here? #3208 (comment) @pierDipi

I'd say not required, more like a future enhancement?

Leo6Leo · 2023-07-19T16:30:22Z

/retest

Leo6Leo · 2023-07-19T17:42:10Z

/retest

Leo6Leo · 2023-07-19T18:26:10Z

/retest

Leo6Leo · 2023-07-19T19:11:01Z

/retest-required

pierDipi

Thank you!

/lgtm
/approve

knative-prow · 2023-07-20T07:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

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

pierDipi · 2023-07-20T09:16:03Z

/test channel-integration-tests-sasl-ssl

pierDipi · 2023-07-20T10:03:53Z

/test channel-integration-tests-sasl-ssl

* Bump Quarkus to 3.1.2 Final (test failing) * update the dependencies, and overwrite the antlr version * Code-gen * codegen * Bump up vertx and coomplete codegen * Trying to implement the keypair rotation * Finish implementing the watcher * codegen * Write the filesystem watcher * Execute the sslOptions update if the changes are detected * Close the watcher if the vertx server is shut down * Update data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Separate the testing env and the production env * Change the key and crt file names to use variable * Change variable to private * Updathe assert rules * Format change * Add the condition check for secretWatcher * Update data-plane/receiver/src/main/java/dev/knative/eventing/kafka/broker/receiver/impl/ReceiverVerticle.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Changed to use onSuccess and onFailure * Move the injection to the constructor * Update the ReceiverVerticleFactory * reformat * Update data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Update data-plane/core/src/main/java/dev/knative/eventing/kafka/broker/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com> * Fixed all the comments in the second round of review * Fixed all the comments in the second round of review * Format fix * Format fix * codegen --------- Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Leo6Leo added 9 commits June 28, 2023 15:20

Bump Quarkus to 3.1.2 Final (test failing)

258c4a3

update the dependencies, and overwrite the antlr version

a7a4efb

Merge branch 'main' into quarkus_3.1.2.Final

2bb5678

Code-gen

fecaa4c

Merge

a09f5d2

codegen

2809e7e

Bump up vertx and coomplete codegen

e53ec29

Trying to implement the keypair rotation

4a451b4

Finish implementing the watcher

97ac6cc

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 Jul 11, 2023

knative-prow bot requested review from aliok and odacremolbap July 11, 2023 20:34

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

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/control-plane area/data-plane labels Jul 11, 2023

pierDipi reviewed Jul 12, 2023

View reviewed changes

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

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

Merge the upstream branch and resolve the merge conflict

9b50bed

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

codegen

1212b5b

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

Write the filesystem watcher

f5b2cdf

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 Jul 12, 2023

Leo6Leo added 2 commits July 19, 2023 10:36

Update the ReceiverVerticleFactory

bc6aa7d

reformat

732e10c

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

merge the upstream main branch

416d501

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

Leo6Leo requested a review from pierDipi July 19, 2023 15:01

pierDipi reviewed Jul 19, 2023

View reviewed changes

Leo6Leo and others added 6 commits July 19, 2023 11:26

Update data-plane/core/src/main/java/dev/knative/eventing/kafka/broke…

07d7e03

…r/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Update data-plane/core/src/main/java/dev/knative/eventing/kafka/broke…

66e6b83

…r/core/file/SecretWatcher.java Co-authored-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>

Fixed all the comments in the second round of review

3d972f7

Fixed all the comments in the second round of review

a3ad5a2

Format fix

eeed8e2

Format fix

c1dbead

Leo6Leo requested a review from pierDipi July 19, 2023 15:36

codegen

7039365

pierDipi approved these changes Jul 20, 2023

View reviewed changes

knative-prow bot assigned pierDipi Jul 20, 2023

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

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

knative-prow bot merged commit b30da88 into knative-extensions:main Jul 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keypair rotation #3208

Keypair rotation #3208

Leo6Leo commented Jul 11, 2023 •

edited

Loading

knative-prow bot commented Jul 11, 2023

Leo6Leo commented Jul 11, 2023

pierDipi commented Jul 12, 2023

codecov bot commented Jul 12, 2023 •

edited

Loading

Leo6Leo commented Jul 12, 2023 •

edited

Loading

pierDipi left a comment

Leo6Leo commented Jul 19, 2023

pierDipi commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

pierDipi left a comment

knative-prow bot commented Jul 20, 2023

pierDipi commented Jul 20, 2023

pierDipi commented Jul 20, 2023

Keypair rotation #3208

Keypair rotation #3208

Conversation

Leo6Leo commented Jul 11, 2023 • edited Loading

Proposed Changes

Task List

Additional Note

knative-prow bot commented Jul 11, 2023

Leo6Leo commented Jul 11, 2023

pierDipi commented Jul 12, 2023

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

Leo6Leo commented Jul 12, 2023 • edited Loading

pierDipi left a comment

Choose a reason for hiding this comment

Leo6Leo commented Jul 19, 2023

pierDipi commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

Leo6Leo commented Jul 19, 2023

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jul 20, 2023

pierDipi commented Jul 20, 2023

pierDipi commented Jul 20, 2023

Leo6Leo commented Jul 11, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

Leo6Leo commented Jul 12, 2023 •

edited

Loading