(PUP-9566) Include user-specified headers in http requests by melissa · Pull Request #7986 · puppetlabs/puppet

melissa · 2020-02-19T18:10:12Z

When we submit a request to the server, we now have the ability to add
in additional arbitrary headers. This should be isolated only to
requests made to a puppetserver, and these headers should not be
included in a request to an arbitrary external host.

These headers can be set with the :http_extra_headers setting in
Puppet. As illustrated in the various tests, this setting can be defined
in the following four forms:

string:
'header1:value,header2:value'
array:
['header1:value', 'header2:value']
array of arrays:
[['header1', 'value'], ['header2', 'value']]
hash:
{'header1' => 'value', 'header2' => 'value'}.

This commit also updates tests for the different services. It bundles
the test for the puppet profile header with the test for these custom
headers. These are both set in the same method, so a single test for
both makes sense. I also added the test for each different type of http
connection for the different services, to ensure consistent behavior
across all of them.

lib/puppet/http/service.rb

puppetcla · 2020-02-19T23:00:17Z

CLA signed by all contributors.

spec/unit/http/service/report_spec.rb

melissa · 2020-02-20T22:50:36Z

I realized we're not testing to ensure each different http request is including puppet headers, so I went a little overboard with the tests. I'm going to scale it back, but I figured I should at least let y'all know why this was taking so long

joshcooper · 2020-02-21T19:33:10Z

jenkins please test with servertests

joshcooper · 2020-02-21T21:23:59Z

jenkins please test with servertests

joshcooper · 2020-02-21T21:56:18Z

jenkins please test this with servertests

spec/unit/http/service/report_spec.rb

melissa · 2020-02-22T05:58:56Z

lib/puppet/settings/http_extra_headers_setting.rb

+    :http_extra_headers
+  end
+
+  def munge(headers)


I don't know how much we care about this, but this leaves open the possibility of a user not specifying headers correctly and not raising any errors.

For example, a user could supply a header, but no value that we can find. This will raise an error farther down the line

irb(main):016:0> [["string", "derp"], ["sadf"]].each do |name, value| irb(main):017:1* puts name irb(main):018:1> puts value irb(main):019:1> headers[name] = value irb(main):020:1> end string derp TypeError: no implicit conversion of String into Integer

but it doesn't seem like a useful error at all.

irb(main):008:0> subject.munge('string') => [["string"]] irb(main):009:0> subject.munge('string:derp') => [["string", "derp"]] irb(main):010:0> subject.munge('string:derp,sadf:fasd') => [["string", "derp"], ["sadf", "fasd"]] irb(main):011:0> subject.munge('string:derp,sadf') => [["string", "derp"], ["sadf"]]

Do we care? Should I make this better?

It would be good to ensure every header has a non-empty value

I'm validating this when we add custom headers to the headers hash rather than here

melissa · 2020-02-22T18:21:30Z

spec/unit/settings/http_extra_headers_spec.rb

+    end
+
+    #TODO should we accept this? should we raise an error?
+    describe 'when given headers with no value'


Open question on how we want to handle this situation

According to https://tools.ietf.org/html/rfc7230#section-3.2

header-field = field-name ":" OWS field-value OWS field-name = token field-value = *( field-content / obs-fold ) field-content = field-vchar [ 1*( SP / HTAB ) field-vchar ] field-vchar = VCHAR / obs-text

So each field-value consists of one or more VCHAR (visible US-ASCII), so this case should be an error.

joshcooper · 2020-02-24T18:12:19Z

lib/puppet/http/service.rb

+
+    # Add additional user-defined headers if they are defined
+    Puppet[:http_extra_headers].each do |name, value|
+      if modified_headers.key?(name)


Since http header names are case-insensitive, you'll need to do something like the following. The number of extra headers should be fairly low, so I'm not too concerned about O(n^2)

modifies_headers.keys.find { |key| key.casecmp(name) == 0 }

I would love to add a test for add_puppet_headers tospec/unit/http/service_spec.rb, but I can't since it's a protected method. Does the benefit of having it protected outweigh being able to test the method directly? I'm mostly being lazy and not wanting to duplicate functionality tests everywhere. I'm adding a check here to raise if we have a header with an empty value

In that case I'd define a test service subclass like class TestService < Puppet::HTTP::Service and define a method headers that calls the protected add_puppet_headers. Then in your test call TestService#headers and verify it parses/rejects headers as expected. That way you can everything in one place, instead of once per service.

in spec/unit/http/service_spec.rb or a different file?

yeah in service_spec.rb. We do something similar in session_spec.rb when creating the DummyResolver.

melissa · 2020-02-25T22:57:32Z

lib/puppet/http/service.rb

+        Puppet.warning(_('Ignoring extra header "%{name}" as it was previously set.') % { name: name })
+      else
+        if value.nil? || value.empty?
+          Puppet.warning(_('Ignoring extra header "%{name}" as it has no value.') % { name: name })


@joshcooper I decided rather than fail, a warning in the logs should be sufficient. I'm happy to make this a proper failure if you think it should be though

joshcooper

Looks great! Travis is complaining about an unused response variable in https://github.com/puppetlabs/puppet/pull/7986/files#diff-972df990c2e008610ec5c7a5edcfebdbR14. Feel free to update and merge after CI passes

When we submit a request to the server, we now have the ability to add in additional arbitrary headers. This should be isolated only to requests made to a puppetserver, and these headers should not be included in a request to an arbitrary external host. These headers can be set with the `:http_extra_headers` setting in Puppet. As illustrated in the various tests, this setting can be defined in the following four forms: - string: `'header1:value,header2:value'` - array: `['header1:value', 'header2:value']` - array of arrays: `[['header1', 'value'], ['header2', 'value']]` - hash: `{'header1' => 'value', 'header2' => 'value'}`. This commit also updates tests for the different services. It bundles the test for the puppet profile header with the test for these custom headers. These are both set in the same method, so a single test for both makes sense. I also added the test for each different type of http connection for the different services, to ensure consistent behavior across all of them.

melissa requested review from a team February 19, 2020 18:10

melissa added Coremunity labels Feb 19, 2020

melissa commented Feb 19, 2020

View reviewed changes

lib/puppet/http/service.rb Outdated Show resolved Hide resolved

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from 8a9e637 to 02df367 Compare February 19, 2020 23:58

melissa commented Feb 19, 2020

View reviewed changes

spec/unit/http/service/report_spec.rb Outdated Show resolved Hide resolved

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from 02df367 to 6fc12a0 Compare February 20, 2020 22:49

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from 6fc12a0 to daba317 Compare February 21, 2020 01:27

melissa changed the title ~~(PUP-9566) Allow extra headers~~ (PUP-9566) Include user-specified headers in http requests Feb 21, 2020

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from daba317 to 57b887b Compare February 21, 2020 01:38

melissa removed the WIP label Feb 21, 2020

melissa mentioned this pull request Feb 21, 2020

(PUP-9566) Add extra_headers setting #7430

Closed

joshcooper approved these changes Feb 21, 2020

View reviewed changes

melissa commented Feb 21, 2020

View reviewed changes

spec/unit/http/service/report_spec.rb Outdated Show resolved Hide resolved

melissa commented Feb 22, 2020

View reviewed changes

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch 4 times, most recently from 49b7f9f to 33e1a6f Compare February 22, 2020 18:21

melissa commented Feb 22, 2020

View reviewed changes

joshcooper reviewed Feb 24, 2020

View reviewed changes

joshcooper self-requested a review February 25, 2020 21:31

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch 2 times, most recently from 21a56fe to 5c58a83 Compare February 25, 2020 22:56

melissa commented Feb 25, 2020

View reviewed changes

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from 5c58a83 to 12918f7 Compare February 25, 2020 22:59

joshcooper approved these changes Feb 25, 2020

View reviewed changes

melissa force-pushed the ticket/master/pup-9566-extra-headers-http-requests branch from 12918f7 to 4dbf4d5 Compare February 25, 2020 23:35

melissa merged commit 15b59fb into puppetlabs:master Feb 26, 2020

melissa deleted the ticket/master/pup-9566-extra-headers-http-requests branch March 25, 2020 18:25

Conversation

melissa commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

puppetcla commented Feb 19, 2020

Uh oh!

Uh oh!

melissa commented Feb 20, 2020

Uh oh!

joshcooper commented Feb 21, 2020

Uh oh!

joshcooper commented Feb 21, 2020

Uh oh!

joshcooper commented Feb 21, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshcooper left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

melissa commented Feb 19, 2020 •

edited

Loading