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 no_proxy option #549

Merged
merged 2 commits into from Jul 16, 2018

Conversation

@stonith
Copy link
Contributor

commented Jul 13, 2018

TL:DR Adds no_proxy option

The behaviour in agent v6 seems to behave differently than v5 when a web proxy is configured. With v5, the agent would connect directly to 127.0.0.1 whereas with v6, the agent will try to connect to 127.0.0.1 via the proxy. In v6, there is a no_proxy option which is not supported the chef datadog cookbook.

@stonith stonith force-pushed the Shopify:add_support_no_proxy branch from b744e8e to 66382dc Jul 13, 2018
@irabinovitch irabinovitch requested a review from olivielpeau Jul 14, 2018
Copy link
Member

left a comment

Thanks @stonith for the PR! I've left a few comments, could you have a look?

@@ -296,6 +296,7 @@
default['datadog']['web_proxy']['user'] = nil
default['datadog']['web_proxy']['password'] = nil
default['datadog']['web_proxy']['skip_ssl_validation'] = nil # accepted values 'yes' or 'no'
default['datadog']['web_proxy']['no_proxy'] = nil

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 16, 2018

Member

Could you add a small comment here to mention it's used for Agent v6+ only?

@@ -37,6 +37,7 @@ if node['datadog']['web_proxy']['host']
user = node['datadog']['web_proxy']['user']
password = node['datadog']['web_proxy']['password']
scheme = ""
no_proxy = node['datadog']['web_proxy']['no_proxy']

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 16, 2018

Member

can you initialize no_proxy to nil outside of this if block

@@ -136,6 +137,10 @@ if !http_proxy.nil?
}
end
if !no_proxy.nil?
agent_config['proxy']['no_proxy'] = no_proxy

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Jul 16, 2018

Member

agent_config['proxy'] should be initialized to a hash here, but the logic is a bit brittle. Could you either check that the key is indeed present in agent_config or move this block inside the !http_proxy.nil? conditional above?

Peer Allan
- adds comment to the attributes to identify it as v6+
- ensures no_proxy variable has as default value
- ensures the proxy key exists in the config:
@pallan

This comment has been minimized.

Copy link

commented Jul 16, 2018

@olivielpeau comments addressed. Thanks!

Copy link
Member

left a comment

LGTM, thanks!

@olivielpeau olivielpeau added this to the 2.16.1 milestone Jul 16, 2018
@olivielpeau olivielpeau merged commit f6f6575 into DataDog:master Jul 16, 2018
2 checks passed
2 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.