joao-r-reis commented on code in PR #1858:
URL:
https://github.com/apache/cassandra-gocql-driver/pull/1858#discussion_r1937284049
##########
host_source.go:
##########
@@ -584,6 +598,9 @@ func (s *Session) hostInfoFromMap(row
map[string]interface{}, host *HostInfo) (*
}
ip, port := s.cfg.translateAddressPort(host.ConnectAddress(), host.port)
+ if !validIpAddr(ip) {
+ return nil, errors.New("invalid connect address")
Review Comment:
```
hostAddr := host.ConnectAddress()
return nil, fmt.Errorf("invalid connect address after translating %v:%v",
hostAddr.String(), host.port)
```
##########
conn.go:
##########
@@ -1697,7 +1697,11 @@ func (c *Conn) awaitSchemaAgreement(ctx context.Context)
(err error) {
}
for _, row := range rows {
- host, err := c.session.hostInfoFromMap(row,
&HostInfo{connectAddress: c.host.ConnectAddress(), port: c.session.cfg.Port})
+ h, err := newHostInfo(c.host.connectAddress,
c.session.cfg.Port)
Review Comment:
`c.host.ConnectAdress()`
##########
host_source.go:
##########
@@ -159,6 +159,18 @@ type HostInfo struct {
tokens []string
}
+func newHostInfo(addr net.IP, port int) (*HostInfo, error) {
+ host := &HostInfo{}
+ host.hostname = addr.String()
+ host.port = port
+ if !validIpAddr(addr) {
Review Comment:
move this check to the top
##########
host_source.go:
##########
@@ -224,18 +236,20 @@ func (h *HostInfo) ConnectAddress() net.IP {
h.mu.RLock()
defer h.mu.RUnlock()
- if addr, _ := h.connectAddressLocked(); validIpAddr(addr) {
- return addr
- }
- panic(fmt.Sprintf("no valid connect address for host: %v. Is your
cluster configured correctly?", h))
+ addr, _ := h.connectAddressLocked()
Review Comment:
There's no case where the `connectAddressLocked()` function should ever
return a non nil err now so it can be changed to just return the address
(basically remove the validation).
##########
control.go:
##########
@@ -172,7 +176,12 @@ func hostInfo(addr string, defaultPort int) ([]*HostInfo,
error) {
}
for _, ip := range ips {
- hosts = append(hosts, &HostInfo{hostname: host, connectAddress:
ip, port: port})
+ h, err := newHostInfo(ip, port)
Review Comment:
my suggestion to use `newHostInfo` was so that we could standardize every
creation of a host info object to use this function so we ensure there is no
host info object with an invalid connect address but as it stands in this PR we
still have a few places where the host info struct is being created and
initialized directly.
E.g. in `hostInfoFromIter` the host info can be created with a nil connect
address but it is provided immediately to `hostInfoFromMap` which will fix it.
This is fine with me but we should make it a bit more consistent.
As I see it, there's currently 3 different ways to create a host info object:
1) create an "empty" host info object with a nil connect address + default
port and call `hostInfoFromMap` to "fill" it
2) create a host info object from a contact point
3) update existing host info object with new values from
system.peers/system.local (also done with `hostInfoFromMap`) - this is usually
done on a host info created by 2)
For 1) I think we can add a new method `session.newHostInfoFromMap(addr
net.IP, port int, row map[string]interface{})` that just wraps around
`hostInfoFromMap`:
```
func (s *Session) newHostInfoFromMap(addr net.IP, port int, row
map[string]interface{}) (*HostInfo, error) {
return s.hostInfoFromMap(row, &HostInfo{connectAddress: addr , port:
port})
}
```
For 2. we can use the new `newHostInfo` function you created.
For 3) we can use `hostInfoFromMap` or we can use
`session.newHostInfoFromMap` just like in 1)
In summary we will have `hostInfo` struct being created in 2 places only:
`session.newHostInfoFromMap()` and `newHostInfo()` and in both of these 2
places we are ensuring that the object being returned has a valid address. Any
host info struct initialization in the codebase should be replaced by a call to
either `session.newHostInfoFromMap` or `newHostInfo()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
