Always set server acl tokens #832
Conversation
lkysow
force-pushed
the
lkysow/consul-server-acl-rerun
branch
from
36363cf to
3692c49
Compare
| @@ -313,7 +314,6 @@ func (c *Command) Run(args []string) int { | |||
| scheme = "https" | |||
| } | |||
|
|
|||
| var updateServerPolicy bool | |||
There was a problem hiding this comment.
This is no longer needed. Previously this would be set when there already exists a kube bootstrap secret to ensure that we update the server policy. Now we call c.bootstrapServers, even when the kube secret exists. That function then calls setServerTokens that then calls c.setServerPolicy(consulClient). So we don't need this codepath anymore.
| @@ -1650,6 +1677,81 @@ func TestRun_AlreadyBootstrapped(t *testing.T) { | |||
| }, consulAPICalls) | |||
| } | |||
|
|
|||
| // Test if there is an old bootstrap Secret and the server token exists | |||
There was a problem hiding this comment.
I added this to test that we don't re-create the server tokens if the token already exists.
lkysow
force-pushed
the
lkysow/consul-server-acl-rerun
branch
from
3692c49 to
c28e0be
Compare
| return 1 | ||
| } | ||
| } | ||
| bootstrapToken, err = c.bootstrapServers(serverAddresses, bootstrapToken, bootTokenSecretName, scheme) |
| case "/v1/acl/tokens": | ||
| fmt.Fprintln(w, `[]`) |
There was a problem hiding this comment.
Some of these tests get modified because now we do a token list call to ensure we're not recreating server tokens.
|
|
||
| if bootstrapToken == "" { | ||
| var err error | ||
| bootstrapToken, err = c.bootstrapACLs(firstServerAddr, scheme, bootTokenSecretName) |
There was a problem hiding this comment.
I extracted some of the code that was here into bootstrapACLs.
If server-acl-init is re-run, ensure that server ACL tokens have been created for each server and call the update token API for each server again. This ensures that if server-acl-init fails to pass out tokens to every server that when it is re-run it won't assume that every server has a token.
lkysow
force-pushed
the
lkysow/consul-server-acl-rerun
branch
from
c28e0be to
bfb52ac
Compare
lkysow
requested review from
a team,
thisisnotashwin and
t-eckert
and removed request for
a team
Increase the memory for the client daemonset's tls-init container. We saw an out of memory error when running on OpenShift.
|
About to review this. Should we add a test for this scenario to our acceptance tests? |
The pattern we follow with acceptance tests is to test the happy path of big features. We don't test failure paths because there are too many and so our acceptance tests would be too large and take too long. With regards to the bugfix where this is now idempotent, I don't think it fits the criteria for acceptance tests because it's not a happy path nor is it a feature. With regards to being able to upgrade to a greater number of servers, this is something that might make sense in an acceptance test but currently we don't do any upgrade testing. We've had a plan to do upgrade testing on our backlog so I think it would make sense to evaluate this when we address that ticket. |
If server-acl-init is re-run, ensure that server ACL tokens have been
created for each server and call the update token API for each server
again.
This ensures that if server-acl-init fails to pass out tokens to every
server that when it is re-run it won't assume that every server has a
token.
In addition this fixes a long-standing issue where if you increase your number of servers the new servers never get ACL tokens provisioned.
Fixes #825, #677
How I've tested this PR:
Brough up cluster with 3 servers:
Then updated to 5 servers and ran helm upgrade.
Confirmed that there are no "Coordinate update" errors in logs of new servers. This would indicate they didn't have an ACL token set.
Confirmed that there are still only 5 server tokens (so it didn't re-create tokens)
How I expect reviewers to test this PR:
Checklist: