-
Notifications
You must be signed in to change notification settings - Fork 324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Run snapshot agent as a sidecar with consul servers #1620
Conversation
14387a1
to
e4c31d7
Compare
e4c31d7
to
eccf43b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I have a couple non-blocking questions about things for my learning but this looks good to me.
vaultCASecret := vault.CASecretName(vaultReleaseName) | ||
|
||
consulHelmValues := map[string]string{ | ||
"server.extraVolumes[0].type": "secret", | ||
"server.extraVolumes[0].name": vaultCASecret, | ||
"server.extraVolumes[0].load": "false", | ||
|
||
"connectInject.enabled": "true", | ||
"connectInject.enabled": "false", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question. In the past, was connectInject and controller being true just an oversight that had no bearing on the tests/functionality?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I think it might have been copy-paste from the vault test.
[ "${actual}" = 'true' ] | ||
} | ||
|
||
@test "server/StatefulSet: snapshot-agent: sets TLS env vars when global.tls.enabled" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking: Do you care about doing the opposite test, without TLS, and verifying that CONSUL_HTTP_ADDR is 'http://127.0.0.1:8500' ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good idea
@@ -1031,7 +1031,7 @@ func TestRun_NamespaceEnabled_ValidateLoginToken_PrimaryDatacenter(t *testing.T) | |||
{ | |||
ComponentName: "connect-injector", | |||
TokenFlags: []string{"-connect-inject"}, | |||
Roles: []string{resourcePrefix + "-connect-injector-acl-role"}, | |||
Roles: []string{resourcePrefix + "-connect-inject-acl-role"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious why this changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we used to derive the role name from the service account of the component. With snapshot agent now being co-located with servers, the service account would be the server's service account. I changed it to instead use the component name that we pass to these functions so that both policy and role are derived the same way and so that the role will still clearly say that it's for the snapshot agent. This resulted in ripple effects for other roles which were not named the same as the component.
af781f9
to
0ce7cf1
Compare
Changes proposed in this PR:
Now that we don't have client, snapshot agent needs to run against the only agent remaining - the server agent.
How I've tested this PR:
acceptance tests
How I expect reviewers to test this PR:
👀
Checklist: