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

Host filter #567

Merged
merged 6 commits into from Oct 4, 2022
Merged

Host filter #567

merged 6 commits into from Oct 4, 2022

Conversation

piodul
Copy link
Collaborator

@piodul piodul commented Oct 4, 2022

This PR adds host filters. With host filters, it is possible to prevent some nodes from establishing connections to the cluster at all. For example, the set of nodes for which connections are allowed can be restricted to the local DC.

Tested manually with cql-stress and a 3 node cluster - applied an allowlist of nodes and observed in metrics that forbidden nodes do not have additional connections and do not receive any requests.

Fixes: #561

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I added appropriate Fixes: annotations to PR description.

@@ -3,6 +3,7 @@ mod cluster;
pub(crate) mod connection;
mod connection_pool;
pub mod downgrading_consistency_retry_policy;
pub mod host_filter;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could also be reexported in lib.rs for convenience.

@@ -251,6 +255,9 @@ impl MetadataReader {
let mut result = self.fetch_metadata(initial).await;
if let Ok(metadata) = result {
self.update_known_peers(&metadata);
if initial {
self.check_that_control_connection_is_an_accepted_host(&metadata);
Copy link
Contributor

@cvybhu cvybhu Oct 4, 2022

Choose a reason for hiding this comment

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

Would it be possible to do the same filtering that we do in update_known_peers in the constructor?
We would be sure that the control connection is connected to an accepted peer.

Forcing the user to specify only known_peers that are accepted by the host filter seems overly restrictive.
I can imagine that someone might want to write a single app with predefined list of peers and then deploy it in multiple DCs, something like:

let session = SessionBuilder::new()
    .known_peers(["eu-1", "eu-2", "eu-3", "us-1", "us-2", "us-3"])
    .host_filter(make_host_filter_for_dc(get_dc_from_env_variable()));
    .build()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be nice if it were possible, but unfortunately not all kinds of filters can be implemented this way. For example, we don't really know which nodes are in which DC until we fetch topology data from system.local/system.peers.

@cvybhu
Copy link
Contributor

cvybhu commented Oct 4, 2022

Let's also make an issue that documentation for HostFilter is missing in the book.

Introduces the host_filter module. Host filters will be used for
restricting the set of nodes towards which the driver should open
connections. The following filters are implemented for the time being:

- AcceptAllHostFilter: accepts all nodes,
- AllowListHostFilter: only nodes from the provided list are accepted,
- DcHostfilter: only nodes from given DC are accepted.
Now, it is possible to provide a HostFilter for a Session via
SessionConfig or SessionBuilder.
In order to make host filters effective the connection pool in Node is
wrapped in an optional. Nodes disabled by the session's host filters
will have the connection pool set to None.
Now, on topology refresh, nodes that are not accepted by the host
filtered are created in disabled mode, i.e. they do not create a
connection pool and do not establish any connections to the node.
Now, the control connection is aware of the host filter and carefully
tries to omit filtered out nodes.

If the initial nodes list contained a node that is not accepted by the
host filter and a control connection has been established to it, a
warning will be printed and the driver will try to re-establish it to
one of the accepted nodes.

Additionally, in case of a mistake where an incorrect host filter is
provided and all nodes are filtered out, a message is printed which
points out the problem.
@piodul
Copy link
Collaborator Author

piodul commented Oct 4, 2022

v2:

  • Re-exported the host_filter module in library root,
  • AllowListHostFilter::new() now accepts types which are convertible to an iterator over items that implement ToSocketAddrs. For example, addresses provided in textual form via Vec<&str> will work now,
  • In addition to warning the user about the control connection being established to a node outside the filtered set, the driver will now re-establish the control connection to another node from the filtered set.

@piodul
Copy link
Collaborator Author

piodul commented Oct 4, 2022

Let's also make an issue that documentation for HostFilter is missing in the book.

#570

@cvybhu cvybhu merged commit 90f8cdc into scylladb:main Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make it possible to provide a host filter
2 participants