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

Add validation for http header #296

Closed
wants to merge 21 commits into from

Conversation

kmashint
Copy link

@kmashint kmashint commented Oct 8, 2017

Thanks for goss! This adds a validation for for http header "name: value" pairs as below.

http:
  https://www.google.com:
    allow-insecure: false
    no-follow-redirects: false
    timeout: 3000
    status: 200
    header: ["Content-Type: text/html"]
    body: ["google"]

@kmashint
Copy link
Author

kmashint commented Oct 8, 2017

The Travis build failed due to the extra http header attribute, but maybe that's expected when adding a new test attribute.

Copy link
Member

@aelsabbahy aelsabbahy left a comment

Choose a reason for hiding this comment

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

Seems like the tests need to be updated to support the new header feature.

glide.yaml Outdated
- package: github.com/fatih/color
- package: github.com/aelsabbahy/go-ps
version: unix_race_condition
Copy link
Member

Choose a reason for hiding this comment

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

Moving off of this branch will break things. This was added due to the race condition explained here: #267

@kmashint
Copy link
Author

Thanks, will review and update.

@kmashint
Copy link
Author

@aelsabbahy I found the missing test expectations and adjusted.

Copy link
Member

@aelsabbahy aelsabbahy left a comment

Choose a reason for hiding this comment

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

Thanks, I think this PR is pretty close now.

A few notes:

  • Glide.lock is out of sync
  • Can you update the http test in goss-shared.yaml to test that this works

Also, see inline comments.

Thanks for taking the time to add this :)

glide.lock Outdated Show resolved Hide resolved
resource/http.go Show resolved Hide resolved
@kmashint
Copy link
Author

I've adjusted with some more updates. I'm making some newbie mistakes with the go project. It all makes sense when I relate concepts to Java, Python, JS but in this new ecosystem I don't know what I don't know.

@aelsabbahy
Copy link
Member

This is almost ready for merging, just need a test in goss-shared.yaml showing that this works.

@kmashint kmashint force-pushed the validate-http-header branch 3 times, most recently from a020e82 to a6023ff Compare December 3, 2017 19:55
@kmashint
Copy link
Author

kmashint commented Dec 4, 2017

I'm working through some problems trying to enable the test for the http header validation. On a Raspberry Pi / debian linux it's working fine but I can't correlate the error from travis-ci build to what's really causing it. I'll try to reproduce the error locally.

@berney
Copy link
Contributor

berney commented Apr 9, 2018

This would be a very handy feature. Has there been any further progress on getting the test case finished?

@kmashint
Copy link
Author

kmashint commented Apr 10, 2018

I have a test case that works in actual use on Raspberry Pi / debian linux but there's something broken when it runs in the travis integration. I'll try again in the next day or two.

@kmashint
Copy link
Author

kmashint commented Apr 10, 2018

I tested building on Raspberry Pi and it works for its executable. I tested building on Ubuntu on Windows and that works for the amd64 executable there. But something still fails on the CentOS7 test on the Travis integration server. I tried building on a local VirtualBox with CentsOS7 and that worked as well.

@kmashint
Copy link
Author

I've tested locally successfully on CentOS7, Ubuntu, R-Pi and rebased on the latest master but I still get a build error from Travis CI, maybe when it's comparing [[ centos7 == \a\r\c\h ]].
@aelsabbahy , @berney , have you seen this before? Is this something I can fix or is it elsewhere?
It would be good to finish this off.

Using default tag: latest
latest: Pulling from aelsabbahy/goss_centos7
++docker_exec /goss/centos7/goss-linux-amd64 --vars /goss/vars.yaml -g /goss/centos7/goss.yaml validate
++docker exec goss_int_test_centos7_amd64 /goss/centos7/goss-linux-amd64 --vars /goss/vars.yaml -g /goss/centos7/goss.yaml validate
...
Count: 84, Failed: 0, Skipped: 0
+[[ centos7 == \a\r\c\h ]]
+egrep -q 'Count: 83, Failed: 0'
+rv=1
+docker rm -vf 7fcd348d3633254b8ce7ff49886c44aa7c08234ab5ddc66367340dff2fa82c76
7fcd348d3633254b8ce7ff49886c44aa7c08234ab5ddc66367340dff2fa82c76
+exit 1
make: *** [centos7] Error 1
The command "make" exited with 2.
Done. Your build exited with 1.

@berney
Copy link
Contributor

berney commented Apr 20, 2018

I'm travelling at the moment, I will try mid next week if the issue is still outstanding.

@timbirk
Copy link
Contributor

timbirk commented May 27, 2018

@berney I could pick this up if you want?? I feel like I discussed HTTP header validation a long time ago but can't remember.

I think it would be a useful addition. I'd especially like it to be able to validate that a header doesn't exist.

For example, I might want to validate that the no-cache header doesn't exist on my high traffic heavily cached website.

@berney
Copy link
Contributor

berney commented Jan 8, 2019

I have been using a private build with this fix and it is working well. This PR should be able to be merged, the correct number of passed tests should be in the egrep line for the testing.

Keith Mashinter and others added 2 commits February 27, 2019 09:58
Conflicts:
	glide.lock
	glide.yaml
	integration-tests/goss/goss-shared.yaml
	integration-tests/test.sh
@kmashint
Copy link
Author

I've merged from master and update the expected test counts in test.sh but it's now giving an error with archlinux:
manifest for base/archlinux:latest not found

@berney
Copy link
Contributor

berney commented Feb 28, 2019

base/archlinux is deprecated use archlinux/base. See https://hub.docker.com/r/base/archlinux/.

$ docker search archlinux
NAME                                  DESCRIPTION                                     STARS               OFFICIAL            AUTOMATED
base/archlinux                        Deprecated repository, use archlinux/base in���   331                                     [OK]

I don't know why but they seemed to have used base as the owner/project name rather than archlinux. I'm not even sure base is the archlinux project (e.g. from the maintainers or not).

@kmashint
Copy link
Author

kmashint commented Apr 7, 2019

@aelsabbahy @berney I've been trying to debug more strange cloud build errors, this time with DNS tests. They run locally but for some reason fail in the Travis cloud build. Any ideas? I'd like to finally finish this!

Why does this fail?  Tried timeouts but that didn't help.  It works on CentOS 7.4.1708 in VirtualBox.
Using dnslookup and set type=txt with txt._test.dnstest.io does give "Hello DNS" but not as an address per se.
DNS: TXT:txt._test.dnstest.io: resolvable:
Expected
    <bool>: false
to equal
    <bool>: true
DNS: TXT:txt._test.dnstest.io: addrs: skipped
Total Duration: 1.000s
Count: 90, Failed: 1, Skipped: 1'

@kmashint
Copy link
Author

kmashint commented Apr 8, 2019

I did a complete rebuild of my CentOS7 VM and now I'm getting similar errors, but they are even from master, not just my branch, so it's a side effect of underlying platform updates.

Conflicts:
	integration-tests/test.sh
@kmashint
Copy link
Author

kmashint commented May 3, 2019

@berney Phew! Finally all checks passed!

@kmashint
Copy link
Author

kmashint commented May 3, 2019

@aelsabbahy This is up to date again and passing tests after merging away the problematic DNS tests.

@kmashint
Copy link
Author

@aelsabbahy @berney This merged the latest changes again.

@aelsabbahy
Copy link
Member

Awesome, I'll take a look at it this week and merge it.

Many thanks for this, great enhancement!

@aelsabbahy aelsabbahy self-requested a review May 23, 2019 23:52
@kmashint
Copy link
Author

@aelsabbahy @berney Any luck with merging this? It would be good to start using it for some of my work projects where I'm currently falling back to bash+curl to validate http headers.

@kmashint
Copy link
Author

@aelsabbahy @berney Just pinging again for merge. It would be good to start using it for some of my work projects where I'm currently falling back to bash+curl to validate http headers.

@aelsabbahy aelsabbahy mentioned this pull request Nov 23, 2019
@aelsabbahy
Copy link
Member

Github rebasing of PRs is a bit confusing.. Not sure the proper way to do this.. But I ended up just doing a rebase locally and pushing it up as a different PR.

It looks like all the author information is preserved, I want to make sure you're credited for the changes.

Thanks a bunch for this!

kmashint referenced this pull request Nov 25, 2019
* Add validation for http header

* Add http headers: [] to goss-expected tests.

* Add http header test in goss-shared.yaml.

* Update expected test counts.
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.

6 participants