Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

allow truffle compile behind corporate proxy Issue 4016 #5847

Closed
wants to merge 4 commits into from

Conversation

joseluu
Copy link

@joseluu joseluu commented Jan 19, 2023

PR description

Did the necessary modifications to the axios calls to allow truffle compile behind corporate proxy

Testing instructions

truffle compile should be able to download the required compiler version without or with proxy
test by changing the compiler version in truffle-config.js to a version you have never used as to force the download of a new version.
truffle deploy should work without or with proxy

Documentation

  • [ x] I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

to allow pass through corporate proxies
you now can define the environment variable HTTP_PROXY=http://username:password@proxy:port
notice that HTTP and http:// are used rather than HTTPS and https://
you may also define NODE_TLS_REJECT_UNAUTHORIZED=0 or NODE_EXTRA_CA_CERTS= if the corporate proxy decripts the https flows

Breaking changes and new features

  • [ x] I have added any needed breaking-change and new-feature labels for the appropriate packages.
    The package compile-solidity has changed, but no breaking change

@jhoos70
Copy link

jhoos70 commented Jan 24, 2023

Please merge. I am waiting for this.

@eggplantzzz
Copy link
Contributor

eggplantzzz commented Jan 24, 2023

We'll get this reviewed, thanks for the submission! @joseluu do you have any material you can reference relevant to your fix?

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @joseluu 🙇. I requested some minor changes. Look forward to getting this out in our next release.

@eggplantzzz
Copy link
Contributor

So I just saw that Truffle is not up-to-date on the latest version of axios. Looking at the changelog there is one PR merged (axios/axios#5097) that looks like it is related to this issue. I'm going to put in a PR to bump axios in order to see if that fixes this proxy issue before we put any code in Truffle.

@cds-amal
Copy link
Member

So I just saw that Truffle is not up-to-date on the latest version of axios. Looking at the changelog there is one PR merged (axios/axios#5097) that looks like it is related to this issue. I'm going to put in a PR to bump axios in order to see if that fixes this proxy issue before we put any code in Truffle.

axios:4436 seems relevant as well.

Copy link
Member

@cds-amal cds-amal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick changes @joseluu. While reviewing your PR, we wondered if the underlying issue could be solved by bumping axios to the latest stable version. Truffle is currently at a pre 1.x version.

This means we'd like to test if the version bump resolves the proxy issue before deciding on merging your changes. Would you be able to test it after this week's release?

Comment on lines +19 to +21
} else {
axios = axiosPlain;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most recent rebase undid initializing axios to axiosPlain

Copy link
Author

@joseluu joseluu Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I will be able to test with the next release incorporating the updated axios.

However I couldn't make the "plain test" below work with axios 1.2.5
activating the embedded proxy code in axios 1.2.5 either by environment vars or
by explicit code led to "handshakefailed" errors from the McAfee proxy I am behind
----------- plain test --------

const axios = require('axios');
axios.get('https://api.ipify.org/?format=json')
    .then(res => {
        console.log(res.data)
    }).catch(err => console.error(err))

---------- proxied test using axios ------------

const axios = require('axios');
 
const req = axios({
  url: 'https://api.ipify.org/?format=json',
  proxy: {
    host: 'proxy-host',
    port: 8080,
    auth: {
      username: ‘the-user’,
      password: ‘his-password’,
    },
  },
});

---------- proxied test using the https-proxy-agent module

const axios_plain = require('axios');
var proxy = process.env.https_proxy || process.env.http_proxy;
var axios;
if (proxy) {
    const HttpsProxyAgent = require('https-proxy-agent');
     console.log('using proxy server %j', proxy);
    const agent = new HttpsProxyAgent(proxy)
    axios=axios_plain.create( {
    httpsAgent: agent
        });
} else {
    axios = axios_plain;
}
axios.get('https://api.ipify.org/?format=json',)
    .then(res => {
        console.log(res.data)
    }).catch(err => console.error(err))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @joseluu, did the latest release resolve your proxy issue? I don't have an easy way to test this :/ I wonder if it's related to this TunnelAgent.httpsOverHttp comment in a related Axios issue. 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the v5.7.4 does not solve the issue (see screenshot)
The TunnelAgent comment is another but similar way of explicitly adding an httpsAgent when creating the connection. This solution uses the package tunnel-agent rather than https-proxy-agent.

truffle_V5 7 4_proxy_test

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joseluu! Bummer the dep upgrade didn't resolve the issue. You mentioned there were problems with axios 1.2.4 above, can you confirm your PR still works for your configuration?

This issue has been dogging us for a while, and I'd like to document the various proxy configurations, so we can incorporate proxy testing in CI. Would you be able to describe your setup? Thanks again!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased the patch on v5.7.4, and checked operation behind a firewall: it works

@joseluu joseluu marked this pull request as ready for review February 2, 2023 23:57
@joseluu joseluu requested a review from cds-amal February 2, 2023 23:58
@cds-amal
Copy link
Member

cds-amal commented Feb 6, 2023

@jhoos70, can you verify this PR resolves your issue? We haven't added a test to CI for this yet, and it would be helpful to get another data point. Thanks in advance!

@eggplantzzz
Copy link
Contributor

Hey @joseluu, we haven't yet figured out a way to test this for ourselves so I think we want to wait for at least one or two more confirmations (that this fixes the issue) before we move forward with merging this fix.

@jhoos70
Copy link

jhoos70 commented Feb 15, 2023

We tried this today but still got issues with the solidity compiler not being fetched. We still don't know why but try to debug it.

@Tyler-Franklin
Copy link

come on, we really need this feature!

@eggplantzzz
Copy link
Contributor

@Tyler-Franklin are you able to verify that this PR fixes the issue that it is intended to fix? We are waiting on a couple of community confirmations from folks that are experiencing this proxy problem as we are unable to reproduce the environment necessary to cause this issue.

@Tyler-Franklin
Copy link

@eggplantzzz
ssh username@ip -D 1080 -N -C
code --proxy-server=socks5://127.0.0.1:1080
in vscode terminal :
truffle --network bsc
still no work

@cds-amal
Copy link
Member

cds-amal commented Apr 6, 2023

We don't have enough feedback to determine if this fix is valid. Unless we get that information, I'm afraid we'll have to close this PR.

@cliffoo
Copy link
Contributor

cliffoo commented May 11, 2023

Closing because we can't confirm that this actually fixes the problem. Sorry!

@cliffoo cliffoo closed this May 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants