support custom configuration to main context of nginx config #2895

tomxor · 2018-08-03T18:24:54Z

What this PR does / why we need it:
Adding custom configuration to the main context of the nginx config file is currently only possible by providing a custom template. IMO doing so should be considered a last resort since it could make the upgrade process more painful. This PR introduces the main-snippet ConfigMap option which enables addition of custom settings to the main context of the nginx config file.

Release note:

Adds `main-snippet` ConfigMap option which enables adding custom settings to the main section of the nginx config file.

Special notes for your reviewer:
Didn't see any tests for similar functions (e.g. http-snippet). I assume this can be verified in the e2e tests but I wasn't entirely sure if that's the proper place to test it.

codecov-io · 2018-08-03T18:48:36Z

Codecov Report

Merging #2895 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2895   +/-   ##
=======================================
  Coverage   47.56%   47.56%           
=======================================
  Files          76       76           
  Lines        5483     5483           
=======================================
  Hits         2608     2608           
  Misses       2540     2540           
  Partials      335      335

Impacted Files	Coverage Δ
internal/ingress/controller/config/config.go	`98.26% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23ce9b5...1bacf16. Read the comment docs.

antoineco

Looks reasonable to me. Would you mind adding a simple e2e test to make sure custom directives make it to the config?

antoineco · 2018-08-03T20:37:11Z

docs/user-guide/nginx-configuration/configmap.md

+## main-snippet
+
+Adds custom configuration to the main section of the nginx configuration.
+_**default:**_ ""


I think you can remove that default: "" from all *-snippet descriptions (including the ones that are not part of this PR). It is mentioned in the table, and is kind of obvious.

33a284ac0c96899e018af79a39692897b236dec2

tomxor · 2018-08-03T20:50:50Z

I'll add the e2e tests and ping when it's ready, thanks for the quick review.

tomxor · 2018-08-03T21:26:13Z

@antoineco I've added a very naive e2e test: 60a4c3fb454e049515a5dc9550e7897b969963df

similar to other tests, it simply asserts the value is contained within the entire configuration file. obviously this is not ideal and preferably we should assert that it is included inside the main context. I can pollute the template with # main custom section start + end comments as done in the server snippet tests, but I'm not sure that's desirable. so if you want me to add that let me know.
on a related note, maybe it's time to consider adding a parser to the test infra, one that will return a type which allows making more accurate assertions.

once this is approved I will squash the commits.

antoineco

The test you added should do since each test runs in its own isolated namespace. Thanks for that!

antoineco · 2018-08-03T21:29:07Z

test/e2e/settings/main_snippet.go

+		err := f.UpdateNginxConfigMapData(mainSnippet, expectedComment)
+		Expect(err).NotTo(HaveOccurred())
+
+		ing, err := f.EnsureIngress(framework.NewSingleIngress(


Not relevant for that particular test.

87455d7445255f68629b155baf26072113a5086b

antoineco · 2018-08-03T21:34:33Z

Thanks @tomxor 🙌 I'll wait for you to squash these 4 commits and approve the PR.

tomxor · 2018-08-03T21:35:26Z

@antoineco squashed, thanks for the review!

antoineco · 2018-08-03T21:44:51Z

/lgtm

antoineco · 2018-08-03T21:46:21Z

/lgtm cancel
(sorry, missed a tiny detail)

antoineco

Apologizes for the round-trips, I missed that one. Feel free to git commit --amend --no-edit.

antoineco · 2018-08-03T21:46:52Z

test/e2e/settings/main_snippet.go

+	mainSnippet := "main-snippet"
+
+	BeforeEach(func() {
+		err := f.NewEchoDeployment()


Also irrelevant.

no worries, pushed.

antoineco · 2018-08-03T22:18:44Z

/lgtm
Thank you again @tomxor :)

k8s-ci-robot · 2018-08-03T22:18:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: antoineco, tomxor

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [antoineco]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2018

antoineco suggested changes Aug 3, 2018

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 3, 2018

antoineco suggested changes Aug 3, 2018

View reviewed changes

k8s-ci-robot assigned antoineco Aug 3, 2018

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 3, 2018

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 3, 2018

antoineco suggested changes Aug 3, 2018

View reviewed changes

support custom configuration to main context of nginx config

1bacf16

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 3, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 3, 2018

k8s-ci-robot merged commit af7fe6b into kubernetes:master Aug 3, 2018

tomxor deleted the main-ctx-snippet-cfg branch August 3, 2018 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support custom configuration to main context of nginx config #2895

support custom configuration to main context of nginx config #2895

tomxor commented Aug 3, 2018 •

edited

Loading

codecov-io commented Aug 3, 2018 •

edited

Loading

antoineco left a comment

antoineco Aug 3, 2018

tomxor Aug 3, 2018

tomxor commented Aug 3, 2018

tomxor commented Aug 3, 2018 •

edited

Loading

antoineco left a comment

antoineco Aug 3, 2018

tomxor Aug 3, 2018

antoineco commented Aug 3, 2018

tomxor commented Aug 3, 2018

antoineco commented Aug 3, 2018

antoineco commented Aug 3, 2018

antoineco left a comment

antoineco Aug 3, 2018

tomxor Aug 3, 2018

antoineco commented Aug 3, 2018

k8s-ci-robot commented Aug 3, 2018

support custom configuration to main context of nginx config #2895

support custom configuration to main context of nginx config #2895

Conversation

tomxor commented Aug 3, 2018 • edited Loading

codecov-io commented Aug 3, 2018 • edited Loading

Codecov Report

antoineco left a comment

Choose a reason for hiding this comment

antoineco Aug 3, 2018

Choose a reason for hiding this comment

tomxor Aug 3, 2018

Choose a reason for hiding this comment

tomxor commented Aug 3, 2018

tomxor commented Aug 3, 2018 • edited Loading

antoineco left a comment

Choose a reason for hiding this comment

antoineco Aug 3, 2018

Choose a reason for hiding this comment

tomxor Aug 3, 2018

Choose a reason for hiding this comment

antoineco commented Aug 3, 2018

tomxor commented Aug 3, 2018

antoineco commented Aug 3, 2018

antoineco commented Aug 3, 2018

antoineco left a comment

Choose a reason for hiding this comment

antoineco Aug 3, 2018

Choose a reason for hiding this comment

tomxor Aug 3, 2018

Choose a reason for hiding this comment

antoineco commented Aug 3, 2018

k8s-ci-robot commented Aug 3, 2018

tomxor commented Aug 3, 2018 •

edited

Loading

codecov-io commented Aug 3, 2018 •

edited

Loading

tomxor commented Aug 3, 2018 •

edited

Loading