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

Log warnings if there are multiple application and/or admin connectors #189

Closed
sleberknight opened this issue Aug 6, 2024 · 1 comment · Fixed by #191
Closed

Log warnings if there are multiple application and/or admin connectors #189

sleberknight opened this issue Aug 6, 2024 · 1 comment · Fixed by #191
Assignees
Labels
enhancement A request for change or improvement to an existing feature
Milestone

Comments

@sleberknight
Copy link
Member

sleberknight commented Aug 6, 2024

If a Dropwizard application contains more than one application or admin connector, then only the last connector's host and port are registered with Consul in ConsulServiceListener. This can cause problems depending on which scheme and port are registered with Consul.

This "last wins" has been the library's behavior for many years now. However, we just encountered it in one specific service which had two application connectors: an "https" one for external clients and an "http" one that binds only to localhost and is used for specialized inter-process communication with another application running locally on the same server.

The localhost application connector was the last one in the YAML configuration, and its "http" scheme and application port was the one registered in Consul. Because the scheme was "http" and not "https", this also caused Consul to de-register the service, since it was trying to contact the application health check using "http" on the admin port instead of "https".

We fixed it by moving the localhost connector as the first one, so that the last one is the one available to external clients and is registered with Consul. And, the scheme was then "https" so Consul was able to communicate with the service health check.

We could have also fixed it by explicitly setting a specific port in the ConsulFactory configuration.

For now, we do not plan to change the existing behavior, but we will add WARN-level logging if there are more than one application and/or admin connector, or if there is a connector with an unknown name (not "application" or "admin") which indicates the "simple" server type is in use. The logging should indicate that only the last scheme and port are registered with Consul unless there is explicit configuration for ConsulFactory.

If the logging had existed when this happened, we could have tracked the problem down much more quickly.

Later, we might consider adding a configuration option that would fail-fast if there are multiple application and/or admin connectors but there is no explicit configuration for the service port, scheme, and/or host.

@sleberknight sleberknight added the enhancement A request for change or improvement to an existing feature label Aug 6, 2024
@sleberknight sleberknight added this to the 1.1.6 milestone Aug 6, 2024
@sleberknight sleberknight self-assigned this Aug 6, 2024
sleberknight added a commit that referenced this issue Aug 15, 2024
* Add logging to ConsulServiceListener when there are multiple
  application and/or admin connectors
* Shimmed several counters into the existing loop instead
  of trying to refactor it
* Put new logging logic in a new method, logWarningsIfNecessary,
  which is called from within the loop

Closes #189
@sleberknight
Copy link
Member Author

Note that the merged code ( see #191 ) always logs the warnings when there are multiple connectors. Without changing the public API (e.g., adding a ConsulFactory argument to the constructor), there is no way for ConsulServiceListener to know whether the configuration includes service and/or admin port. In addition, ConsulFactory does not contain anything about the scheme to use, instead relying on the protocols in the application/admin connectors. So, at this point it's not worth adding this.

For the same reason as above, we won't add an option to "fail-fast if there are multiple application and/or admin connectors but there is no explicit configuration".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A request for change or improvement to an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant