Clean JSON before post request to update configuration #2365

aledbf · 2018-04-17T13:24:53Z

What this PR does / why we need it:

Remove Target object reference from endpoints (this is not used for the dynamic configuration)

ElvinEfendi · 2018-04-17T13:52:22Z

internal/ingress/controller/nginx.go

+	copyOfPcfg := &ingress.Configuration{}
+
+	mergo.Merge(copyOfRunningConfig, n.runningConfig)
+	mergo.Merge(copyOfPcfg, pcfg)


is this safer? I'm curious why you switched to using mergo.

codecov-io · 2018-04-24T19:36:14Z

Codecov Report

Merging #2365 into master will increase coverage by 0.28%.
The diff coverage is 55.55%.

@@            Coverage Diff             @@
##           master    #2365      +/-   ##
==========================================
+ Coverage   40.91%   41.19%   +0.28%     
==========================================
  Files          74       74              
  Lines        5248     5263      +15     
==========================================
+ Hits         2147     2168      +21     
+ Misses       2809     2798      -11     
- Partials      292      297       +5

Impacted Files	Coverage Δ
internal/ingress/controller/controller.go	`2.23% <0%> (-0.02%)`	⬇️
internal/ingress/controller/nginx.go	`9.76% <100%> (+4.65%)`	⬆️
internal/file/filesystem.go	`40.98% <0%> (-0.4%)`	⬇️

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 bad526b...c6728aa. Read the comment docs.

ElvinEfendi · 2018-04-24T21:42:32Z

internal/ingress/controller/nginx.go

 // that is handled by Lua
-func (n *NGINXController) ConfigureDynamically(pcfg *ingress.Configuration) error {


why are you changing this?

to be able to test it without creating a new NGINXController instance.
Also removes an export we should not expose.

to be able to test it without creating a new NGINXController instance.

Is this a big deal though? I'm already doing it at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165L55 to test the other function.

Also removes an export we should not expose.

👍 for not exporting this function

Is this a big deal though? I'

I just want to make sure we don't have targets in the endpoints. This is the biggest object in the json request

I understand that, I was commenting on "to be able to test it without creating a new NGINXController instance." - it is a few lines of code to create a new NGINXController instance in test.

ElvinEfendi · 2018-04-24T22:33:58Z

internal/ingress/controller/nginx.go

@@ -779,9 +787,8 @@ func (n *NGINXController) ConfigureDynamically(pcfg *ingress.Configuration) erro
 		return err
 	}

-	glog.V(2).Infof("posting backends configuration: %s", buf)
+	glog.Infof("posting backends configuration: %s", buf)


This is gonna spam the logs with not so valuable information. It is important to see what's being posted only during debugging.

What do you think we should alway log without escalated logging level?

change reverted

ElvinEfendi · 2018-04-24T22:35:29Z

internal/ingress/controller/nginx.go

+	copyOfPcfg := &ingress.Configuration{}
+
+	mergo.MergeWithOverwrite(copyOfRunningConfig, n.runningConfig)
+	mergo.MergeWithOverwrite(copyOfPcfg, pcfg)


is this safer? what was the issue with existing implementation?

what was the issue with existing implementation?

No issue. We already use mergo for this kind of tasks

ElvinEfendi · 2018-04-24T22:40:15Z

internal/ingress/controller/nginx_test.go

+	err := configureDynamically(commonConfig, ts.URL)
+	if err != nil {
+		t.Errorf("unexpected error posting dynamic configuration: %v", err)
+	}


can you also assert that Target field has not changed for the endpoints defined at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165R101?

ElvinEfendi · 2018-04-24T22:41:26Z

internal/ingress/controller/nginx_test.go

+			t.Fatal(err)
+		}
+		body := string(b)
+		if strings.Index(body, "target") != -1 {


can we also assert that "service" does not exist?

ElvinEfendi · 2018-04-25T01:32:12Z

internal/ingress/controller/nginx_test.go

+			t.Errorf("unexpected target reference in JSON content: %v", body)
+		}
+
+		if strings.Index(body, "service") != -1 {


Could you include a dummy service object for the backend at https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165R99 as well? so that this assertion becomes more meaningful

ElvinEfendi · 2018-04-25T01:35:00Z

internal/ingress/controller/nginx_test.go

+		t.Errorf("unexpected error posting dynamic configuration: %v", err)
+	}
+	t.Logf("%v", commonConfig.Backends[0].Endpoints[0])
+	if commonConfig.Backends[0].Endpoints[0].Target == nil {


What about comparing this with its expected value &apiv1.ObjectReference{}(from https://github.com/kubernetes/ingress-nginx/pull/2365/files#diff-b43320e0cdcbbb75b01313d88f637165R105) instead? IMHO that would be a complete check since it covers the case where target is being changed to something other than nil

ElvinEfendi · 2018-04-25T01:38:31Z

internal/ingress/controller/controller.go

-			err := n.ConfigureDynamically(&pcfg)
+
+			url := fmt.Sprintf("http://localhost:%d/configuration/backends", n.cfg.ListenPorts.Status)
+			err := configureDynamically(&pcfg, url)


I realize it makes testing easier but to me this feels counterintuitive for configureDynamically to not know about the endpoint URL itself and get it from a caller.

ElvinEfendi · 2018-04-25T01:43:30Z

I think there's still some flakiness at https://github.com/kubernetes/ingress-nginx/blob/master/test/e2e/lua/dynamic_configuration.go#L61, maybe bump it up to 5s in this PR?

ElvinEfendi · 2018-04-26T18:26:43Z

internal/ingress/resolver/main.go

+	}
+	if asslc1 == nil || assl2 == nil {
+		return false
+	}


Is this a no-op change or addressing an actual issue?

addressing an actual issue?

Comparing a valid instance against null.

ElvinEfendi

Thanks! Loving to see more and more test coverage 😍

ElvinEfendi · 2018-04-26T18:34:41Z

/lgtm

k8s-ci-robot · 2018-04-26T18:34:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, ElvinEfendi

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~~ [aledbf]

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

fw42 · 2024-07-20T19:07:31Z

I just ran into a situation where it would be very useful to have access to endpoint.target in Lua (the pod name in particular). Was about to create a PR to add it when I saw this PR which explicitly removed it.

What was the reason for removing it?

Any opinions on adding it back? Maybe in some kind of opt-in way.

@ElvinEfendi @aledbf

ElvinEfendi · 2024-07-22T14:11:31Z

IIRC the only reason was to avoid unnecessarily polluting NGINX Lua shared memory space used to store the backend configuration. I see no issue with extracting pod name from target and including it as a metadata in the endpoints objects.

That being said, neither Manuel nor me are maintainers anymore and the current team has decided to deprecate Lua and therefore does not seem to be accepting changes touching to Lua side: #11381 (comment) (I guess you can still make the PR as it likely does not dirtectly touch to Lua code anyway).

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 17, 2018

ElvinEfendi reviewed Apr 17, 2018

View reviewed changes

aledbf force-pushed the clean-json branch 2 times, most recently from dc43d30 to 8e3eab6 Compare April 24, 2018 19:16

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 Apr 24, 2018

aledbf assigned ElvinEfendi Apr 24, 2018

ElvinEfendi reviewed Apr 24, 2018

View reviewed changes

aledbf force-pushed the clean-json branch from 8e3eab6 to 171d91e Compare April 25, 2018 00:27

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2018

ElvinEfendi reviewed Apr 25, 2018

View reviewed changes

aledbf force-pushed the clean-json branch 8 times, most recently from 41cb034 to 5945b58 Compare April 26, 2018 18:14

ElvinEfendi reviewed Apr 26, 2018

View reviewed changes

Clean JSON before post request to update configuration

c6728aa

aledbf force-pushed the clean-json branch from 5945b58 to c6728aa Compare April 26, 2018 18:27

ElvinEfendi approved these changes Apr 26, 2018

View reviewed changes

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

k8s-ci-robot merged commit 0a58b1f into kubernetes:master Apr 26, 2018

aledbf deleted the clean-json branch April 26, 2018 23:57

fw42 mentioned this pull request Jul 29, 2024

Add include-endpoint-target-refs option #11703

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean JSON before post request to update configuration #2365

Clean JSON before post request to update configuration #2365

aledbf commented Apr 17, 2018

ElvinEfendi Apr 17, 2018

codecov-io commented Apr 24, 2018 •

edited

Loading

ElvinEfendi Apr 24, 2018

aledbf Apr 24, 2018

ElvinEfendi Apr 24, 2018

aledbf Apr 25, 2018

ElvinEfendi Apr 25, 2018

ElvinEfendi Apr 24, 2018

aledbf Apr 25, 2018

ElvinEfendi Apr 24, 2018

aledbf Apr 25, 2018

ElvinEfendi Apr 24, 2018

aledbf Apr 25, 2018

ElvinEfendi Apr 24, 2018

aledbf Apr 25, 2018

ElvinEfendi Apr 25, 2018

aledbf Apr 26, 2018

ElvinEfendi Apr 25, 2018

ElvinEfendi Apr 25, 2018 •

edited

Loading

ElvinEfendi commented Apr 25, 2018

ElvinEfendi Apr 26, 2018

aledbf Apr 26, 2018

ElvinEfendi left a comment

ElvinEfendi commented Apr 26, 2018

k8s-ci-robot commented Apr 26, 2018

fw42 commented Jul 20, 2024

ElvinEfendi commented Jul 22, 2024

		// that is handled by Lua
		func (n NGINXController) ConfigureDynamically(pcfg ingress.Configuration) error {

Clean JSON before post request to update configuration #2365

Clean JSON before post request to update configuration #2365

Conversation

aledbf commented Apr 17, 2018

Choose a reason for hiding this comment

codecov-io commented Apr 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi left a comment

Choose a reason for hiding this comment

ElvinEfendi commented Apr 26, 2018

k8s-ci-robot commented Apr 26, 2018

fw42 commented Jul 20, 2024

ElvinEfendi commented Jul 22, 2024

codecov-io commented Apr 24, 2018 •

edited

Loading

ElvinEfendi Apr 25, 2018 •

edited

Loading