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 Support for Reject Handler with SmokescreenContext #232

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

saurabhbhatia-stripe
Copy link
Contributor

@saurabhbhatia-stripe saurabhbhatia-stripe commented Oct 3, 2024

Description

Enhance Smokescreen to implement reject handler functionality. This handler will be invoked with the Smokescreen context when a connection failure occurs.

Testing Details

HTTP Connect Failure

HTTPS_PROXY=localhost:4750 curl https://localhost.com
curl: (56) CONNECT tunnel failed, response 504

Logs

{"conn_establish_time_ms":75001,"id":"crvobmqf8fl8sruiooeg","inbound_remote_addr":"[::1]:52672","level":"error","msg":"Timed out connecting to remote host: dial tcp 74.125.224.72:443: connect: operation timed out","project":"","proxy_type":"connect","requested_host":"localhost.com:443","role":"","start_time":"2024-10-04T06:06:19.38513Z","time":"2024-10-04T11:37:34+05:30","trace_id":""}
RejectResponseHandlerWithCtx Called

HTTP Connect Success

 HTTPS_PROXY=localhost:4750 curl https://api.github.com/zen 
Practicality beats purity.%       

Logs

{"allow":true,"decision_reason":"Egress ACL is not configured","dns_lookup_time_ms":60,"enforce_would_deny":false,"id":"crvoipaf8fl8sruioohg","inbound_remote_addr":"[::1]:53414","level":"info","msg":"CANONICAL-PROXY-DECISION","project":"","proxy_type":"connect","requested_host":"api.github.com:443","role":"","start_time":"2024-10-04T06:21:25.653434Z","time":"2024-10-04T11:51:25+05:30","trace_id":""}
{"level":"info","msg":"AcceptResponseHandler","stdlog":"1","time":"2024-10-04T11:51:25+05:30"}
{"bytes_in":4684,"bytes_out":594,"conn_establish_time_ms":97,"duration":0.416362375,"end_time":"2024-10-04T06:21:26.227127Z","error":"","id":"crvoipaf8fl8sruioohg","inbound_remote_addr":"[::1]:53414","last_activity":"2024-10-04T06:21:26.226825Z","level":"info","msg":"CANONICAL-PROXY-CN-CLOSE","outbound_local_addr":"10.246.129.166:53415","outbound_remote_addr":"20.207.73.85:443","project":"","proxy_type":"connect","requested_host":"api.github.com:443","role":"","start_time":"2024-10-04T06:21:25.653434Z","time":"2024-10-04T11:51:26+05:30","trace_id":""}

@coveralls
Copy link

coveralls commented Oct 3, 2024

Pull Request Test Coverage Report for Build 11231455598

Details

  • 11 of 13 (84.62%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 54.539%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/smokescreen/smokescreen.go 6 8 75.0%
Totals Coverage Status
Change from base Build 10911749373: 0.1%
Covered Lines: 1436
Relevant Lines: 2633

💛 - Coveralls

Copy link
Collaborator

@manishsb-stripe manishsb-stripe left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment.

resp, err := client.Get("http://127.0.0.1")
r.NoError(err)

// The RejectResponseHandler should set our custom header
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be RejectResponseHandlerWithCtx? (Same in all places in this test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will update this comment.

Comment on lines 90 to 91
// Custom handler to allow clients to modify reject responses
RejectResponseHandlerWithCtx func(*SmokescreenContext, *http.Response)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we guard against configuring both this handler and the legacy one? If not, we'd need to document ordering of handler execution et all for consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it makes sense to only let users configure one of these handler since new one is the superset of legacy one.
I can mark legacy one as depreciated.

@saurabhbhatia-stripe saurabhbhatia-stripe merged commit f6f8191 into master Oct 8, 2024
9 checks passed
@saurabhbhatia-stripe saurabhbhatia-stripe deleted the saurabhbhatia/add-reject-handler branch October 8, 2024 09:22
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.

4 participants