Skip to content
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

add service registration polling to connect-init #452

Merged
merged 20 commits into from
Mar 19, 2021

Conversation

kschoche
Copy link
Contributor

@kschoche kschoche commented Mar 12, 2021

Changes proposed in this PR:

  • Extends connect-init command to wait for the service to be registered. When the endpoints controller is added this will be allow the init container to wait for the service to be registered prior to bootstrapping envoy.
  • Adds pod-name , pod-namespace flags to connect-init as well as a flag to disable the new service registration polling feature to preserve backwards compatibility until the endpoints controller is merged.

How I've tested this PR:
Unit tests added and old ones passing.
Manually build consul-k8s dev image and deploy with ACLs enabled and connect inject an application.

How I expect reviewers to test this PR:
code review and manually test by deploying consul and an injected app and seeing that it gets injected and started.

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@kschoche kschoche added area/connect Related to Connect service mesh, e.g. injection theme/tproxy Items related to transparent proxy labels Mar 12, 2021
@kschoche kschoche requested review from a team, lkysow and ishustava and removed request for a team March 16, 2021 16:02
@kschoche kschoche marked this pull request as ready for review March 16, 2021 16:03
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good so far! I've left some comments, questions, and suggestions. Most of them are minor code or cleanup suggestions and improvements, as well as some testing suggestions.

connect-inject/container_init.go Outdated Show resolved Hide resolved
connect-inject/container_init.go Outdated Show resolved Hide resolved
subcommand/connect-init/command.go Outdated Show resolved Hide resolved
subcommand/connect-init/command.go Outdated Show resolved Hide resolved
subcommand/connect-init/command.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Nice RetryServicePolling test! Just had an idea for a comment to be added.

subcommand/connect-init/command.go Outdated Show resolved Hide resolved
require.Equal(t, 0, exitCode)
case <-time.After(time.Second * 10):
// Fail if the stopCh was not caught.
require.Fail(t, "timeout waiting for command to exit")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although thinking about it and the way the test is set up I don't think we will ever hit this case with the way the code is currently written. If the command times out after 10 seconds then it'll send 1 to the exitCode channel and that will still fall into the first case.

The only way it could go into the second case is when the command is implemented where it doesn't exit after 10 sec (or at all). Then we'd hit this case, but that would mean that the command is still running after the test has finished running. So I think to be good citizens when you start something in a goroutine in a test you should always make sure the goroutine finishes when the test terminates, and we probably shouldn't make an assumption that the command will always be implemented in a certain way.

Like you said though, the command doesn't really catch signals. But perhaps there are a couple of ways we can work with it that we can look into:

  1. One idea is to set context to the backoff and allow that context to be set by the tests. I think you can do that by wrapping it with backoff.WithContext function. Then you could always terminate the command by canceling the context.
  2. You could instead start the test agent in a goroutine after a delay. Then because the agent is not available, it'll force polling to retry. You can always terminate the agent by calling agent.
  3. You could register services in a goroutine after a delay.

subcommand/connect-init/command_test.go Outdated Show resolved Hide resolved
Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>
@kschoche kschoche merged commit 20c8a03 into feature-tproxy Mar 19, 2021
@kschoche kschoche deleted the tproxy-add-service-registration-polling branch March 19, 2021 17:46
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
* Add service registration polling to connect-init
Co-authored-by: Iryna Shustava <iryna@hashicorp.com>
thisisnotashwin pushed a commit that referenced this pull request Mar 26, 2021
* Add service registration polling to connect-init
Co-authored-by: Iryna Shustava <iryna@hashicorp.com>
ishustava pushed a commit that referenced this pull request Apr 14, 2021
* Add service registration polling to connect-init
Co-authored-by: Iryna Shustava <iryna@hashicorp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connect Related to Connect service mesh, e.g. injection theme/tproxy Items related to transparent proxy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants