Moving leader election inside vault-k8s #271

tvoran · 2021-07-09T01:16:58Z

This uses operator-sdk's Become() function to determine leadership for certificate generation, instead of relying on a leader-elector container as in #198.

The associated helm chart changes are in hashicorp/vault-helm#568

Depends on #273

The v1 and v1beta1 AdmissionReview, AdmissionResponse, and AdmissionRequest are the same, so this bumps those to v1. Checks at the start of certWatcher() if AdmissionregistrationV1 exists, and falls back to v1beta1 if it doesn't.

And bail out if Become() fails, to prevent split-brain.

These are in a loop that retries once a second, and the TLS error is normal before the leader generates the cert Secret.

it needs to be more robust than a few retries

The v1 and v1beta1 AdmissionReview, AdmissionResponse, and AdmissionRequest are the same, so this bumps those to v1. Checks at the start of certWatcher() if AdmissionregistrationV1 exists, and falls back to v1beta1 if it doesn't.

and declare payload as a []byte

…eader

tomhjp

Generally LGTM, though one question on resilience to errors.

tomhjp · 2021-07-27T12:33:36Z

deploy/injector-rbac.yaml

+    - "get"
+    - "patch"


I haven't read the whole change set yet, but what uses these permissions?

It's part of the Become() call; it looks like there's some checking and cleanup around situations where an old leader pod was evicted or the underlying node is NotReady: https://github.com/operator-framework/operator-lib/blob/v0.4.1/leader/leader.go#L166-L189

tomhjp · 2021-07-27T14:30:07Z

leader/leader.go

+		err := operator_leader.Become(ctx, "vault-k8s-leader")
+		if err != nil {
+			logger.Error("trouble becoming leader:", "error", err)
+			return


Given that we don't restart the Become attempt, could we end up in a situation where none of the replicas are trying to become the leader, if they all error here? Seems like maybe we should have this error feed back into some sort of retry logic to guard against that.

I agree with @tomhjp , it would a good idea to handle the error from Become() some how. Perhaps the injector should exit right away in this case, and let the scheduler handle the restart?

Good point, I added an infinite retry when there's an error in 75258f7.

@benashz Hmm, I added retries on error in 75258f7, I wonder if you're looking at an old diff?

Exiting right away is an interesting idea too.

The more I think about this, the more I think it would be better for this to retry a few times, then if Become() is still failing, throw a signal or something that would do a system exit non-zero. Otherwise folks would need to be watching the logs to know there was a problem with the replica.

@benashz @tomhjp added retries then exit non-zero in b73527f.

I like that line of thinking 👍

…eader

…-leader

calvn · 2021-07-27T21:30:01Z

leader/leader.go

+		// become the leader when the current leader replica stops running, and
+		// the Kubernetes garbage collector deletes the vault-k8s-leader
+		// ConfigMap.
+		err := operator_leader.Become(ctx, "vault-k8s-leader")


If a node calls this with a leader already elected, does this return an error, nil, or blocks until context cancelation?

It looks like it blocks forever until it can achieve leadership. The context passed in is used for k8s API calls, but I don't see it being checked for cancellation in Become(): https://github.com/operator-framework/operator-lib/blob/v0.4.1/leader/leader.go#L88

calvn · 2021-07-27T21:54:37Z

deploy/injector-deployment.yaml

@@ -95,7 +67,7 @@ spec:
              port: 8080
              scheme: HTTPS
            failureThreshold: 2
-            initialDelaySeconds: 1


Does this change on initial delay for readiness and liveness mean that leader election is a bit longer to establish through this method, or is it to make it less flaky in general?

Yeah, I think it was just taking a little longer than a second while I was testing it locally to achieve leadership and generate the certs so that the liveness probe would pass.

…-leader

benashz

LGTM!

leader/leader.go

benashz · 2021-07-27T18:57:37Z

leader/leader.go

+		err := operator_leader.Become(ctx, "vault-k8s-leader")
+		if err != nil {
+			logger.Error("trouble becoming leader:", "error", err)
+			return


I agree with @tomhjp , it would a good idea to handle the error from Become() some how. Perhaps the injector should exit right away in this case, and let the scheduler handle the restart?

pod delete rbac and no more endpoints

…-leader

Co-authored-by: Ben Ash <[email protected]>

Retries Become() 10 times (with exp backoff) and then signals the caller to exit if it failed. command.Run() now watches an exitOnError channel for that case.

tomhjp

LGTM - I think the failure mode flow looks good, just one suggestion on the logging.

leader/leader.go

tomhjp · 2021-08-31T09:02:29Z

subcommand/injector/command.go

@@ -218,11 +221,12 @@ func (c *Command) Run(args []string) int {
 	}()
 	go func() {
 		select {
+		case <-exitOnError:
+			logger.Error("shutting down due to errors")


As per the comment above, I think this log message should include the error that triggered the shutdown.

It looks like on exitOnError does not result in a non-zero status being returned to the caller. I presumed that it would make its way to os.Exit().

It looks like on exitOnError does not result in a non-zero status being returned to the caller. I presumed that it would make its way to os.Exit().

The flow here is:

c.shutdownHandler() calls server.Shutdown()

server.Shutdown() causes server.ListenAndServeTLS() to return an error, triggering a return 1 for this function:

vault-k8s/subcommand/injector/command.go

Lines 237 to 240 in b73527f

if err := server.ListenAndServeTLS(c.flagCertFile, c.flagKeyFile); err != nil {

c.UI.Error(fmt.Sprintf("Error listening: %s", err))

return 1

}

And that makes its way back to os.Exit in main.go:

vault-k8s/main.go

Lines 17 to 21 in b73527f

exitStatus, err := c.Run()

if err != nil {

log.Println(err)

}

os.Exit(exitStatus)

As per the comment above, I think this log message should include the error that triggered the shutdown.

Added the error to the log message in a45b35e

Thanks for the explanation. I guess we are relying on the fact that server.Shutdown() is called before server.ListenAndServeTLS()?

benashz · 2021-08-31T14:26:44Z

subcommand/injector/command.go

 			if err != nil {
-				log.Error(fmt.Sprintf("error checking leader: %s", err))
+				log.Warn(fmt.Sprintf("Could not check leader: %s. Trying again...", err))


I noticed that some log lines are capitalized, and in other cases not. I guess we should probably be consistent either way.

Sure, I think generally log messages should start with a capitalized word, and errors themselves should start with lowercase.

Totally agree 👍

benashz · 2021-08-31T22:01:26Z

subcommand/injector/command.go

-			cancelFunc()
+		case err := <-exitOnError:
+			logger.Error("Shutting down due to error", "error", err)
+			c.shutdownHandler(ctx, server, cancelFunc)


Suggested change

c.shutdownHandler(ctx, server, cancelFunc)

// we rely on a premature shutdown to fail the server, and cause the injector to abort.

c.shutdownHandler(ctx, server, cancelFunc)

benashz

Looking good. Only one request to add comment for exitOnError

Using operator-sdk's Become() for leader election. Retries Become() 10 times (with exp backoff) and then signals the caller to exit if it failed. command.Run() now watches an exitOnError channel for that case. Co-authored-by: Ben Ash <[email protected]>

tvoran added 3 commits July 6, 2021 14:42

using operator-sdk Become() for leader election

1060be0

support v1 and v1beta1 admission api's

7eebe89

The v1 and v1beta1 AdmissionReview, AdmissionResponse, and AdmissionRequest are the same, so this bumps those to v1. Checks at the start of certWatcher() if AdmissionregistrationV1 exists, and falls back to v1beta1 if it doesn't.

set a leader label on injector replicas

885d96b

tvoran mentioned this pull request Jul 10, 2021

Support vault-k8s internal leader election hashicorp/vault-helm#568

Merged

tvoran marked this pull request as ready for review July 10, 2021 00:24

tvoran added 2 commits July 20, 2021 00:08

retry with backoff for setting leader label

e133800

And bail out if Become() fails, to prevent split-brain.

warn about problems loading TLS keypair and leader

d12851b

These are in a loop that retries once a second, and the TLS error is normal before the leader generates the cert Secret.

tvoran requested review from tomhjp, benashz and calvn July 20, 2021 07:14

tvoran added 3 commits July 22, 2021 17:09

removed leader labeling

6d837ce

it needs to be more robust than a few retries

updating k8s client-go

3736c60

support v1 and v1beta1 admission api's

3303666

The v1 and v1beta1 AdmissionReview, AdmissionResponse, and AdmissionRequest are the same, so this bumps those to v1. Checks at the start of certWatcher() if AdmissionregistrationV1 exists, and falls back to v1beta1 if it doesn't.

tvoran mentioned this pull request Jul 26, 2021

Update client go to latest #273

Merged

more error handling for getAdminAPIVersion()

91474cf

and declare payload as a []byte

tvoran changed the base branch from master to VAULT-2403/update-client-go July 27, 2021 05:09

tvoran added 3 commits July 26, 2021 22:30

Merge branch 'VAULT-2403/update-client-go' into VAULT-2403/internal-l…

4b6837b

…eader

updating webhook deployment yaml for v1

719ee63

Merge branch 'VAULT-2403/update-client-go' into VAULT-2403/internal-l…

22ea7fe

…eader

tomhjp reviewed Jul 27, 2021

View reviewed changes

tvoran added 2 commits July 27, 2021 11:58

v1 -> admissionv1

fe858b7

Merge branch 'VAULT-2403/update-client-go' into VAULT-2403/internal-l…

cf78476

…eader

Base automatically changed from VAULT-2403/update-client-go to master July 27, 2021 21:14

Merge remote-tracking branch 'origin/master' into VAULT-2403/internal…

31df5c8

…-leader

calvn reviewed Jul 27, 2021

View reviewed changes

tvoran mentioned this pull request Aug 3, 2021

Pods from same replicaset have different CA Bundles hashicorp/vault-helm#580

Open

tvoran added 3 commits August 10, 2021 14:24

added retries for the Become() call

75258f7

operator-lib 0.6.0

d9ef2ff

Merge remote-tracking branch 'origin/master' into VAULT-2403/internal…

30dc9cb

…-leader

defer stop ticker

2f0b6ee

benashz approved these changes Aug 23, 2021

View reviewed changes

tvoran and others added 4 commits August 24, 2021 16:31

Cleaning up deploy yaml

e5522cc

pod delete rbac and no more endpoints

Merge remote-tracking branch 'origin/master' into VAULT-2403/internal…

e6d0206

…-leader

Update leader/leader.go

0be17c3

Co-authored-by: Ben Ash <[email protected]>

retry Become() 10 times then exit

b73527f

Retries Become() 10 times (with exp backoff) and then signals the caller to exit if it failed. command.Run() now watches an exitOnError channel for that case.

tvoran requested a review from tomhjp August 31, 2021 00:56

tomhjp approved these changes Aug 31, 2021

View reviewed changes

benashz reviewed Aug 31, 2021

View reviewed changes

Pass an actual error through exitOneError channel

a45b35e

tvoran merged commit 0c69acb into master Aug 31, 2021

tvoran deleted the VAULT-2403/internal-leader branch August 31, 2021 21:52

benashz reviewed Aug 31, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moving leader election inside vault-k8s #271

Moving leader election inside vault-k8s #271

tvoran commented Jul 9, 2021 •

edited

Loading

tomhjp left a comment

tomhjp Jul 27, 2021

tvoran Aug 10, 2021

tomhjp Jul 27, 2021

benashz Jul 27, 2021

tvoran Aug 10, 2021

tvoran Aug 24, 2021

tvoran Aug 27, 2021

tvoran Aug 31, 2021

tomhjp Aug 31, 2021

calvn Jul 27, 2021

tvoran Aug 10, 2021

calvn Jul 27, 2021

tvoran Aug 10, 2021

benashz left a comment

benashz Jul 27, 2021

tomhjp left a comment

tomhjp Aug 31, 2021

benashz Aug 31, 2021 •

edited

Loading

tvoran Aug 31, 2021

tvoran Aug 31, 2021

benashz Aug 31, 2021

benashz Aug 31, 2021

tvoran Aug 31, 2021

benashz Aug 31, 2021

benashz Aug 31, 2021 •

edited

Loading

benashz left a comment

	if err := server.ListenAndServeTLS(c.flagCertFile, c.flagKeyFile); err != nil {
	c.UI.Error(fmt.Sprintf("Error listening: %s", err))
	return 1
	}

	exitStatus, err := c.Run()
	if err != nil {
	log.Println(err)
	}
	os.Exit(exitStatus)

	c.shutdownHandler(ctx, server, cancelFunc)
	// we rely on a premature shutdown to fail the server, and cause the injector to abort.
	c.shutdownHandler(ctx, server, cancelFunc)

Moving leader election inside vault-k8s #271

Moving leader election inside vault-k8s #271

Conversation

tvoran commented Jul 9, 2021 • edited Loading

tomhjp 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

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

benashz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomhjp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benashz Aug 31, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benashz Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

benashz left a comment

Choose a reason for hiding this comment

tvoran commented Jul 9, 2021 •

edited

Loading

benashz Aug 31, 2021 •

edited

Loading

benashz Aug 31, 2021 •

edited

Loading