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

hotfix(config) parse headers config option from .kong_env #3419

Closed
wants to merge 1 commit into from

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented Apr 26, 2018

This fixes a bug introduced in 976dd87 (#3300)
where headers config option was being overwritten
as a table. The table is not persisted in the intermediate
config file and hence the config option does not take effect.

This commit also exposes get_running_conf helper function to
read conf from prefix directory in use during the current test.

@hbagdi
Copy link
Member Author

hbagdi commented Apr 26, 2018

@thibaultcha I missed the serialization part in the previous commit and this fix would take care of it. The approach is similar to what we do with admin_listen and proxy_listen options.

If this looks approach looks good to you, then I'll add test for the generated .kong_env file. I couldn't find any integration test for this file, so I'm assuming that it will be additional work. Let me know if that's not the case. Thanks!

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

@hbagdi SGTM

conf.headers = headers_enabled
local mt = { __tostring = function() return "" end }
conf.enabled_headers = enabled_headers
setmetatable(conf.enabled_headers, mt)
Copy link
Member

Choose a reason for hiding this comment

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

This can be made a lot more concise, in the spirit of writing referential code :)

conf.enabled_headers = setmetatable(enabled_headers, {
  __tostring = function() return "" end,
})

@hbagdi hbagdi force-pushed the fix/headers-config branch 3 times, most recently from 07aa247 to 3eb3d0b Compare April 27, 2018 03:18
@hbagdi
Copy link
Member Author

hbagdi commented Apr 27, 2018

@thibaultcha I've updated the PR with a regression test.

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Note: when providing a fix to a behaviour that was not yet shipped in a release, our style guide instructs us to use the hotfix() scope (instead of fix()).

local conf = helpers.get_running_conf()
assert.equal("server_tokens", conf.headers[1])
assert.equal(1, #conf.headers)
end)
Copy link
Member

Choose a reason for hiding this comment

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

imho, those tests are too implementation-specific - only the unit tests and a couple of integration tests are aware of the .kong_env file.

Instead, it might be better to write an integration test here, which will both be more resilient to implementation changes, and explicitly states the issue (which is nice for the future selves who will browse this test suite and modify this trying not to break it).

Such a test could be written if we load the headers value from a configuration file (instead of environment variables). We also try to keep track of regression test with a comment explaining their existence, which will make this test's importance clearer to the reader:

      it("can also be configured via configuration file", function()
        -- a regression test added with https://github.com/Kong/kong/pull/3419
        -- to ensure that the `headers` configuration value can be specified
        -- via config file as well as via environment variables (all
        -- other tests in this file only test the latter).
      end)

You may create a new configuration file under spec/fixtures/headers.conf which contains something like:

headers = server_tokens

And start Kong with:

          assert(helpers.kong_exec("restart -c spec/fixtures/headers.conf", {
            database = strategy,
            proxy_listen = "0.0.0.0:9000, 0.0.0.0:9443 ssl", -- a bit ugly but already done in `core_entities_invalidations_spec.lua`
          }))

Another pattern could be to duplicate spec/kong_tests.conf and append a headers = directive.

This approach also has the benefit of not needed to expose get_running_conf(). I am also sceptical on testing the same thing 3 times, since a breaking change in the code will trigger 3 test failures instead of a clearly defined and documented one. This philosophy behind our tests are made our lives easier at times (and harder when it wasn't followed we well...).

@hbagdi hbagdi force-pushed the fix/headers-config branch from 3eb3d0b to 589a8f3 Compare April 30, 2018 23:18
@hbagdi hbagdi changed the title fix(config) parse headers config option from .kong_env hotfix(config) parse headers config option from .kong_env May 1, 2018
This fixes a bug introduced in 976dd87 (#3300)
where headers config option was being overwritten
as a table. The table is not persisted in the intermediate
config file and hence the config option does not take effect.
@hbagdi hbagdi force-pushed the fix/headers-config branch from 19ec907 to e0b544f Compare May 1, 2018 02:30
thibaultcha pushed a commit that referenced this pull request May 3, 2018
This fixes a bug introduced in 976dd87 (#3300) where the `headers`
config option were not properly written to `.kong_env`. This would be an
issue when the value was specified from the configuration file (vs. via
environment variables).

From #3419

Signed-off-by: Thibault Charbonnier <[email protected]>
@thibaultcha
Copy link
Member

Merged, thanks!

@thibaultcha thibaultcha closed this May 3, 2018
@thibaultcha thibaultcha deleted the fix/headers-config branch May 3, 2018 01:12
hbagdi added a commit that referenced this pull request May 3, 2018
…atus header

The hotfix commit 75e071b (#3419) changes the configuration property from
`headers` to `enabled_headers`.
The commit did not update a conditional statement introduced in
60c335e (#3263). This commit corrects the check from `headers` to
`enabled_headers`.

This was caught by the test suite and hence no regression test
has been added.
thibaultcha pushed a commit that referenced this pull request May 3, 2018
The hotfix commit 75e071b (#3419) changes the configuration property from
`headers` to `enabled_headers`.

The commit did not update a conditional statement introduced in
60c335e (#3263). This commit corrects the check from `headers` to
`enabled_headers`.

This was caught by the test suite and hence no regression test
has been added.

From #3433
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.

2 participants