connect: rewrite envoy bootstrap on every restart #19787
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes #19781 Do not mark the envoy bootstrap hook as done after successfully running once. Since the bootstrap file is written to /secrets, which is a tmpfs on supported platforms, it is not persisted across reboots. This causes the task and allocation to fail on reboot (see #19781). This fixes it by *always* rewriting the envoy bootstrap file every time the Nomad agent starts. This does mean we may write a new bootstrap file to an already running Envoy task, but in my testing that doesn't have any impact. *Alternative 1: Use a regular file* An alternative approach would be to write the bootstrap file somewhere other than the tmpfs, but this is *unsafe* as when Consul ACLs are enabled the file will contain a secret token: https://developer.hashicorp.com/consul/commands/connect/envoy#bootstrap *Alternative 2: Detect if file is already written* An alternative approach would be to detect if the bootstrap file exists, and only write it if it doesn't. This is just a more complicated form of the current fix. I think in general in the absence of other factors task hooks should be idempotent and therefore able to rerun on any agent startup. This simplifies the code and our ability to reason about task restarts vs agent restarts vs node reboots by making them all take the same code path.
tgross
reviewed
Jan 22, 2024
There was a problem hiding this comment.
LGTM code-wise. Interestingly, this fix looks similar to what we did in the envoy_version hook: #16815. When we get some time it might be helpful to see where else we have this kind of logic in the hooks that has forgotten about host restarts.
The TestTaskRunner_EnvoyBootstrapHook_gateway_ok test is failing, which seems relevant to this change. (There's also a flaky test in the leadership transfer command tests which we know about in #19718).
schmichael
added
backport/1.5.x
It's no longer accurate and isn't really related to this test.
shoenig
reviewed
Jan 24, 2024
| // | ||
| // Done is useful for expensive operations such as downloading artifacts, or | ||
| // for operations which might fail needlessly if rerun while a node is | ||
| // disconnected. | ||
| Done bool |
There was a problem hiding this comment.
great comment, this actually clarifies a lot
shoenig
approved these changes
Jan 24, 2024
tgross
approved these changes
Jan 24, 2024
This was referenced Jan 24, 2024
nvanthao
pushed a commit
to nvanthao/nomad
that referenced
this pull request
Mar 1, 2024
Fixes hashicorp#19781 Do not mark the envoy bootstrap hook as done after successfully running once. Since the bootstrap file is written to /secrets, which is a tmpfs on supported platforms, it is not persisted across reboots. This causes the task and allocation to fail on reboot (see hashicorp#19781). This fixes it by *always* rewriting the envoy bootstrap file every time the Nomad agent starts. This does mean we may write a new bootstrap file to an already running Envoy task, but in my testing that doesn't have any impact. This commit doesn't necessarily fix every use of Done by hooks, but hopefully improves the situation. The comment on Done has been expanded to hopefully avoid misuse in the future. Done assertions were removed from tests as they add more noise than value. *Alternative 1: Use a regular file* An alternative approach would be to write the bootstrap file somewhere other than the tmpfs, but this is *unsafe* as when Consul ACLs are enabled the file will contain a secret token: https://developer.hashicorp.com/consul/commands/connect/envoy#bootstrap *Alternative 2: Detect if file is already written* An alternative approach would be to detect if the bootstrap file exists, and only write it if it doesn't. This is just a more complicated form of the current fix. I think in general in the absence of other factors task hooks should be idempotent and therefore able to rerun on any agent startup. This simplifies the code and our ability to reason about task restarts vs agent restarts vs node reboots by making them all take the same code path.
nvanthao
pushed a commit
to nvanthao/nomad
that referenced
this pull request
Mar 1, 2024
Fixes hashicorp#19781 Do not mark the envoy bootstrap hook as done after successfully running once. Since the bootstrap file is written to /secrets, which is a tmpfs on supported platforms, it is not persisted across reboots. This causes the task and allocation to fail on reboot (see hashicorp#19781). This fixes it by *always* rewriting the envoy bootstrap file every time the Nomad agent starts. This does mean we may write a new bootstrap file to an already running Envoy task, but in my testing that doesn't have any impact. This commit doesn't necessarily fix every use of Done by hooks, but hopefully improves the situation. The comment on Done has been expanded to hopefully avoid misuse in the future. Done assertions were removed from tests as they add more noise than value. *Alternative 1: Use a regular file* An alternative approach would be to write the bootstrap file somewhere other than the tmpfs, but this is *unsafe* as when Consul ACLs are enabled the file will contain a secret token: https://developer.hashicorp.com/consul/commands/connect/envoy#bootstrap *Alternative 2: Detect if file is already written* An alternative approach would be to detect if the bootstrap file exists, and only write it if it doesn't. This is just a more complicated form of the current fix. I think in general in the absence of other factors task hooks should be idempotent and therefore able to rerun on any agent startup. This simplifies the code and our ability to reason about task restarts vs agent restarts vs node reboots by making them all take the same code path.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #19781
Do not mark the envoy bootstrap hook as done after successfully running once. Since the bootstrap file is written to /secrets, which is a tmpfs on supported platforms, it is not persisted across reboots. This causes the task and allocation to fail on reboot (see #19781).
This fixes it by always rewriting the envoy bootstrap file every time the Nomad agent starts. This does mean we may write a new bootstrap file to an already running Envoy task, but in my testing that doesn't have any impact.
Alternative 1: Use a regular file
An alternative approach would be to write the bootstrap file somewhere other than the tmpfs, but this is unsafe as when Consul ACLs are enabled the file will contain a secret token:
https://developer.hashicorp.com/consul/commands/connect/envoy#bootstrap
Alternative 2: Detect if file is already written
An alternative approach would be to detect if the bootstrap file exists, and only write it if it doesn't.
This is just a more complicated form of the current fix. I think in general in the absence of other factors task hooks should be idempotent and therefore able to rerun on any agent startup. This simplifies the code and our ability to reason about task restarts vs agent restarts vs node reboots by making them all take the same code path.