Add the datacenter name validation if provided #206

sylwiaszunejko · 2024-07-04T12:02:30Z

Previously there was no check if DC name provided in the policy (DCAware and RackAware) is correct. That could lead to e.g. making routing decisions based on the wrong DC name.

This PR introduces failing to connect if DC name is different in the topology than the one the user entered.

Fixes: #205

mykaul · 2024-07-04T12:09:08Z

session.go

+		datacenters = append(datacenters, host.DataCenter())
+	}
+	if !s.policy.IsDatacenterValid(datacenters) {
+		return nil, fmt.Errorf("gocql: unable to create session: datacenter provided in the policy is not valid")


Suggested change

return nil, fmt.Errorf("gocql: unable to create session: datacenter provided in the policy is not valid")

return nil, fmt.Errorf("gocql: unable to create session: datacenter provided in the policy was not found in the topology")

dkropachev · 2024-07-04T12:35:50Z

policies.go

@@ -319,6 +319,8 @@ type HostSelectionPolicy interface {
 	// so it's safe to have internal state without additional synchronization as long as every call to Pick returns
 	// a different instance of NextHost.
 	Pick(ExecutableQuery) NextHost
+	// Checks if datacenter is valid if local aware policy is used (explicitly or as a fallback)
+	IsDatacenterValid(datacenters []string) bool


Pardon me for harsh critic, but I think this API is confusing.
What it actually does is veryfies that policy is able to properly operate on given cluster.

And I would suggest to ammed API to respresent exactly that, handover newly created Session to a policy so that it could check it self if it can operate on it:

Suggested change

IsDatacenterValid(datacenters []string) bool

IsOperational(*Session) error

But it actually just performs as single validation (today)? It'd be great if in the future we could think and maybe add additional validation, but I'm not aware of any?

I agree for 100% here, there is no other cases, at least for built-in policies, that is why my apeal only to API outlook, logic behind it and the way it may confuse future users.
@mykaul , WDYT considering all that?

I thought the name describes exactly what the function checks, so at the moment it's good enough.

What do you think of making it future proof and just feed Session to a policy to let it self check what it needs to check ?

I'm against it, just because of time pressure, not because of correctness. I think the code right now is good enough, and we are under some pressure to deliver it.

The change can be done at the same time.
(I also don't think the original issue put any time pressure here).

This issue is not that urgent - this is a footgun, not something that needs immediate fix.
Changing the interface now is easy, changing it later is not because of backwards compatibility.

Also, we should verify rack name too, right? Maybe a more general mechanism that can be extended later would be better.

@mykaul , I see that 4 people are for changing API to <>(s *Session) error ? is it ok with you to go that way ?
We can also cover rack validation same way

policies.go

dkropachev · 2024-07-04T15:26:17Z

session.go

+		if err != nil {
+			return nil, fmt.Errorf("gocql: unable to create session: %v", err)
+		}
+		for _, host := range hosts {


Please filter them out by s.cfg.filterHost(h)

sylwiaszunejko · 2024-07-04T16:43:57Z

@mykaul @Lorak-mmk
I pushed the new version with IsOperational(*Session) error API and additional check for rack. Is it what you had in mind @dkropachev ?

dkropachev

Small nit picks

dkropachev · 2024-07-04T17:09:28Z

policies.go

@@ -319,6 +319,8 @@ type HostSelectionPolicy interface {
 	// so it's safe to have internal state without additional synchronization as long as every call to Pick returns
 	// a different instance of NextHost.
 	Pick(ExecutableQuery) NextHost
+	// Checks if session is operational with the cluster on Session initialization, not when you actually run the query


Suggested change

// Checks if session is operational with the cluster on Session initialization, not when you actually run the query

// IsOperational checks if host policy can properly work with given Session/Cluster/ClusterConfig

dkropachev · 2024-07-04T17:14:42Z

policies.go

+	if !session.cfg.disableInit && !session.cfg.disableControlConn {
+		var datacenters []string
+		hosts, _, err := session.hostSource.GetHosts()
+		if err != nil {
+			return fmt.Errorf("gocql: unable to check if session is operational: %v", err)
+		}
+		for _, host := range hosts {
+			if !session.cfg.filterHost(host) {
+				datacenters = append(datacenters, host.DataCenter())
+			}
+		}
+
+		found := false
+
+		for _, dc := range datacenters {
+			if dc == d.local {
+				found = true
+				break
+			}
+		}
+
+		if !found {
+			return fmt.Errorf("gocql: datacenter provided in the policy was not found in the topology")
+		}
+	}
+
+	return nil
+
+}


@sylwiaszunejko , it is a nit, not necessary to follow, but early return is a realy good practice:

Suggested change

if !session.cfg.disableInit && !session.cfg.disableControlConn {

var datacenters []string

hosts, _, err := session.hostSource.GetHosts()

if err != nil {

return fmt.Errorf("gocql: unable to check if session is operational: %v", err)

}

for _, host := range hosts {

if !session.cfg.filterHost(host) {

datacenters = append(datacenters, host.DataCenter())

}

}

found := false

for _, dc := range datacenters {

if dc == d.local {

found = true

break

}

}

if !found {

return fmt.Errorf("gocql: datacenter provided in the policy was not found in the topology")

}

}

return nil

}

if session.cfg.disableInit || session.cfg.disableControlConn {

return nil

}

var datacenters []string

hosts, _, err := session.hostSource.GetHosts()

if err != nil {

return fmt.Errorf("gocql: unable to check if session is operational: %v", err)

}

for _, host := range hosts {

if !session.cfg.filterHost(host) && host.DataCenter() == d.local {

// Policy can work properly only if there is at least one host from target DC

// No need to check host status, since it could be down due to the outage

// We only need to make sure that policy is not misconfigured with wrong DC

return nil

}

}

return fmt.Errorf("gocql: dcAwarePolicy: datacenter %s was not found in the topology", d.local)

}

dkropachev · 2024-07-04T17:24:57Z

policies.go

+	if !session.cfg.disableInit && !session.cfg.disableControlConn {
+		var datacenters []string
+		var racks []string
+		hosts, _, err := session.hostSource.GetHosts()
+		if err != nil {
+			return fmt.Errorf("gocql: unable to check if session is operational: %v", err)
+		}
+		for _, host := range hosts {
+			if !session.cfg.filterHost(host) {
+				datacenters = append(datacenters, host.DataCenter())
+				racks = append(racks, host.Rack())
+			}
+		}
+
+		foundDC := false
+		for _, dc := range datacenters {
+			if dc == d.localDC {
+				foundDC = true
+				break
+			}
+		}
+
+		foundRack := false
+		for _, rack := range racks {
+			if rack == d.localRack {
+				foundRack = true
+				break
+			}
+		}
+
+		if !foundDC {
+			return fmt.Errorf("gocql: datacenter provided in the policy was not found in the topology")
+		}
+		if !foundRack {
+			return fmt.Errorf("gocql: rack provided in the policy was not found in the topology")
+		}
+	}
+
+	return nil
+}


Suggested change

if !session.cfg.disableInit && !session.cfg.disableControlConn {

var datacenters []string

var racks []string

hosts, _, err := session.hostSource.GetHosts()

if err != nil {

return fmt.Errorf("gocql: unable to check if session is operational: %v", err)

}

for _, host := range hosts {

if !session.cfg.filterHost(host) {

datacenters = append(datacenters, host.DataCenter())

racks = append(racks, host.Rack())

}

}

foundDC := false

for _, dc := range datacenters {

if dc == d.localDC {

foundDC = true

break

}

}

foundRack := false

for _, rack := range racks {

if rack == d.localRack {

foundRack = true

break

}

}

if !foundDC {

return fmt.Errorf("gocql: datacenter provided in the policy was not found in the topology")

}

if !foundRack {

return fmt.Errorf("gocql: rack provided in the policy was not found in the topology")

}

}

return nil

}

if session.cfg.disableInit || session.cfg.disableControlConn {

return nil

}

hosts, _, err := session.hostSource.GetHosts()

if err != nil {

return fmt.Errorf("gocql: unable to check if session is operational: %v", err)

}

for _, host := range hosts {

if !session.cfg.filterHost(host) && host.DataCenter() == d.localDC && host.Rack() == d.localRack {

// Policy can work properly only if there is at least one host from target DC+Rack

// No need to check host status, since it could be down due to the outage

// We only need to make sure that policy is not misconfigured with wrong DC+Rack

return nil

}

}

return fmt.Errorf("gocql: rack %s/%s was not found in the topology", d.localDC, d.localRack)

}

wprzytula · 2024-07-05T07:34:38Z

policies.go

+
+	return fmt.Errorf("gocql: datacenter %s in the policy was not found in the topology", d.local)


Maybe let's add a hint what's the reason? "... - probable DC aware policy misconfiguration"

wprzytula · 2024-07-05T07:34:58Z

policies.go

+			return nil
+		}
+	}
+	return fmt.Errorf("gocql: rack %s/%s was not found in the topology", d.localDC, d.localRack)


Fail to connect if DC or rack name is different in the topology than the one the user entered.

sylwiaszunejko self-assigned this Jul 4, 2024

sylwiaszunejko requested review from mykaul, Lorak-mmk, roydahan, dkropachev and wprzytula July 4, 2024 12:03

mykaul reviewed Jul 4, 2024

View reviewed changes

sylwiaszunejko force-pushed the dc_name_validation branch from bde593a to ad02224 Compare July 4, 2024 12:22

sylwiaszunejko requested a review from mykaul July 4, 2024 12:25

dkropachev requested changes Jul 4, 2024

View reviewed changes

policies.go Outdated Show resolved Hide resolved

roydahan requested review from vponomaryov and fruch July 4, 2024 13:15

sylwiaszunejko force-pushed the dc_name_validation branch 2 times, most recently from d8d02d6 to 7182b55 Compare July 4, 2024 14:34

dkropachev requested changes Jul 4, 2024

View reviewed changes

sylwiaszunejko force-pushed the dc_name_validation branch from 7182b55 to ca6ff59 Compare July 4, 2024 16:42

dkropachev approved these changes Jul 4, 2024

View reviewed changes

sylwiaszunejko force-pushed the dc_name_validation branch from ca6ff59 to 3b17292 Compare July 5, 2024 06:04

sylwiaszunejko requested a review from dkropachev July 5, 2024 06:05

wprzytula previously approved these changes Jul 5, 2024

View reviewed changes

Add the validation if session is operational

ec64325

Fail to connect if DC or rack name is different in the topology than the one the user entered.

sylwiaszunejko dismissed wprzytula’s stale review via ec64325 July 5, 2024 07:52

sylwiaszunejko force-pushed the dc_name_validation branch from 3b17292 to ec64325 Compare July 5, 2024 07:52

sylwiaszunejko requested a review from wprzytula July 5, 2024 07:52

wprzytula approved these changes Jul 5, 2024

View reviewed changes

sylwiaszunejko merged commit ed9f13a into scylladb:master Jul 5, 2024
1 check passed

Lorak-mmk mentioned this pull request Jul 9, 2024

goCQL driver accepts wrong case datacenter which disables lwt partition routing optimization #201

Closed

wprzytula self-assigned this Jul 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the datacenter name validation if provided #206

Add the datacenter name validation if provided #206

sylwiaszunejko commented Jul 4, 2024

mykaul Jul 4, 2024 •

edited

Loading

sylwiaszunejko Jul 4, 2024

dkropachev Jul 4, 2024

mykaul Jul 4, 2024

dkropachev Jul 4, 2024

mykaul Jul 4, 2024

dkropachev Jul 4, 2024

mykaul Jul 4, 2024

roydahan Jul 4, 2024

Lorak-mmk Jul 4, 2024

Lorak-mmk Jul 4, 2024 •

edited

Loading

dkropachev Jul 4, 2024 •

edited

Loading

dkropachev Jul 4, 2024

sylwiaszunejko commented Jul 4, 2024

dkropachev left a comment

dkropachev Jul 4, 2024

sylwiaszunejko Jul 5, 2024

dkropachev Jul 4, 2024 •

edited

Loading

sylwiaszunejko Jul 5, 2024

dkropachev Jul 4, 2024

sylwiaszunejko Jul 5, 2024

wprzytula Jul 5, 2024

wprzytula Jul 5, 2024

	return nil, fmt.Errorf("gocql: unable to create session: datacenter provided in the policy is not valid")
	return nil, fmt.Errorf("gocql: unable to create session: datacenter provided in the policy was not found in the topology")

	IsDatacenterValid(datacenters []string) bool
	IsOperational(*Session) error

	// Checks if session is operational with the cluster on Session initialization, not when you actually run the query
	// IsOperational checks if host policy can properly work with given Session/Cluster/ClusterConfig


		return fmt.Errorf("gocql: datacenter %s in the policy was not found in the topology", d.local)

Add the datacenter name validation if provided #206

Add the datacenter name validation if provided #206

Conversation

sylwiaszunejko commented Jul 4, 2024

mykaul Jul 4, 2024 • edited Loading

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

Lorak-mmk Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

dkropachev Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylwiaszunejko commented Jul 4, 2024

dkropachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkropachev Jul 4, 2024 • edited Loading

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

mykaul Jul 4, 2024 •

edited

Loading

Lorak-mmk Jul 4, 2024 •

edited

Loading

dkropachev Jul 4, 2024 •

edited

Loading

dkropachev Jul 4, 2024 •

edited

Loading