Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions #170

sgreene570 · 2020-08-11T20:17:31Z

HAProxy backend stanzas cannot have duplicate server lines. Services with multiple port specs that reference the same target port break this rule when a route is created with an integer target port that matches the (duplicate) service target port. Services that meet this criteria should also be counted as 1 (one) endpoint when calculating service endpoint weights and when determining if an HAProxy backend health probe is applicable.

openshift-ci-robot · 2020-08-12T20:27:17Z

@sgreene570: This pull request references Bugzilla bug 1858879, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1858879: Filter duplicate endpoints

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Miciah

Would you mind adding unit tests for calculateServiceWeights and for processEndpointsForAlias?

What if a route specifies two services that have an endpoint in common? (Maybe that is outside the scope of this fix.)

pkg/router/template/template_helper.go

pkg/router/template/router.go

sgreene570 · 2020-08-19T21:24:14Z

Would you mind adding unit tests for calculateServiceWeights and for processEndpointsForAlias?

Sure, I will add unit tests for those 2 functions. Thanks for the suggestion!

What if a route specifies two services that have an endpoint in common? (Maybe that is outside the scope of this fix.)

Are you concerned about the endpoint weights being incorrect or HAProxy breaking in this situation? A route that specifies two services with a common endpoint will not break HAProxy with or without this change, since each service is given a separate backend stanza.

This PR does not change how endpoints common across multiple services will be weighted, since duplicate endpoints are filtered on a per-service basis. Are you suggesting that we change how services with common endpoints weight each of their endpoints? If so, I think that is out of scope of this particular bugfix.

Miciah · 2020-08-20T00:38:23Z

Are you concerned about the endpoint weights being incorrect or HAProxy breaking in this situation?

I meant the latter (HAProxy's breaking if a route has two services with a common endpoint, same as it breaks if a route has one service with two identical endpoints).

A route that specifies two services with a common endpoint will not break HAProxy with or without this change, since each service is given a separate backend stanza.

Are you sure that each service is given a separate backend stanza? A route with two services is translated to a service unit alias with two associated service units:

router/pkg/router/template/router.go

Lines 854 to 856 in 4a6284e

    
           // Get the service weights from each service in the route. Count the active 
        
           // ones (with a non-zero weight) 
        
           serviceUnits := getServiceUnits(route)

router/pkg/router/template/router.go

Lines 1171 to 1177 in 4a6284e

    
           key := endpointsKeyFromParts(route.Namespace, route.Spec.To.Name) 
        
           serviceUnits[key] = getServiceUnitWeight(route.Spec.To.Weight) 
        
           for _, svc := range route.Spec.AlternateBackends { 
        
           	key = endpointsKeyFromParts(route.Namespace, svc.Name) 
        
           	serviceUnits[key] = getServiceUnitWeight(svc.Weight) 
        
           }

The template produces a backend stanza for each service unit alias:

router/images/router/haproxy/conf/haproxy-config.template

Lines 435 to 440 in 4a6284e

    
           {{- range $cfgIdx, $cfg := .State }} 
        
             {{- if matchValues (print $cfg.TLSTermination) "" "edge" "reencrypt" }} 
        
           # Plain http backend or backend with TLS terminated at the edge or a 
        
           # secure backend with re-encryption. 
        
           backend {{genBackendNamePrefix $cfg.TLSTermination}}:{{$cfgIdx}}

And under that backend stanza, the template produces a server stanza for each endpoint of each service unit associated with the service unit alias:

router/images/router/haproxy/conf/haproxy-config.template

Lines 559 to 563 in 4a6284e

    
           {{- range $serviceUnitName, $weight := $cfg.ServiceUnitNames }} 
        
             {{- if ge $weight 0 }}{{/* weight=0 is reasonable to keep existing connections to backends with cookies as we can see the HTTP headers */}} 
        
               {{- with $serviceUnit := index $.ServiceUnits $serviceUnitName }} 
        
                 {{- range $idx, $endpoint := processEndpointsForAlias $cfg $serviceUnit (env "ROUTER_BACKEND_PROCESS_ENDPOINTS" "") }} 
        
           server {{$endpoint.ID}} {{$endpoint.IP}}:{{$endpoint.Port}} cookie {{$endpoint.IdHash}} weight {{$weight}}

Line 559 is iterating over the service unit, and line 562 is iterating over the endpoints of the service unit. So it looks like two services with the same endpoint would produce two server stanzas, right?

That said, in tracing through this code, I realized that $endpoint.ID includes the name of the Kubernetes endpoints object, which means that the endpoint id will be different for two endpoints that come from different services, even if the endpoints are the same:

router/pkg/router/template/plugin.go

Line 325 in 4a6284e

    
           ep.ID = fmt.Sprintf("pod:%s:%s:%s:%d", ep.TargetName, endpoints.Name, a.IP, p.Port)

So this last discovery means that having a route with two services that have a common endpoint should not break anything, as the server name (= endpoint id) will differ.

However, this last discovery also raises a question: Would it be an alternative (and simpler) solution to change the endpoint id to include the port name (p.Name)?

sgreene570 · 2020-08-20T03:48:46Z

A route with two services is translated to a service unit alias with two associated service units

Ah, there's the missing piece that tripped me up. Thanks for the detailed explanation! I will ear mark this PR for future reference 😄

Would it be an alternative (and simpler) solution to change the endpoint id to include the port name (p.Name)?

Yes, absolutely. I prefer this approach more, since endpoints would not need to be filtered out at all, and weight calculation code does not need to be modified. Also, p.Name collisions are not possible, from my understanding. Thank you for this suggestion!

I'll go ahead and make the change to the ep.ID string format, along with add in the unit tests mentioned earlier (not as important with this new approach, but still should be covered from this point forward, so I'll take care of that!).

sgreene570 · 2020-08-21T14:54:49Z

@Miciah commit 763e086 uses your suggested fix and commit 22d67c5 adds the mentioned unit test.
PTAL when you get the chance.

sgreene570 · 2020-08-21T16:44:06Z

/test e2e

sgreene570 · 2020-08-25T14:39:38Z

/retest

sgreene570 · 2020-08-25T18:42:24Z

Unable to connect to the server: dial tcp: lookup api.ci-op-h9sw744l-8f0fe.origin-ci-int-gce.dev.openshift.com on 10.142.0.107:53: no such host

/retest

Miciah · 2020-08-27T22:47:54Z

pkg/router/template/template_helper_test.go

+
+	for _, tc := range testCases {
+		alias.PreferPort = tc.preferPort
+		router.AddEndpoints(suKey, tc.endpoints)


This makes me nervous because router.AddEndpoints assigns the slice to the service unit's endpoints table, and processEndpointsForAlias can mutate the service unit's endpoints table, meaning it can mutate tc.endpoints. This does not break the test now (processEndpointsForAlias does not mutate the endpoints when PreferPort is set or action is empty), but it would be safer to copy the endpoints table to avoid the test data from being modified under us:

Suggested change

router.AddEndpoints(suKey, tc.endpoints)

endpointsCopy := make([]Endpoint, len(tc.endpoints))

for i := range tc.endpoints {

endpointsCopy[i] = tc.endpoints[i]

}

router.AddEndpoints(suKey, endpointsCopy)

Ah, yea, definitely makes sense to copy the endpoints table first. Thanks for catching this!!

openshift-bot · 2020-08-31T16:24:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T16:50:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T18:08:11Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T18:33:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T20:04:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T20:30:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T21:48:48Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T22:27:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-08-31T23:32:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T00:11:53Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T00:51:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T01:29:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T02:34:46Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T02:47:45Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T03:40:25Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T04:22:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-01T04:31:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2020-09-01T04:36:10Z

/hold
while we investigate BZ#1874278.

sgreene570 · 2020-09-03T13:47:59Z

/hold cancel
/retest

sgreene570 · 2020-09-03T15:41:59Z

error: failed to push image
/retest

openshift-bot · 2020-09-03T16:07:47Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-03T16:21:24Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-03T16:33:38Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-09-03T18:25:30Z

@sgreene570: All pull requests linked via external trackers have merged:

openshift/router#170

Bugzilla bug 1858879 has been moved to the MODIFIED state.

In response to this:

Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sgreene570 · 2020-12-09T17:38:41Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-12-09T17:38:44Z

@sgreene570: #170 failed to apply on top of branch "release-4.5":

Applying: pkg/router/template: Use port.Name in endpoint.ID
Using index info to reconstruct a base tree...
M	pkg/router/template/plugin.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/router/template/plugin.go
CONFLICT (content): Merge conflict in pkg/router/template/plugin.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 pkg/router/template: Use port.Name in endpoint.ID
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2020

openshift-ci-robot requested review from Miciah and smarterclayton August 11, 2020 20:17

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2020

sgreene570 force-pushed the bz-1858879 branch 4 times, most recently from 3df7d52 to e954d26 Compare August 12, 2020 19:31

sgreene570 changed the title ~~[WIP] Remove duplicates in createRouterEndpoints~~ Bug 1858879: Filter duplicate endpoints Aug 12, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 12, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Aug 12, 2020

Miciah reviewed Aug 19, 2020

View reviewed changes

pkg/router/template/template_helper.go Outdated Show resolved Hide resolved

pkg/router/template/template_helper.go Outdated Show resolved Hide resolved

pkg/router/template/template_helper.go Outdated Show resolved Hide resolved

pkg/router/template/router.go Outdated Show resolved Hide resolved

pkg/router/template: Use port.Name in endpoint.ID

763e086

sgreene570 force-pushed the bz-1858879 branch 3 times, most recently from fcf1e05 to 22d67c5 Compare August 20, 2020 15:22

sgreene570 changed the title ~~Bug 1858879: Filter duplicate endpoints~~ Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions Aug 20, 2020

sgreene570 changed the title ~~Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions~~ Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions Aug 20, 2020

Miciah reviewed Aug 27, 2020

View reviewed changes

pkg/router/template: Add unit tests

5a5cc82

sgreene570 force-pushed the bz-1858879 branch from 22d67c5 to 5a5cc82 Compare August 28, 2020 13:34

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2020

openshift-merge-robot merged commit 8871d79 into openshift:master Sep 3, 2020

sgreene570 mentioned this pull request Dec 9, 2020

[release-4.5] Bug 1906132: back port: Change router's internal endpoint.ID to prevent HAProxy server line collisions #234

Merged

-		router.AddEndpoints(suKey, tc.endpoints)
+		endpointsCopy := make([]Endpoint, len(tc.endpoints))
+		for i := range tc.endpoints {
+			endpointsCopy[i] = tc.endpoints[i]
+		}
+		router.AddEndpoints(suKey, endpointsCopy)

Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions #170

Bug 1858879: Change router's internal endpoint.ID to prevent HAProxy server line collisions #170

Conversation

sgreene570 commented Aug 11, 2020 • edited

openshift-ci-robot commented Aug 12, 2020

Miciah left a comment

Choose a reason for hiding this comment

sgreene570 commented Aug 19, 2020 • edited

Miciah commented Aug 20, 2020

sgreene570 commented Aug 20, 2020

sgreene570 commented Aug 21, 2020

sgreene570 commented Aug 21, 2020

sgreene570 commented Aug 25, 2020

sgreene570 commented Aug 25, 2020

Miciah Aug 27, 2020

Choose a reason for hiding this comment

sgreene570 Aug 28, 2020

Choose a reason for hiding this comment

sgreene570 Aug 28, 2020

Choose a reason for hiding this comment

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Aug 31, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

openshift-bot commented Sep 1, 2020

Miciah commented Sep 1, 2020

sgreene570 commented Sep 3, 2020

sgreene570 commented Sep 3, 2020

openshift-bot commented Sep 3, 2020

openshift-bot commented Sep 3, 2020

openshift-bot commented Sep 3, 2020

openshift-ci-robot commented Sep 3, 2020

sgreene570 commented Dec 9, 2020

openshift-cherrypick-robot commented Dec 9, 2020

sgreene570 commented Aug 11, 2020 •

edited

sgreene570 commented Aug 19, 2020 •

edited