Fix filter namespaces in template router #15916

pravisankar · 2017-08-23T01:56:23Z

Colon instead of underscore is used to uniquely identify ServiceAliasConfig(route for a service).

pravisankar · 2017-08-23T01:58:41Z

@knobunc @rajatchopra @imcsk8 PTAL

pravisankar · 2017-08-23T01:58:46Z

[test]

smarterclayton · 2017-08-23T02:08:27Z

Do we have a test for this?

imcsk8 · 2017-08-23T03:30:08Z

LGTM

pravisankar · 2017-08-23T17:59:32Z

On Tue, Aug 22, 2017 at 7:08 PM, Clayton Coleman ***@***.***> wrote: Do we have a test for this?

I will add a test case.

…

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15916 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABM0hj5EIMymobfYOsxNLuH8-mnudD_Rks5sa4megaJpZM4O_ZJe> .

pravisankar · 2017-08-24T23:10:07Z

Updates:

Removed endpoints and route key format assumptions in the template plugin and router code
Added test cases for router FilterNamespaces()

@knobunc @rajatchopra @imcsk8 @smarterclayton PTAL

pravisankar · 2017-08-24T23:10:22Z

/retest

pravisankar · 2017-08-28T19:01:01Z

/retest

pravisankar · 2017-08-28T21:57:00Z

/test extended_templates extended_conformance_gce

rajatchopra

/lgtm

Squash the commits maybe?

Points to be addressed in a subsequent PR:

We should not need to parse the ServiceUnit key anywhere. We should have the Namespace and unmodified Name as the fields, and the key be generated one way.
Also the delimiter should be made programmable (useful for nginx e.g.)

openshift-bot · 2017-08-29T01:51:39Z

/retest

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

openshift-bot · 2017-08-29T09:55:39Z

/retest

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

knobunc

Generally good. I just have a bunch of minor questions. Thanks

knobunc · 2017-08-29T13:43:49Z

pkg/router/template/plugin.go

@@ -19,6 +20,10 @@ import (
 	unidlingapi "github.com/openshift/origin/pkg/unidling/api"
 )

+const (


Can we have a comment that says what this is used for please

knobunc · 2017-08-29T13:45:36Z

pkg/router/template/plugin.go

@@ -197,14 +202,28 @@ func (p *TemplatePlugin) Commit() error {

 // endpointsKey returns the internal router key to use for the given Endpoints.
 func endpointsKey(endpoints *kapi.Endpoints) string {
-	return fmt.Sprintf("%s/%s", endpoints.Namespace, endpoints.Name)
+	return fmt.Sprintf("%s%s%s", endpoints.Namespace, endpointsKeySeparator, endpoints.Name)


Should this just call endpointsKeyFromParts?

Yes, I added endpointsKeyFromParts later out of necessity and didn't release it can be reused here.

knobunc · 2017-08-29T13:45:53Z

pkg/router/template/plugin.go

 }

 // peerServiceKey may be used by the underlying router when handling endpoints to identify
 // endpoints that belong to its peers.  THIS MUST FOLLOW THE KEY STRATEGY OF endpointsKey.  It
 // receives a NamespacedName that is created from the service that is added by the oadm command
 func peerEndpointsKey(namespacedName ktypes.NamespacedName) string {
-	return fmt.Sprintf("%s/%s", namespacedName.Namespace, namespacedName.Name)
+	return fmt.Sprintf("%s%s%s", namespacedName.Namespace, endpointsKeySeparator, namespacedName.Name)


This too... and then there's no risk of divergence

knobunc · 2017-08-29T13:47:15Z

pkg/router/template/plugin_test.go

-	return route.Spec.Host + "-" + route.Spec.Path
+// getKey create an identifier for the route consisting of host-path
+func getKey(route *routeapi.Route) string {
+	return route.Spec.Host + routeKeySeparator + route.Spec.Path


Any idea why this doesn't use sprintf? Or why the other ones don't do it this way?

And should this call routeKeyFromParts?

fmt.Sprintf and ' + ' are used in several places and looks like we don't care which one we use.
Yes, we could call routeKeyFromParts

knobunc · 2017-08-29T13:49:21Z

pkg/router/template/router.go

 	routeFile       = "routes.json"
 	certDir         = "certs"
 	caCertDir       = "cacerts"
 	defaultCertName = "default"

 	caCertPostfix   = "_ca"
 	destCertPostfix = "_pod"
+
+	// '-' is not used because namespace can contain dashes
+	// '_' is not used as this could be part of the name in the future


Should we say why / can't be used? The comment below implies that / can't be used because it needs to be written to disk.

ok, I will mention about '/'

Colon instead of underscore is used to uniquely identify ServiceAliasConfig(route for a service).

…gin and router code Get the endpoints and route key related information from one place, this will be less prone to errors and easy to modify in the future if needed.

pravisankar · 2017-08-29T16:35:20Z

Commits are broken down to differentiate between actual bug fix and refactor work.
Only the essential bug fix commit will be back ported to 3.6

pravisankar · 2017-08-29T16:35:48Z

@knobunc @rajatchopra Updated, PTAL

openshift-ci-robot · 2017-08-29T17:24:10Z

@pravisankar: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_templates	`82899ce`	link	`/test extended_templates`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

knobunc

Great! Thanks.
/lgtm

openshift-merge-robot · 2017-08-29T17:48:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc, pravisankar, rajatchopra

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/router/OWNERS~~ [knobunc,rajatchopra]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-08-30T00:02:40Z

/retest

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

openshift-merge-robot · 2017-08-30T01:37:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-30T02:50:43Z

Automatic merge from submit-queue (batch tested with PRs 15853, 15916, 16017, 16027, 16043)

openshift-merge-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 23, 2017

openshift-merge-robot assigned imcsk8 and rajatchopra Aug 23, 2017

pravisankar added the component/routing label Aug 23, 2017

pravisankar force-pushed the router-filter branch from c2c7b2f to 308d1d3 Compare August 24, 2017 22:45

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 24, 2017

pravisankar force-pushed the router-filter branch from 308d1d3 to 2e7ec8f Compare August 24, 2017 22:48

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2017

pravisankar force-pushed the router-filter branch from 2e7ec8f to 82899ce Compare August 24, 2017 23:04

pravisankar changed the title ~~Fix router FilterNamespaces()~~ Fix filter namespaces in template router Aug 24, 2017

pravisankar force-pushed the router-filter branch from 82899ce to 8c26d55 Compare August 28, 2017 19:27

rajatchopra approved these changes Aug 28, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017

knobunc requested changes Aug 29, 2017

View reviewed changes

pravisankar mentioned this pull request Aug 29, 2017

Sharded router based on namespace labels should notice routes immediately #16039

Merged

Fix filter namespaces in template router

86a8ea0

Colon instead of underscore is used to uniquely identify ServiceAliasConfig(route for a service).

Ravi Sankar Penta added 2 commits August 29, 2017 09:30

Remove endpoints and route key format assumptions in the template plu…

29e84f8

…gin and router code Get the endpoints and route key related information from one place, this will be less prone to errors and easy to modify in the future if needed.

Added test cases for router FilterNamespaces()

f088be6

pravisankar force-pushed the router-filter branch from 8c26d55 to f088be6 Compare August 29, 2017 16:31

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2017

knobunc approved these changes Aug 29, 2017

View reviewed changes

openshift-ci-robot assigned knobunc Aug 29, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2017

openshift-merge-robot merged commit 22375a5 into openshift:master Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix filter namespaces in template router #15916

Fix filter namespaces in template router #15916

pravisankar commented Aug 23, 2017

pravisankar commented Aug 23, 2017

pravisankar commented Aug 23, 2017

smarterclayton commented Aug 23, 2017

imcsk8 commented Aug 23, 2017

pravisankar commented Aug 23, 2017 via email

pravisankar commented Aug 24, 2017

pravisankar commented Aug 24, 2017

pravisankar commented Aug 28, 2017

pravisankar commented Aug 28, 2017

rajatchopra left a comment

openshift-bot commented Aug 29, 2017

openshift-bot commented Aug 29, 2017

knobunc left a comment

knobunc Aug 29, 2017

knobunc Aug 29, 2017

pravisankar Aug 29, 2017

knobunc Aug 29, 2017

knobunc Aug 29, 2017

knobunc Aug 29, 2017

pravisankar Aug 29, 2017

knobunc Aug 29, 2017

pravisankar Aug 29, 2017

pravisankar commented Aug 29, 2017

pravisankar commented Aug 29, 2017

openshift-ci-robot commented Aug 29, 2017 •

edited

Loading

knobunc left a comment

openshift-merge-robot commented Aug 29, 2017

openshift-bot commented Aug 30, 2017

openshift-merge-robot commented Aug 30, 2017

openshift-merge-robot commented Aug 30, 2017

Fix filter namespaces in template router #15916

Fix filter namespaces in template router #15916

Conversation

pravisankar commented Aug 23, 2017

pravisankar commented Aug 23, 2017

pravisankar commented Aug 23, 2017

smarterclayton commented Aug 23, 2017

imcsk8 commented Aug 23, 2017

pravisankar commented Aug 23, 2017 via email

pravisankar commented Aug 24, 2017

pravisankar commented Aug 24, 2017

pravisankar commented Aug 28, 2017

pravisankar commented Aug 28, 2017

rajatchopra left a comment

Choose a reason for hiding this comment

openshift-bot commented Aug 29, 2017

openshift-bot commented Aug 29, 2017

knobunc left a 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

pravisankar commented Aug 29, 2017

pravisankar commented Aug 29, 2017

openshift-ci-robot commented Aug 29, 2017 • edited Loading

knobunc left a comment

Choose a reason for hiding this comment

openshift-merge-robot commented Aug 29, 2017

openshift-bot commented Aug 30, 2017

openshift-merge-robot commented Aug 30, 2017

openshift-merge-robot commented Aug 30, 2017

openshift-ci-robot commented Aug 29, 2017 •

edited

Loading