Enable auto ssl keystore reload #156

yanglei99 · 2019-11-05T05:18:25Z

https://confluentinc.atlassian.net/browse/MCM-462

Here is the one pager: https://confluentinc.atlassian.net/wiki/spaces/MCM/pages/938970260/One+Pager+Auto+Reload+SSL+Certificate+for+Http+Endpoints#OnePager:AutoReloadSSLCertificateforHttpEndpoints-CPComponentsusingrest-util

Here is the related logic in kafka ( using WatchService)

https://github.com/confluentinc/ce-kafka/blob/cc608b2d0856e9eee0b0f0faef280a3572f8e20d/ce-broker-plugins/src/main/java/io/confluent/kafka/multitenant/PhysicalClusterMetadata.java#L508

Here is the end-end test for kafka-api:

https://github.com/confluentinc/cc-spec-kafka/pull/54

ghost · 2019-11-05T05:18:27Z

It looks like @yanglei99 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

norwood · 2019-11-05T06:45:31Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+          log.info("WatchKey not recognized");
+          continue;
+        }
+        log.info("Watch Key notified");


may want many of these .info to be .debug

Changed most of the info to debug. But want to keep this notified...

norwood · 2019-11-05T06:46:29Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+              break;
+            }
+          } catch (java.io.IOException e) {
+            log.info("Hit error process the change event", e);


and this should probably be warn instead of info.

norwood · 2019-11-05T06:47:00Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+              try {
+                callback.run();
+              } catch (Exception e) {
+                log.error("Error while reloading cert", e);


feels weird for this to specifically mention cert reloading, but the class is written to be generic

norwood · 2019-11-05T06:47:49Z

core/src/main/java/io/confluent/rest/RestConfig.java

+  public static final String SSL_KEYSTORE_RELOAD_CONFIG = "ssl.keystore.reload";
+  protected static final String SSL_KEYSTORE_RELOAD_DOC =
+      "Enable auto reload of ssl keystore";
+  protected static final boolean SSL_KEYSTORE_RELOAD_DEFAULT = false;


this seems like a great addition. why do you want this default false?

Backward compatible? As current behavior is not auto-reload... I do not know if there are cases (like on prem), we do not want to auto-reload... I can change it to true if we do not see potential issues

aren't your changes backwards compatible?

i mean, i guess not if people are relying on the ability to change the keystore file and have nothing happen. but that seems like a crazy thing to do in the first place.

revised, thanks!

norwood · 2019-11-05T06:58:05Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+        log.info("Watch Key notified");
+        for (WatchEvent<?> event : key.pollEvents()) {
+          WatchEvent.Kind<?> kind = event.kind();
+          if (kind != StandardWatchEventKinds.ENTRY_MODIFY) {


would this miss a delete followed by a create?

since we already know this.file, is there anything to be gained by doing all these checks anyhow? can we not just jump straight to callback.run() and lfg?

As we only listen to MODIFY, the only thing I need to check is OVERFLOW – Indicates that events might have been lost or discarded. You do not have to register for the OVERFLOW event to receive it.. I can make it specific.

norwood · 2019-11-05T06:59:17Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+        }
+        // reset key
+        if (!key.reset()) {
+          break;


this seems to be killing the watcher, so probably a good place for a .info("Ending my watch") too.

xvrl · 2019-11-05T21:14:19Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+import java.util.concurrent.Future;
+
+// reference https://gist.github.com/danielflower/f54c2fe42d32356301c68860a4ab21ed
+public class FileWatcher implements Runnable {


what do we use in ce-kafka to auto-reload ssl certs, can we reuse some of the code there?

I checked with @aashishkohli , she mentioned Kafka is using library from open source kafka repo... I will let her add more input.

xvrl · 2019-11-05T21:16:04Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+          break;
+        }
+      }
+    } catch (InterruptedException e) {


what if any other kind of exception happens, wouldn't that kill the watcher thread and prevent future reloads?

We caught other exceptions in the loop so that we can carry on with the watch. InterruptedException is the one we do want to catch for thread interruption on watchService.take() so we can stop the work

we should still have a catch for any unexpected runtime exceptions, otherwise the thread will stop

xvrl · 2019-11-05T21:17:10Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+  }
+
+  /**
+    * Starts watching a file and the given path and calls the callback when it is changed.


what do we mean by "file and the given path", do we mean a file or a directory?

revised the comment

xvrl · 2019-11-05T21:24:42Z

core/src/main/java/io/confluent/rest/Application.java

+          FileWatcher.onFileChange(keystorePath, () -> 
+              sslContextFactory.reload(scf -> log.info("Reloaded SSL cert")));
+          log.info("Enabled SSL cert auto reload: " + keystorePath);
+        } catch (java.io.IOException e) {


do we know when this might fail?

The IOException can be thrown for both creating a watch service and register

https://docs.oracle.com/javase/7/docs/api/java/nio/file/FileSystem.html#newWatchService()

https://docs.oracle.com/javase/7/docs/api/java/nio/file/Path.html#register(java.nio.file.WatchService,%20java.nio.file.WatchEvent.Kind...)

yanglei99 · 2019-11-06T03:57:30Z

[clabot:check]

ghost · 2019-11-06T03:57:31Z

@confluentinc It looks like @yanglei99 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

norwood

lfg if xavier gives it the old 👍

xvrl · 2019-11-14T21:50:28Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+            continue;
+          }
+          WatchEvent<Path> ev = (WatchEvent<Path>)event;
+          Path changed = this.file.getParent().resolve(ev.context());


nit, we don't seem to be consistent about file vs. this.file. Typically the distinction is only needed if there is ambiguity between a local variable and a member variable

xvrl · 2019-11-14T22:03:31Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+  public static void onFileChange(Path file, Callback callback) throws IOException {
+    log.info("Configure watch file change: " + file);
+    FileWatcher fileWatcher = new FileWatcher(file, callback);
+    ExecutorService executor = Executors.newSingleThreadExecutor();


let's make sure this is a daemon thread, so it doesn't prevent the service from shutting down. Then you can also remove the shutdownhook

xvrl · 2019-11-14T22:03:56Z

core/src/main/java/io/confluent/rest/FileWatcher.java

+    FileWatcher fileWatcher = new FileWatcher(file, callback);
+    ExecutorService executor = Executors.newSingleThreadExecutor();
+    Future<?> future = executor.submit(fileWatcher);
+    Runtime.getRuntime().addShutdownHook(new Thread(executor::shutdownNow));


shutdown hook is not necessary if you make this a daemon thread.

FileWatcher is a Runnable... it is using newSingleThreadExecutor to run it... which uses a worker thread... https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html#newSingleThreadExecutor()
What benefit we get out of a deamon thread?

xvrl

two things: let's make sure the background thread is a daemon thread, and make sure we catch unchecked exceptions in the polling loop. We can merge once that's addressed.

yanglei99 requested review from xvrl and norwood November 5, 2019 05:18

norwood reviewed Nov 5, 2019

View reviewed changes

xvrl reviewed Nov 5, 2019

View reviewed changes

norwood approved these changes Nov 6, 2019

View reviewed changes

yanglei99 requested review from agg111 and alexlod November 6, 2019 04:03

alexlod approved these changes Nov 6, 2019

View reviewed changes

Yang Lei added 11 commits November 7, 2019 13:20

rebase

8bcee06

revise auto reload

03e4974

test case

d497b1c

revise parameter

66cc34f

revise logging

9866b0a

shutdown logic

8439046

take care of delete then create

1a237ab

revise test

b974c27

revise doc

a166704

revise port

8665ed4

fix issue

169115a

yanglei99 force-pushed the master branch from e2d28cf to 169115a Compare November 7, 2019 18:36

Yang Lei added 3 commits November 7, 2019 13:39

revise test

5d6380c

revise style

3d43ea7

handle k8s secret symbolic link

d678186

xvrl reviewed Nov 14, 2019

View reviewed changes

xvrl approved these changes Nov 14, 2019

View reviewed changes

Yang Lei added 3 commits November 14, 2019 22:41

revise thread and exception handling

f33ec0c

revise log

47ca26b

revise log

76f4f85

yanglei99 merged commit 7dd182c into confluentinc:master Nov 15, 2019

vcrfxia mentioned this pull request May 31, 2020

feat: reload TLS certificate without restarting server confluentinc/ksql#5516

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable auto ssl keystore reload #156

Enable auto ssl keystore reload #156

yanglei99 commented Nov 5, 2019 •

edited

Loading

ghost commented Nov 5, 2019

norwood Nov 5, 2019

yanglei99 Nov 6, 2019

norwood Nov 5, 2019

norwood Nov 5, 2019

norwood Nov 5, 2019

yanglei99 Nov 5, 2019

norwood Nov 6, 2019

yanglei99 Nov 6, 2019

norwood Nov 5, 2019

yanglei99 Nov 5, 2019

norwood Nov 5, 2019

yanglei99 Nov 6, 2019

xvrl Nov 5, 2019

yanglei99 Nov 6, 2019

xvrl Nov 5, 2019

yanglei99 Nov 6, 2019 •

edited

Loading

xvrl Nov 14, 2019

xvrl Nov 5, 2019

yanglei99 Nov 6, 2019

xvrl Nov 5, 2019

yanglei99 Nov 6, 2019 •

edited

Loading

yanglei99 commented Nov 6, 2019

ghost commented Nov 6, 2019

norwood left a comment

xvrl Nov 14, 2019

xvrl Nov 14, 2019

xvrl Nov 14, 2019

yanglei99 Nov 14, 2019 •

edited

Loading

xvrl left a comment

Enable auto ssl keystore reload #156

Enable auto ssl keystore reload #156

Conversation

yanglei99 commented Nov 5, 2019 • edited Loading

ghost commented Nov 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanglei99 Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanglei99 Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

yanglei99 commented Nov 6, 2019

ghost commented Nov 6, 2019

norwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanglei99 Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

xvrl left a comment

Choose a reason for hiding this comment

yanglei99 commented Nov 5, 2019 •

edited

Loading

yanglei99 Nov 6, 2019 •

edited

Loading

yanglei99 Nov 6, 2019 •

edited

Loading

yanglei99 Nov 14, 2019 •

edited

Loading