Skip to content
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

Issue 312: http proxy support #589

Merged
merged 22 commits into from
Dec 2, 2020
Merged

Conversation

OperationalDev
Copy link
Contributor

@OperationalDev OperationalDev commented May 23, 2020

Checklist
  • make test-all (UNIX) passes. CI will also test this
  • unit and/or integration tests are included (if applicable)
  • documentation is changed or added (if applicable)

Description of change

Added proxy support as per #312 . Supports proxy environment variables and setting a proxy per resource. Setting proxy on resource takes precedence over proxy environment variables.

Testing adds tinyproxy and tests connecting to a remote URL through it.

Note: I've disabled arch and wheezy images from tests as the actual containers don't seem to be working. If the images are fixed tinyproxy is available for both.

@aelsabbahy
Copy link
Member

aelsabbahy commented Jun 1, 2020

Eyeball check on the changes look good.

Couple of things are needed before this can be merged:

  1. Update docs with note about proxy from environment change.
  2. Enable all integration tests. What's not working with the containers?

@aelsabbahy aelsabbahy changed the title Issue 312 Issue 312: http proxy support Jun 16, 2020
@OperationalDev
Copy link
Contributor Author

OperationalDev commented Aug 30, 2020

Updated docs.

I managed to get all the containers working except for arch.
For wheezy I had to change apt to use archive.debian.org/debian
For precise I had to do some monkey patching to make status for tinyproxy work.
I can't get arch to work so I disabled it. I tried your original image which didn't work for me. I also tried building my own which didn't work (they don't start complaining /sbin/init does not exist).

Otherwise should be good to go.

@damzog
Copy link

damzog commented Sep 22, 2020

Hi guys, I would really love to see this feature. Any reason it was not merged?

@damzog
Copy link

damzog commented Oct 5, 2020

@aelsabbahy Could you support here? Maybe you can temporarly grant write access to @OperationalDev so he can merge? Can I do anything to support (I have basic knowlege in go)?

@aelsabbahy
Copy link
Member

aelsabbahy commented Oct 5, 2020

Any reason it was not merged?

Yes, the problem with this PR is that it disables the Archlinux integration tests (see makefile changes). I've been prioritizing reviewing and merging other PRs that have a full set of passing tests.

With this PR, one of two things need to happen before it's merged, either:
A) Author and/or other contributor fixes the tests, and I can review/merge.

B) I eventually get around to trying to fix/update the tests for this PR myself. I don't have a timeline on that since I have a lot of other PRs I'm working on at the moment.

"A" Gets this PR merged sooner, I think "B" will eventually happen.. but no set timeline.

@OperationalDev
Copy link
Contributor Author

@aelsabbahy Could you support here? Maybe you can temporarly grant write access to @OperationalDev so he can merge? Can I do anything to support (I have basic knowlege in go)?

@damzog I added tests for the proxy functionality to each of the distros that make up the testing. I did not want to use an external proxy and add dependencies, so I decided to add a proxy (I opted for tinyproxy) to each of the docker images. Unfortunately the docker files are quite old and in some cases no longer valid (Arch/Wheeezy). Also in the case of Precise being end of life, this had it's own challenges with getting tinyproxy to work.

I've did manage to get the Precise/Wheezy images to work, but the Arch image isn't working. I could disable the proxy tests for Arch, but that just felt like the wrong solution here.

Feel free to take a look.

P.S. with Windows/Darwin being added, these would need some sort of proxy as well.

@aelsabbahy
Copy link
Member

Get https://www.microsoft.com: proxyconnect tcp: dial tcp 127.0.0.1:8888: connect: connection refused
Get https://www.microsoft.com: proxyconnect tcp: dial tcp 127.0.0.1:8888: connect: connection refused

https://travis-ci.org/github/aelsabbahy/goss/jobs/729495936#L5553

Seems the precise test is what's broken. The goss add is failing with the error linked and an entry for https://www.microsoft.com is missing from the precise expected.yml file.

@OperationalDev When you get a chance, take a look at these changes. I think they're a step in the right direction.

  • Upgraded precise -> trusty
  • Changed archlinux base image (but not sure how it's relevant here)
  • In goss_wait added a wait for tcp://localhost:8888
  • Added the microsoft.com test w/ proxy to Trusty (this looks like it was always failing, see my link above)

https://github.com/OperationalDev/goss/compare/issue_312...aelsabbahy:issue_312_fixes?expand=1

Once tinyproxy is working correctly in trusty this PR is good to go.

Side/future note: I might refactor the tests moving forward to only test package/service on each OS type and only do the full integration test on one OS.

@aelsabbahy
Copy link
Member

One thing I forgot to mention precise works great locally on my laptop (Fedora), but is acting up in travis.. not sure why yet.. the last commit I added a debug by doing cat /var/log/tinyproxy/tinyproxy.log hopefully that provides some clues.

Related note, created a new ticket #649 to move off of travis-ci. Each build is taking 45m-1hr. That combined with the lack of ability to reproduce this locally is really sucking.

Sorry for the delay in reviewing this.. finally was able to get around to it.

@aelsabbahy
Copy link
Member

Thank you for your contribution. I made some updates to your PR to allow the tests to pass. This should be in the next goss release.

Awesome feature, appreciate you taking the time to work on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants