Open kurojishi opened 8 months ago
Now what was your intention for valid endpoints for gobgp? i think it make sense to give the option to give a full fledged URI and then validate, i can send the patch if that's the case and i can improve the validation method.
@kurojishi , it has been a while ... I don't remember the reasons. If you feel it needs to be modified, I am all ears.
cc: @mrueg , @icedream , @vj396
On our end, we don't use hostnames or dns:
for specifying an address at all. I can attempt to derive the intention from the code.
My interpretation is the code is supposed to be compatible with the common dns:
address input that the gRPC dialing routine also accepts, as well as simple IP:port
. In its current state...
net.ParseIP
is explicitly called on the hostname. That's probably intentional.dns:
scheme check would indeed never properly trigger as the err
check takes priority.ipv4:
/ipv6:
/unix socket syntax.It would make sense to first check for dns:
(or really any scheme the gRPC document defines, maybe with net/url
package or check for substring :/
) before falling back to parsing as IP:port
. Whether we also want to add our own DNS resolving to that separately, or whether we want to simply forward the address string to gRPC dialing if it contains any scheme, I leave that open to discussion. I personally think it's not needed since gRPC already provides a framework for resolving addresses like that, it just needs documentation once the dns:
scheme check is fixed up.
Sorry for the delays, holidays and whatsnots
Imho instead of doing a lot of this manually using
https://pkg.go.dev/net/url#ParseRequestURI
then checking on the parts is more effective
objections?
https://github.com/greenpau/gobgp_exporter/pull/30 here it is, a bit of a different behavior, but it tested it now and works properly on docker
High, anyone can give a check to the related MR?
Hi, seems like the validation for hostnames it's not happy with not using ip addresses.
while trying to use the dns:// like in https://github.com/greenpau/gobgp_exporter/blob/main/pkg/gobgp_exporter/router_node.go#L96 prefix returns another error
Now this logic seems a bit weird to me and i want to make sure this is intended before sending out a patch especially cause the test have a lot of bad cases, for rejection but none of the good ones for ensuring that positive cases don't get rejected
https://github.com/greenpau/gobgp_exporter/blob/main/pkg/gobgp_exporter/gobgp_exporter_test.go#L35
and adding in the
{address: "dns://localhost:50051", ok: true},
in the list does get it rejected.But if i read this correctly is rejecting anything that is not an IP address
SplitHostPort tries to split in host/port but any colon before the port address will throw the too many colons error (https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/net/ipsock.go;l=164)
so the dns:// that is saying it's valid will always throw an error, and if there's any host it will only trigger the first branch of the if/then/else statament so it's impossible for a dns name to pass.
Now what was your intention for valid endpoints for gobgp? i think it make sense to give the option to give a full fledged URI and then validate, i can send the patch if that's the case and i can improve the validation method.