Add support to disable the namespace ownership checks #12441

ramr · 2017-01-11T03:11:23Z

This allows routes to claim non-overlapping hosts (+ paths) and wildcards across namespace
boundaries. Update generated docs and completions.

Associated trello card: https://trello.com/c/jd6RksVX/368-5-allow-host-claims-to-be-disabled-in-the-router-ingress

@knobunc PTAL

cc: @openshift/networking if any one else can also review. Thx

…utes to claim non-overlapping hosts (+ paths) and wildcards across namespace boundaries. Update generated docs and completions.

ramr · 2017-01-11T03:12:12Z

[test]

knobunc

The code LGTM. But I think we need to be a little more strident about the safety implications in the help text.

knobunc · 2017-01-11T14:26:52Z

docs/man/man1/oadm-router.1

@@ -36,6 +36,10 @@ If a router does not exist with the given name, this command will create a deplo
    Optional path to a certificate file that be used as the default certificate.  The file should contain the cert, key, and any CA certs necessary for the router to serve the certificate.

 .PP
+\fB\-\-disable\-namespace\-ownership\-check\fP=false
+    Disables the namespace ownership check and allows different namespaces to claim either different paths to a route host or overlapping host names in case of a wildcard route. The default behavior (false) to restrict claims to the oldest namespace that has claimed either the host or the subdomain.


I think we need to add some dire warnings to this. Something like:
Beware: A user in a different can use this to "steal" sub-paths for existing domains if they can create routes. This is only safe if route creation privilege is restricted, or if all users are trusted.

ack - will do.

knobunc · 2017-01-11T14:27:41Z

docs/man/man1/openshift-infra-f5-router.1

@@ -68,6 +68,10 @@ You may restrict the set of routes exposed to a single project (with \-\-namespa
    List of comma separated domains to deny in routes

 .PP
+\fB\-\-disable\-namespace\-ownership\-check\fP=false
+    Disables the namespace ownership checks for a route host with different paths or for overlapping host names in the case of wildcard routes


Should this be the same message as elsewhere? Also, should it end with a .?

This is generated from the pkg/cmd/infra/router/f5.go and the prior one from pkg/cmd/admin/router/router.go - will make sure the text syncs up.

knobunc · 2017-01-11T14:28:13Z

pkg/cmd/admin/router/router.go

+	//      www.example.org/path2 to be claimed by namespaces nsone and
+	//      nstwo respectively. And for wildcard routes, this allows
+	//      overlapping host names (*.example.test vs foo.example.test)
+	//      to be claimed by different namespaces.


Please put a warning here too

knobunc · 2017-01-11T14:29:06Z

pkg/cmd/infra/router/router.go

@@ -68,6 +69,7 @@ func (o *RouterSelection) Bind(flag *pflag.FlagSet) {
 	flag.StringSliceVar(&o.DeniedDomains, "denied-domains", envVarAsStrings("ROUTER_DENIED_DOMAINS", "", ","), "List of comma separated domains to deny in routes")
 	flag.StringSliceVar(&o.AllowedDomains, "allowed-domains", envVarAsStrings("ROUTER_ALLOWED_DOMAINS", "", ","), "List of comma separated domains to allow in routes. If specified, only the domains in this list will be allowed routes. Note that domains in the denied list take precedence over the ones in the allowed list")
 	flag.BoolVar(&o.AllowWildcardRoutes, "allow-wildcard-routes", cmdutil.Env("ROUTER_ALLOW_WILDCARD_ROUTES", "") == "true", "Allow wildcard host names for routes")
+	flag.BoolVar(&o.DisableNamespaceOwnershipCheck, "disable-namespace-ownership-check", cmdutil.Env("ROUTER_DISABLE_NAMESPACE_OWNERSHIP_CHECK", "") == "true", "Disables the namespace ownership checks for a route host with different paths or for overlapping host names in the case of wildcard routes")


... wildcard routes. This is unsafe in many environments, please read the documentation

ramr · 2017-01-11T21:15:26Z

[test]

ramr · 2017-01-11T23:39:03Z

No errors in test log - https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/9928/console
I see this in the console output ...

[INFO] Test Succeeded

[INFO] Dumping container logs to /tmp/openshift/test-end-to-end-docker//logs
[INFO] Dumping etcd contents to /tmp/openshift/test-end-to-end-docker//artifacts/etcd_dump.json
[INFO] remove the openshift container
origin
origin
[INFO] Stopping k8s docker containers
c458bf334b69

...

76465515d552
[INFO] Exiting
/data/src/github.com/openshift/origin/hack/test-end-to-end-docker.sh took 404 seconds
make: Target `test' not remade because of errors.
Build step 'Execute shell' marked build as failure

hack/test-end-to-end-docker.sh runs ${OS_ROOT}/test/end-to-end/core.sh which succeeded
but the hack/test-end-to-end-docker.sh script returned a non-zero exit code after printing
[INFO] Test Succeeded so not sure where the error is.

@marun do you have any ideas? Or is this a known flake?

knobunc · 2017-01-12T15:52:13Z

hack/test-tools.sh
Running hack/test-tools.sh:9: executing 'tools/junitreport/test/integration.sh' expecting success...
SUCCESS after 1.457s: hack/test-tools.sh:9: executing 'tools/junitreport/test/integration.sh' expecting success
test-tools: ok
KUBE_COVER=" " KUBE_RACE=" " hack/test-integration.sh
++ Building go targets for linux/amd64: test/integration/integration.test
# github.com/openshift/origin/test/integration
test/integration/router_stress_test.go:299: not enough arguments in call to "github.com/openshift/origin/pkg/router/controller".NewUniqueHost
[ERROR] PID 21410: hack/common.sh:257: `local -a binaries=("$@")` exited with status 1.
[INFO] 		Stack Trace: 
[INFO] 		  1: hack/common.sh:257: `local -a binaries=("$@")`
[INFO] 		  2: hack/build-go.sh:33: os::build::build_binaries
[INFO]   Exiting with code 1.
[ERROR] PID 21311: hack/test-integration.sh:33: `CGO_ENABLED=0 "${OS_ROOT}/hack/build-go.sh" "${package}/${name}.test" -installsuffix=cgo` exited with status 1.
[INFO] 		Stack Trace: 
[INFO] 		  1: hack/test-integration.sh:33: `CGO_ENABLED=0 "${OS_ROOT}/hack/build-go.sh" "${package}/${name}.test" -installsuffix=cgo`
[INFO]   Exiting with code 1.
Complete
make: *** [test-integration] Error 1
hack/build-go.sh

…ocs. Fix failing integration test.

ramr · 2017-01-12T20:09:47Z

thanks @knobunc - aargh, my bad!! :^(

ramr · 2017-01-12T20:09:59Z

[test]

knobunc · 2017-01-12T20:11:19Z

No worries, it was hard to spot, but I've had to find those before so I knew where to look.

openshift-bot · 2017-01-12T20:13:31Z

Evaluated for origin test up to b97a0d2

openshift-bot · 2017-01-12T21:41:44Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/12818/) (Base Commit: 831b981)

knobunc · 2017-01-18T20:07:44Z

@rajatchopra @openshift/networking @JacobTanenbaum PTAL

JacobTanenbaum

LGTM

knobunc · 2017-01-19T19:08:28Z

[merge]

openshift-bot · 2017-01-19T19:21:29Z

Evaluated for origin merge up to b97a0d2

openshift-bot · 2017-01-19T19:29:33Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13039/) (Base Commit: 7a315d5) (Image: devenv-rhel7_5726)

Add support to disable the namespace ownership checks. This allows ro…

d2dd466

…utes to claim non-overlapping hosts (+ paths) and wildcards across namespace boundaries. Update generated docs and completions.

knobunc requested changes Jan 11, 2017

View reviewed changes

Update help text as per @knobunc review comments and regenerate the d…

b97a0d2

…ocs. Fix failing integration test.

ramr force-pushed the disable-ns-checks branch from e92985c to b97a0d2 Compare January 12, 2017 20:08

knobunc approved these changes Jan 12, 2017

View reviewed changes

knobunc requested review from rajatchopra and JacobTanenbaum January 18, 2017 20:08

JacobTanenbaum approved these changes Jan 19, 2017

View reviewed changes

openshift-bot merged commit b2508ea into openshift:master Jan 19, 2017

ramr deleted the disable-ns-checks branch February 2, 2017 23:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to disable the namespace ownership checks #12441

Add support to disable the namespace ownership checks #12441

ramr commented Jan 11, 2017

ramr commented Jan 11, 2017

knobunc left a comment

knobunc Jan 11, 2017

ramr Jan 11, 2017

knobunc Jan 11, 2017

ramr Jan 11, 2017

knobunc Jan 11, 2017

knobunc Jan 11, 2017

ramr commented Jan 11, 2017

ramr commented Jan 11, 2017

knobunc commented Jan 12, 2017

ramr commented Jan 12, 2017

ramr commented Jan 12, 2017

knobunc commented Jan 12, 2017

openshift-bot commented Jan 12, 2017

openshift-bot commented Jan 12, 2017

knobunc commented Jan 18, 2017

JacobTanenbaum left a comment

knobunc commented Jan 19, 2017

openshift-bot commented Jan 19, 2017

openshift-bot commented Jan 19, 2017 •

edited

Loading

Add support to disable the namespace ownership checks #12441

Add support to disable the namespace ownership checks #12441

Conversation

ramr commented Jan 11, 2017

ramr commented Jan 11, 2017

knobunc left a comment

Choose a reason for hiding this comment

knobunc Jan 11, 2017

Choose a reason for hiding this comment

ramr Jan 11, 2017

Choose a reason for hiding this comment

knobunc Jan 11, 2017

Choose a reason for hiding this comment

ramr Jan 11, 2017

Choose a reason for hiding this comment

knobunc Jan 11, 2017

Choose a reason for hiding this comment

knobunc Jan 11, 2017

Choose a reason for hiding this comment

ramr commented Jan 11, 2017

ramr commented Jan 11, 2017

knobunc commented Jan 12, 2017

ramr commented Jan 12, 2017

ramr commented Jan 12, 2017

knobunc commented Jan 12, 2017

openshift-bot commented Jan 12, 2017

openshift-bot commented Jan 12, 2017

knobunc commented Jan 18, 2017

JacobTanenbaum left a comment

Choose a reason for hiding this comment

knobunc commented Jan 19, 2017

openshift-bot commented Jan 19, 2017

openshift-bot commented Jan 19, 2017 • edited Loading

openshift-bot commented Jan 19, 2017 •

edited

Loading