Add ability to connect to nodes through proxy #97

zimnx · 2022-06-06T14:03:20Z

Services deployed in cloud often are hidden behind proxy which
dispatches connections based on Server Name Identifier (SNI)
taken from TLS Client Hello packet.
New method of creating ClusterConfig - NewCloudCluster - allows to
connect to nodes behind SNI proxy based on provided configuration file.

Because each datacenter may have different TLS configurtion (CA, proxy
address etc), more granular method of configuring connection details was
needed. CloudCluster use special HostDialer which connect to nodes using
information taken from HostInfo (datacenter, host_id) to go through SNI
proxy.

Currently driver identifies nodes based on their broadcasted IP
address. In cloud case, broadcasted IP addresses are private and are not
meant to be used as a contact point, and they may change overtime.
Hence driver internals were changed to identify nodes based on their
host_id which is unique per node and it's persistant throught entire
node lifecycle.
Because CQL Events are still using broadcasted IP addresses, driver
keep mapping between already known IP addresses and host_ids.

cloud_config.go

mmatczuk · 2022-06-06T14:07:46Z

cloud_config.go

+	ProxyURL string `yaml:"proxyUrl,omitempty"`
+}
+
+type Context struct {


Please add Cloud prefix to everything defined in the cloud context or move the code to cloud package for namespacing.

moved to cloud pkg

removed pkg dir, cloud is now in root dir

mmatczuk · 2022-06-06T14:10:45Z

cloud_config.go

+func (cc *CloudConnectionConfig) GetDatacenterCAPool(datacenterName string) (*x509.CertPool, error) {
+	dc, ok := cc.Datacenters[datacenterName]
+	if !ok {
+		return nil, fmt.Errorf("datacenter %s not found in cloud connection config", datacenterName)


Use %q everywhere, if you want to be super nice you can list available DoCs

cloud_config.go

common_test.go

mmatczuk · 2022-06-07T08:25:57Z

In a library there is no need to add pkg

mmatczuk · 2022-06-07T08:27:46Z

pkg/cloud/config.go

+	// Servers may infer this from the endpoint the client submits requests to.
+	// In CamelCase.
+	// +optional
+	Kind string `yaml:"kind,omitempty"`


Why do we need it? I find no usages.

Both Kind and APIVersion will be used to distiguish version of configuration. It's not decided yet how we are going to call it (Kind), and from which version (APIVersion) we are going to start with (v1alpha1, v1beta1, v1 etc). Once we progress with API, client code would be able to parse just these two fields to find out which version of API is used and pass right data structure for unmarshalling.

mmatczuk · 2022-06-07T08:28:16Z

pkg/cloud/config.go

+	// Servers should convert recognized schemas to the latest internal value, and
+	// may reject unrecognized values.
+	// +optional
+	APIVersion string `yaml:"apiVersion,omitempty"`


mmatczuk · 2022-06-07T08:32:06Z

pkg/cloud/config.go

+	Contexts map[string]*Context `yaml:"contexts"`
+	// CurrentContext is the name of the context that you would like to use by default.
+	CurrentContext string `yaml:"currentContext"`


What is the purpose of dynamically changing the context?
It is not even a use case if you thy changing it there is a race.

It's not meant to be dynamic. This is representation of configuration file taken from disk which is read at a time of when connection to cluster is created, usually on application startup.

Gotcha.
See my suggestion on making this package capable of producing gocql.Cluster and being just a helper.

mmatczuk · 2022-06-07T08:34:17Z

pkg/cloud/config.go

+	Username string `yaml:"username,omitempty"`
+	// Password is the password for basic authentication to the Scylla cluster.
+	// +optional	`
+	Password string `yaml:"password,omitempty"`


Can we use the username and password from Authenticator?

You can overwrite it after calling NewCloudCluster. This is meant to set up a PasswordAuthenticator from configuration file without any code change.

mmatczuk · 2022-06-07T08:41:09Z

pkg/cloud/config.go

+type Parameters struct {
+	// DefaultConsistency is the default consistency level used for queries.
+	// +optional
+	DefaultConsistency ConsistencyString `yaml:"defaultConsistency,omitempty"`
+}
+
+type ConsistencyString string
+
+// just AnyConsistency etc is better, but there's already SerialConsistency defined elsewhere.
+const (
+	DefaultAnyConsistency         ConsistencyString = "ANY"
+	DefaultOneConsistency         ConsistencyString = "ONE"
+	DefaultTwoConsistency         ConsistencyString = "TWO"
+	DefaultThreeConsistency       ConsistencyString = "THREE"
+	DefaultQuorumConsistency      ConsistencyString = "QUORUM"
+	DefaultAllConsistency         ConsistencyString = "ALL"
+	DefaultLocalQuorumConsistency ConsistencyString = "LOCAL_QUORUM"
+	DefaultEachQuorumConsistency  ConsistencyString = "EACH_QUORUM"
+	DefaultSerialConsistency      ConsistencyString = "SERIAL"
+	DefaultLocalSerialConsistency ConsistencyString = "LOCAL_SERIAL"
+	DefaultLocalOneConsistency    ConsistencyString = "LOCAL_ONE"
+)


Why not use consistency settings in the driver?
Also SerialCosistency and Consistency are distinct things.

Driver consistency settings use uint16, where in configuration file this option should be human readable. It is unmarshalled to uint16 Consistency here: https://github.com/zimnx/gocql/blob/e6c71c39c4fba48984118757290a2697cf1105dc/cluster.go#L315-L321

Yeah now i see there're two fields for query consistency, then I guess Config API should reflect that too.

Added second consistency to config and validation

mmatczuk · 2022-06-07T08:43:44Z

pkg/cloud/config.go

+	return caPool, nil
+}
+
+func (cc *ConnectionConfig) GetInitialContactPoints() []string {


There is a naming mismatch here and in Datacenter.Server

Initial contact point is something that is related to how drivers works, initially they contact a list of endpoints and discover peers from system table. From API point of view, it's just a address to datacenter hence name mismatch.

mmatczuk · 2022-06-07T08:44:22Z

ring.go

@@ -15,7 +14,7 @@ type ring struct {

 	// hosts are the set of all hosts in the cassandra ring that we know of
 	mu    sync.RWMutex
-	hosts map[string]*HostInfo
+	hosts map[UUID]*HostInfo


This change could go to a separate commit.

mmatczuk · 2022-06-07T08:46:30Z

ring.go

+	for _, hi := range r.hosts {
+		if hi.connectAddress.String() == ip {
+			return hi, true
+		}
+	}
+	return nil, false


Most likely this code is in a driver hot loop did you profile the changes?
Use something with O(1) runtime.

I see now it's in events only so no a biggie I guess. Still it would be nice to preserve the mapping from IP to ID and fallback to getHost.

mmatczuk · 2022-06-07T08:52:45Z

events.go

@@ -159,57 +159,57 @@ func (s *Session) handleNodeEvent(frames []frame) {
 		}
 	}

-	for _, f := range events {
+	for _, event := range events {


Can we remain the orig name? It would be easier to maintain.

It's no longer a frame, but parsed event hence f doesn't have anything in common with variable semantic. I stumbled upon it, and fixed to maintain higher quality of code.

I understand that but it also modifies lines that would not be otherwise modified and thus makes it harder to review / maintain in the future.

brought it back

Feel free to submit the renaming in a separate commit/pull request though, we can update it both in gocql/gocql and scylladb/gocql.

mmatczuk · 2022-06-07T08:54:41Z

events.go

+	if ok && host.IsUp() {
+		if host.IsUp() {
+			return
+		}


The inner if looks like a dead code

bugf/leftover, fixed

mmatczuk · 2022-06-07T08:57:23Z

events.go

 	// Get host info and apply any filters to the host
-	hostInfo, err := s.hostSource.getHostInfo(ip, port)


Let's not rename variables if we do not have to.

mmatczuk · 2022-06-07T09:13:27Z

cluster.go

+	connConf := &cloud.ConnectionConfig{}
+
+	bundleFile, err := os.Open(bundlePath)
+	if err != nil {
+		return nil, fmt.Errorf("can't open bundle path: %w", err)
+	}
+	defer bundleFile.Close()
+
+	if err := yaml.NewDecoder(bundleFile).Decode(connConf); err != nil {
+		return nil, fmt.Errorf("can't decode bundle file at %q: %w", bundlePath, err)
+	}
+
+	if _, ok := connConf.Contexts[connConf.CurrentContext]; !ok {
+		return nil, fmt.Errorf("current context points to unknown context")
+	}
+
+	confContext := connConf.Contexts[connConf.CurrentContext]
+
+	if _, ok := connConf.AuthInfos[confContext.AuthInfoName]; !ok {
+		return nil, fmt.Errorf("context %q auth info points to unknown authinfo", connConf.CurrentContext)
+	}
+
+	if _, ok := connConf.Datacenters[confContext.DatacenterName]; !ok {
+		return nil, fmt.Errorf("context %q datacenter points to unknown datacenter", connConf.CurrentContext)
+	}
+
+	authInfo := connConf.AuthInfos[confContext.AuthInfoName]
+
+	caPool, err := connConf.GetRootCAPool()
+	if err != nil {
+		return nil, fmt.Errorf("can't create root CA pool: %w", err)
+	}
+


Please move the parsing to cloud pkg.

I can't due to cycle in imports - i'm reusing default values from NewCluster here.

martin-sucha · 2022-06-20T18:59:00Z

Would something like kiwicom@5b9259c work for you?

@martin-sucha If you can merge it upstream we would rebase and use HostDialer.

I tried cherry-picking your commit and using it here, unfortuantely it's not going to work with our shard-aware ports where we are using custom dialer created in runtime. We would need to somehow integrate HostDialer with shard-aware port logic, but it's outside of scope of this PR.

Yes, I tried cherry-picking it last week, but didn't manage to post a response here. I have an unfinished experimental code to make the HostDialer work with shard-aware ports, will try to finish it and send as a separate PR.

martin-sucha · 2022-06-21T13:35:40Z

I've opened gocql#1629 and #98 for the HostDialer. Please let me know in those PRs what do you think about it.

zimnx · 2022-06-22T13:20:44Z

Rebased on top of #98, Endpoint structure was removed and code looks cleaner now.

Requires #98 before merge.

martin-sucha · 2022-06-22T18:10:22Z

@zimnx It seems commits

Fix race condition in HostInfo.HostnameAndPort
Identify nodes by their host_id instead broadcasted_address

would be useful upstream, would you please submit them to gocql/gocql as well?

zimnx · 2022-06-22T19:20:33Z

Done:
gocql#1632
gocql#1631

martin-sucha · 2022-06-22T19:48:47Z

Thanks!

Function changed HostInfo.hostname without holding write lock. Added write lock around it.

Currently driver identifies nodes based on their broadcasted IP address. In cloud case, broadcasted IP addresses are private and are not meant to be used as a contact point, and they may change overtime. Hence driver internals were changed to identify nodes based on their host_id which is unique per node and it's persistant throught entire node lifecycle. Because CQL Events are still using broadcasted IP addresses, driver keep mapping between already known IP addresses and host_ids. Prepared statement cache key was also changed to host_id, to not invalidate it upon IP change.

Services deployed in cloud often are hidden behind proxy which dispatches connections based on Server Name Identifier (SNI) taken from TLS Client Hello packet. New method of creating ClusterConfig - NewCloudCluster - allows to connect to nodes behind SNI proxy based on provided configuration file. Because each datacenter may have different TLS configurtion (CA, proxy address etc), more granular method of configuring connection details was needed. CloudCluster use special HostDialer which connect to nodes using information taken from HostInfo (datacenter, host_id) to go through SNI proxy.

mmatczuk · 2022-06-30T09:49:12Z

LGTM

So the different drivers can be test their new implementations of sni-proxy for using it, need to start the cluster from the commandline like this: ``` ❯ ccm start --sni-proxy sni_proxy listening on: 127.0.0.1:443 ``` using it from python code, would be a bit diffrent: ```python nodes_info = get_cluster_info(self.cluster.get_path(), address=self.cluster.nodelist()[0].address(), port=9142) docker_id, listen_address, listen_port = \ start_sni_proxy(self.cluster.get_path(), nodes_info=nodes_info) ``` Ref: scylladb/gocql#97

using code introduce in scylladb/gocql#97 for connect via sni_proxy to the serverless operator Ref: scylladb/gocql@751bff9

mmatczuk reviewed Jun 6, 2022

View reviewed changes

cloud_config.go Outdated Show resolved Hide resolved

mmatczuk reviewed Jun 6, 2022

View reviewed changes

cloud_config.go Outdated Show resolved Hide resolved

zimnx force-pushed the mz/scaas branch from e546218 to 027602d Compare June 6, 2022 15:00

zimnx requested review from piodul and tnozicka June 6, 2022 15:03

zimnx force-pushed the mz/scaas branch 8 times, most recently from 3bb45a9 to 03ceff4 Compare June 7, 2022 08:08

mmatczuk reviewed Jun 7, 2022

View reviewed changes

common_test.go Outdated Show resolved Hide resolved

mmatczuk reviewed Jun 7, 2022

View reviewed changes

zimnx force-pushed the mz/scaas branch from 03ceff4 to 7d19d36 Compare June 7, 2022 09:09

mmatczuk reviewed Jun 7, 2022

View reviewed changes

zimnx force-pushed the mz/scaas branch from 40b19df to 7ead81d Compare June 22, 2022 12:57

zimnx mentioned this pull request Jun 22, 2022

Add HostDialer interface #98

Merged

zimnx force-pushed the mz/scaas branch from 7ead81d to 742f4cf Compare June 22, 2022 13:19

zimnx force-pushed the mz/scaas branch from 742f4cf to 5a4c3f6 Compare June 22, 2022 19:23

zimnx force-pushed the mz/scaas branch 7 times, most recently from 6aa6646 to 337e28e Compare June 29, 2022 10:10

zimnx added 3 commits June 30, 2022 11:48

Fix race condition in HostInfo.HostnameAndPort

666d37e

Function changed HostInfo.hostname without holding write lock. Added write lock around it.

zimnx force-pushed the mz/scaas branch from 337e28e to 36c7755 Compare June 30, 2022 09:48

mmatczuk self-requested a review June 30, 2022 09:49

mmatczuk approved these changes Jun 30, 2022

View reviewed changes

mmatczuk merged commit 751bff9 into scylladb:master Jun 30, 2022

fruch added a commit to fruch/scylla-bench that referenced this pull request Oct 2, 2022

Add support for passing down cloud bundle

5e756b4

using code introduce in scylladb/gocql#97 for connect via sni_proxy to the serverless operator Ref: scylladb/gocql@751bff9

fruch mentioned this pull request Oct 2, 2022

Add support for passing down cloud bundle scylladb/scylla-bench#108

Merged

fruch added a commit to scylladb/scylla-bench that referenced this pull request Nov 2, 2022

Add support for passing down cloud bundle

e57d2c5

using code introduce in scylladb/gocql#97 for connect via sni_proxy to the serverless operator Ref: scylladb/gocql@751bff9

		// Get host info and apply any filters to the host
		hostInfo, err := s.hostSource.getHostInfo(ip, port)

Add ability to connect to nodes through proxy #97

Add ability to connect to nodes through proxy #97

Conversation

zimnx commented Jun 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimnx Jun 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmatczuk Jun 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-sucha commented Jun 20, 2022

martin-sucha commented Jun 21, 2022

zimnx commented Jun 22, 2022

martin-sucha commented Jun 22, 2022

zimnx commented Jun 22, 2022

martin-sucha commented Jun 22, 2022

mmatczuk commented Jun 30, 2022

zimnx commented Jun 6, 2022 •

edited

zimnx Jun 7, 2022 •

edited

mmatczuk Jun 7, 2022 •

edited