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

Remove every default header except User-Agent #3644

Merged
merged 8 commits into from
Apr 26, 2019
Merged

Conversation

dabcoder
Copy link
Contributor

@dabcoder dabcoder commented Apr 17, 2019

What does this PR do?

GET requests do not need to include 'Content-Type' in headers (some application firewalls may block them), unless users specifically decide to use custom headers that include such 'Content-Type'.
The goal of this PR is to provide a ('http_method') argument to get different headers depending on the use case (+ ensure backward compatibility).

EDIT:

  • remove all default headers but User-Agent.
  • re-add 'Content-Type' for checks that use other http methods than GET (the http_check only).

Motivation

Customer reported issue.

Review checklist (to be filled by reviewers)

  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • Feature or bugfix must have tests
  • Git history must be clean
  • If PR adds a configuration option, it must be added to the configuration file.

@dabcoder dabcoder requested a review from a team as a code owner April 17, 2019 12:50
@codecov
Copy link

codecov bot commented Apr 17, 2019

Codecov Report

Merging #3644 into master will decrease coverage by 2.39%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #3644      +/-   ##
==========================================
- Coverage   86.26%   83.86%    -2.4%     
==========================================
  Files         739       62     -677     
  Lines       38341     4693   -33648     
  Branches     4606      574    -4032     
==========================================
- Hits        33075     3936   -29139     
+ Misses       4000      636    -3364     
+ Partials     1266      121    -1145

@nmuesch
Copy link
Collaborator

nmuesch commented Apr 17, 2019

Thanks for the PR!
Would it make more sense to just make the default here False -

default_headers = is_affirmative(instance.get("include_default_headers", True))

If the user provided this header, we shouldn't remove it

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

What @nmuesch said 🙂

@dabcoder
Copy link
Contributor Author

Thanks for the PR!
Would it make more sense to just make the default here False -

integrations-core/http_check/datadog_checks/http_check/config.py

Line 38 in 943534a

default_headers = is_affirmative(instance.get("include_default_headers", True))
If the user provided this header, we shouldn't remove it

🤦‍♂️ I didn't even think of that... Good point.

@masci
Copy link
Contributor

masci commented Apr 18, 2019

This does more than what's in the title, for example default headers contain User-Agent, do we add it back somehow?

@ofek ofek changed the title Remove Content-Type for GET requests Make include_default_headers default to false Apr 18, 2019
@dabcoder dabcoder requested a review from a team as a code owner April 23, 2019 14:11
@dabcoder dabcoder changed the title Make include_default_headers default to false Remove Content-Type for GET requests Apr 23, 2019
@dabcoder
Copy link
Contributor Author

Reverting to the original PR title. Editing the headers instead of removing content in individual checks.

Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

@dabcoder We've decided to not ensure backward compatibility. Please remove every default header except User-Agent.

@ofek ofek changed the title Remove Content-Type for GET requests Remove every default header except User-Agent Apr 24, 2019
ofek
ofek previously approved these changes Apr 24, 2019
Copy link
Contributor

@ofek ofek left a comment

Choose a reason for hiding this comment

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

Thanks!

@ofek ofek merged commit 86558ac into master Apr 26, 2019
@ofek ofek deleted the davidb/content-headers branch April 26, 2019 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants