Reload certificate periodically #280

gthao313 · 2022-10-10T17:56:54Z

when cert-manager rotates the certificate brupop-apiserver-certificate (which by default will be after 60 days), brupop apiserver needs to reload certificate to bounce all the agent and APIServer pods.

Issue number:
Closes: #233

Description of changes:

Author: Tianhao Geng <tianhg@amazon.com>
Date:   Mon Oct 10 17:46:26 2022 +0000

    Reload certificate periodically

    when cert-manager rotates the certificate brupop-apiserver-certificate
    (which by default will be after 60 days), brupop apiserver needs to reload
    certificate to bounce all the agent and APIServer pods.

Add a mechanism to monitor if certificate has been renewed. If yes, stop servers and apiserver will be restarted to reload new certificate. Agent doesn't need do anything since the logic in brupop that agent will always use current certificate on volume to talk with apiserver.

Testing done:

Set Certificate to be expired on 7 mins

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: brupop-apiserver-certificate
  namespace: brupop-bottlerocket-aws
spec:
  isCA: true
  commonName: my-selfsigned-ca
  secretName: brupop-tls
  renewBefore: 2159h53m

Run integration test to monitor if all nodes still are updated to latest version.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

webern · 2022-10-10T18:21:35Z

apiserver/src/api/mod.rs

+                "Certificate has been renewed, restarting server to reload new certificate"
+            );
+            server.stop(true).await;
+            std::process::exit(0)


I don't like to see a process exit buried so deeply in a program. Ideally you would return a bool (or enum) that would say whether or not an exit is needed. Bubble that all the way up to main if possible. Ideally only main should have process exits IMO.

After testing, I think we don't need process exit since server.stop(true).await; will restart apiserver. I'll just remove this std::process::exit(0). thanks

gthao313 · 2022-10-11T17:57:40Z

rebase

jpmcb

Awesome! 🕺🏼

jpmcb · 2022-10-11T20:47:44Z

models/src/node/mod.rs

+pub mod mod_error {
+    use snafu::Snafu;
+
+    #[derive(Debug, Snafu)]
+    #[snafu(visibility(pub))]
+    pub enum Error {
+        #[snafu(display(
+            "IO error occurred while attempting to use APIServerClient: '{}'",
+            source
+        ))]
+        IOError { source: Box<dyn std::error::Error> },
+    }
+}


Would this error be better placed in the node/error.rs file with the rest of this modules errors? (also see comment above where we seem to already have a Result type for this module

jpmcb · 2022-10-11T20:49:01Z

models/src/node/mod.rs

+use std::io::Read;
+
+/// The module-wide result type.
+type Result<T> = std::result::Result<T, mod_error::Error>;


Isn't this already defined here?

bottlerocket-update-operator/models/src/node/error.rs

Line 5 in 72bf1b3

pub type Result<T> = std::result::Result<T, Error>;

jpmcb · 2022-10-11T20:51:26Z

apiserver/src/api/mod.rs

@@ -101,6 +104,9 @@ pub async fn run_server<T: 'static + BottlerocketShadowClient>(
    k8s_client: kube::Client,
    prometheus_exporter: Option<opentelemetry_prometheus::PrometheusExporter>,
 ) -> Result<()> {
+    let public_key_path = format!("{}/{}", TLS_KEY_MOUNT_PATH, PUBLIC_KEY_NAME);
+    let certificate_cache = read_certificate(&public_key_path).unwrap();


Let's replace this unwrap with a structured error in the API module (reference: bottlerocket-os/bottlerocket@5a58d4b)

jpmcb · 2022-10-11T20:55:42Z

apiserver/src/api/mod.rs

+// Certificate is refreshed periodically (default 60 days). Once certificate is renewed, apiserver
+// needs to stop the server for reloading new certificate.
+// We cache the certificate initially when brupop starts server, and compare to update-to-date certificate periodically.
+// If they aren't match, we recognize it as new certificate is coming out, so the server needs to be restarted.


A few nits here:

Suggested change

// Certificate is refreshed periodically (default 60 days). Once certificate is renewed, apiserver

// needs to stop the server for reloading new certificate.

// We cache the certificate initially when brupop starts server, and compare to update-to-date certificate periodically.

// If they aren't match, we recognize it as new certificate is coming out, so the server needs to be restarted.

// The certificate is refreshed periodically (default 60 days). Once the certificate is renewed, the apiserver

// needs to stop in order to reload the new certificate.

// We cache the certificate initially when brupop starts the server, and compare it to the update-to-date certificate periodically.

// If they don't match, we recognize it as a new certificate, so the server needs to be restarted.

gthao313 · 2022-10-11T21:29:20Z

push above address @jpmcb comments

when cert-manager rotates the certificate brupop-apiserver-certificate (which by default will be after 60 days), brupop apiserver needs to reload certificate to bounce all the agent and APIServer pods.

jpmcb

🥇

cbgbt · 2022-11-16T22:47:51Z

apiserver/src/api/mod.rs

+// We cache the certificate initially when brupop starts the server, and compare it to the update-to-date certificate periodically.
+// If they don't match, we recognize it as a new certificate, so the server needs to be restarted.
+async fn reload_certificate(
+    server: Server,


Sorry to comment on an old PR, but I was catching up on some of the stuff that's changed in brupop!

I wonder if it makes more sense to make this a mutable reference? It seems unusual to clone it. Although, I suppose if it works, Actix must have done some work to make sure that the interior state of the Server is thread-safe.

I actually find out that using https://github.com/notify-rs/notify to watch the fs change and reload the certificate is what other people like to do.

@cbgbt - This has actually changed recently: 561e6c0#diff-5cad36d03cb8802a9c5a0a20ae2e841d683e1cd564146c441e2940fb1ecf652eR255

A few APIs changed in Actix so cloning the server and using it directly wasn't a great option. Instead, now, we can use a actix_web::dev::ServerHandle as a "server handler" to call into it and stop it like:

server_handler.stop(true).await;

gthao313 force-pushed the reload-cert branch from 28fb1c7 to 381630b Compare October 10, 2022 18:16

webern approved these changes Oct 10, 2022

View reviewed changes

gthao313 force-pushed the reload-cert branch from 381630b to e52589a Compare October 10, 2022 18:45

gthao313 requested a review from jpmcb October 10, 2022 18:45

gthao313 force-pushed the reload-cert branch from e52589a to f6fb9a6 Compare October 11, 2022 17:57

jpmcb reviewed Oct 11, 2022

View reviewed changes

gthao313 force-pushed the reload-cert branch from f6fb9a6 to 806c2c1 Compare October 11, 2022 21:28

Reload certificate periodically

5e6251f

when cert-manager rotates the certificate brupop-apiserver-certificate (which by default will be after 60 days), brupop apiserver needs to reload certificate to bounce all the agent and APIServer pods.

gthao313 force-pushed the reload-cert branch from 806c2c1 to 5e6251f Compare October 11, 2022 21:44

gthao313 requested a review from jpmcb October 11, 2022 21:45

jpmcb approved these changes Oct 11, 2022

View reviewed changes

gthao313 merged commit 0ec8a9a into bottlerocket-os:develop Oct 11, 2022

jpmcb mentioned this pull request Nov 8, 2022

TLS: Utilize leaf certs from root CA for brupop API server and agent #340

Merged

cbgbt reviewed Nov 16, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reload certificate periodically #280

Reload certificate periodically #280

gthao313 commented Oct 10, 2022

webern Oct 10, 2022

gthao313 Oct 10, 2022

gthao313 commented Oct 11, 2022

jpmcb left a comment

jpmcb Oct 11, 2022

jpmcb Oct 11, 2022

jpmcb Oct 11, 2022

jpmcb Oct 11, 2022

gthao313 commented Oct 11, 2022

jpmcb left a comment

cbgbt Nov 16, 2022

somnusfish Nov 16, 2022

jpmcb Nov 21, 2022

Reload certificate periodically #280

Reload certificate periodically #280

Conversation

gthao313 commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gthao313 commented Oct 11, 2022

jpmcb 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

Choose a reason for hiding this comment

gthao313 commented Oct 11, 2022

jpmcb 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