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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Start adding test for listeners #2379

wants to merge 7 commits into
base: master


2 participants
Copy link

commented Jul 11, 2019


This begins the process of adding better testing around our server listeners.

Reviewer Notes

Is there anything you would like reviewers to give additional scrutiny?


Add any steps or code to run in this section to help others prepare to run your code:

echo "Code goes here"

Code Review Verification Steps

  • Code follows the guidelines for Logging
  • The requirements listed in
    Querying the Database Safely
    have been satisfied.
  • Any new migrations/schema changes:
    • Follow our guidelines for zero-downtime deploys (see Zero-Downtime Deploys)
    • Have been communicated to #dp3-engineering
    • Secure migrations have been tested using scripts/run-prod-migrations
  • There are no aXe warnings for UI.
  • This works in Supported Browsers and their phone views (Chrome, Firefox, IE, Edge).
  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • User facing changes have been reviewed by design.
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?



If this PR makes visible UI changes, an image of the finished UI can help reviewers and casual
observers understand the context of the changes. A before image is optional and
can be included at the submitter's discretion.

Consider using an animated image to show an entire workflow instead of using multiple images. You may want to use GIPHY CAPTURE for this! 馃摳

Please frame screenshots to show enough useful context but also highlight the affected regions.

@chrisgilmerproj chrisgilmerproj self-assigned this Jul 11, 2019


This comment has been minimized.

Copy link

commented Jul 11, 2019

Codecov Report

Merging #2379 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #2379     +/-   ##
+ Coverage    59.7%   59.7%   +0.1%     
  Files         267     267             
  Lines       14883   14883             
+ Hits         8881    8885      +4     
+ Misses       4970    4965      -5     
- Partials     1032    1033      +1
Impacted Files Coverage 螖
pkg/server/server.go 78.1% <0%> (+9.8%) 猬嗭笍

@chrisgilmerproj chrisgilmerproj marked this pull request as ready for review Jul 11, 2019


This comment has been minimized.

Copy link

commented Jul 11, 2019

Can you dump the TLS information from the request within the handler to a variable to confirm in some way that it did a mutual TLS handshake?

@chrisgilmerproj chrisgilmerproj force-pushed the cg_164441057_test_listeners branch from 23b9d51 to a64596a Jul 12, 2019

suite.Equal("localhost", connState.ServerName)
suite.Equal("Snake Oil", connState.PeerCertificates[0].Subject.Organization[0])
suite.Equal("Snake Oil", connState.VerifiedChains[0][0].Subject.Organization[0])

This comment has been minimized.

Copy link

chrisgilmerproj Jul 12, 2019

Author Contributor

@pjdufour-truss - here's where I'm testing the TLS connection inside the handler. What do you think? Is this a good test?


This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

Need a test that verifies that a request is blocked if the certificate or cert chain is invalid. If I drop the cert pool would the orders api still let it through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.