You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@devlake.apache.org by GitBox <gi...@apache.org> on 2022/11/18 10:23:46 UTC
[GitHub] [incubator-devlake] albinvass opened a new pull request, #3761: Don't check network connection when service is behind a proxy
albinvass opened a new pull request, #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761
### ⚠️ Pre Checklist
> Please complete _ALL_ items in this checklist, and remove before submitting
- [x] I have read through the [Contributing Documentation](https://devlake.apache.org/community/).
- [x] I have added relevant tests.
- [x] I have added relevant documentation.
- [x] I will add labels to the PR, such as `pr-type/bug-fix`, `pr-type/feature-development`, etc.
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes.
Please fill out as many sections below as possible.
-->
### Summary
Currently the NewApiClient function errors if the service is behind a proxy since it cannot establish a direct connection.
Instead skip the network test when a proxy is configured.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#discussion_r1026793254
##########
plugins/helper/api_client.go:
##########
@@ -84,10 +84,13 @@ func NewApiClient(
if err != nil {
return nil, errors.Default.New("Failed to resolve Port")
}
- err = utils.CheckNetwork(parsedUrl.Hostname(), port, 10*time.Second)
- if err != nil {
- return nil, errors.Default.Wrap(err, "Failed to connect")
- }
+
+ if proxy != "" {
Review Comment:
I'm not sure we'd want to skip checking the connection altogether. Could we not just call CheckNetwork() on the proxy instead?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] albinvass commented on a diff in pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
albinvass commented on code in PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#discussion_r1027696049
##########
plugins/helper/api_client.go:
##########
@@ -84,10 +84,13 @@ func NewApiClient(
if err != nil {
return nil, errors.Default.New("Failed to resolve Port")
}
- err = utils.CheckNetwork(parsedUrl.Hostname(), port, 10*time.Second)
- if err != nil {
- return nil, errors.Default.Wrap(err, "Failed to connect")
- }
+
+ if proxy != "" {
Review Comment:
That we could do.
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] albinvass commented on pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
albinvass commented on PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#issuecomment-1321923625
> But why not CheckNetwork by proxy?
Well currently the network check is just trying to set up a connection to the server, which isn't possible because it's behind an http proxy. To test by proxy the helper object would need to do an http request. Not sure if that can be done in a generic enough way to support any http api (if ApiClient is supposed to be a generic helper). Maybe it's better to let the different plugins check connectivity to the service?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh commented on pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
likyh commented on PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#issuecomment-1321724386
But why not CheckNetwork by proxy?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on a diff in pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
keon94 commented on code in PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#discussion_r1028393385
##########
plugins/helper/api_client.go:
##########
@@ -107,6 +110,16 @@ func NewApiClient(
}
if proxy != "" {
+ var pu, err := url.Parse(proxy)
+ if err != nil {
+ return nil, errors.BadInput.Wrap(err, fmt.Sprintf("Invalid Proxy URL: %s", proxy))
+ }
+
+ err = utils.CheckNetwork(pu.Hostname(), port, 10*time.Second)
+ if err != nil {
+ return nil, errors.Default.Wrap(err, "Failed to connect to proxy")
Review Comment:
are you still encountering timeouts after making this change?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] keon94 commented on pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
keon94 commented on PR #3761:
URL: https://github.com/apache/incubator-devlake/pull/3761#issuecomment-1322468828
> > But why not CheckNetwork by proxy?
>
> Well currently the network check is just trying to set up a connection to the server, which isn't possible because it's behind an http proxy. To test by proxy the helper object would need to do an http request. Not sure if that can be done in a generic enough way to support any http api (if ApiClient is supposed to be a generic helper). Maybe it's better to let the different plugins check connectivity to the service?
Looking at [this post](https://stackoverflow.com/questions/14661511/setting-up-proxy-for-http-client), looks like there is a generic way to handle proxy calls (via the http.DefaultTransport modification). Perhaps we can leverage that?
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-devlake] likyh closed pull request #3761: Don't check network connection when service is behind a proxy
Posted by GitBox <gi...@apache.org>.
likyh closed pull request #3761: Don't check network connection when service is behind a proxy
URL: https://github.com/apache/incubator-devlake/pull/3761
--
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: commits-unsubscribe@devlake.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org