-
Notifications
You must be signed in to change notification settings - Fork 18
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
Using gobgp_exporter with a dns address #29
Comments
@kurojishi , it has been a while ... I don't remember the reasons. If you feel it needs to be modified, I am all ears. |
On our end, we don't use hostnames or My interpretation is the code is supposed to be compatible with the common
It would make sense to first check for |
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? |
#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.
The text was updated successfully, but these errors were encountered: