Make updating status on the router optional #17420
Conversation
openshift-ci-robot
added
the
size/M
openshift-merge-robot
added
the
approved
|
LGTM (not saying the other way so the bot doesn't merge it yet) Regarding the difference, I think that was an unintentional error. We should have been writing the canonical name in for the F5 case. |
|
BTW the test failed due to missing completions. |
smarterclayton
force-pushed
the
router_consistency
branch
from
bfc4f26 to
d9836cd
Compare
openshift-ci-robot
added
size/L
openshift-merge-robot
added
the
needs-rebase
smarterclayton
force-pushed
the
router_consistency
branch
from
d9836cd to
a437836
Compare
openshift-ci-robot
added
size/M
openshift-merge-robot
removed
the
needs-rebase
|
I need to add a few unit tests to better prove this out.
…On Tue, Nov 28, 2017 at 3:24 PM, OpenShift Merge Robot < ***@***.***> wrote:
@smarterclayton <https://github.com/smarterclayton> PR needs rebase
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17420 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p-m2IR9z0qXhyBJ2eO4cownM0xvoks5s7GwHgaJpZM4Qm02F>
.
|
openshift-merge-robot
added
the
needs-rebase
smarterclayton
force-pushed
the
router_consistency
branch
from
a437836 to
3ac3cd0
Compare
openshift-bot
removed
the
needs-rebase
openshift-ci-robot
added
size/L
|
Updated with an e2e test that correctly validates status is not set (and adds a test to scoped router to verify status is set) |
|
Test failure for GCP is because the image isn't updated |
|
/retest |
|
Please review, this is ready for merge once we branch (I'll disable the test before merge, then enable once we've published it). |
|
@ramr if you can give a final pass |
There was a problem hiding this comment.
@smarterclayton just 1 comment - unless something has changed, think it is a typo (copy-pasta) as I saw the same changes in pkg/cmd/infra/router/template.go.
| plugin = controller.NewExtendedValidator(plugin, recorder) | ||
| } | ||
| plugin = controller.NewUniqueHost(plugin, o.RouteSelectionFunc(), o.RouterSelection.DisableNamespaceOwnershipCheck, recorder) | ||
| plugin = controller.NewHostAdmitter(plugin, o.F5RouteAdmitterFunc(), o.AllowWildcardRoutes, o.RouterSelection.DisableNamespaceOwnershipCheck, recorder) |
There was a problem hiding this comment.
false instead of o.AllowWildcardRoutes
I didn't think we supported wildcard routes on the F5 router? AFAICR, it was doing an exact host match in the lookup. That said my knowledge is a bit dated - so maybe that's changed - @Miciah / @rajatchopra can confirm.
There was a problem hiding this comment.
No, we explicitly block wildcard routes on F5. https://github.com/openshift/origin/blob/master@%7B2018-02-20%7D/pkg/cmd/infra/router/f5.go#L194
There was a problem hiding this comment.
Yeah, I didn't want the code to drift. If we add it, it can be done in the admitter func.
There was a problem hiding this comment.
Can understand that. Am ok with the change but just wanted to call out here that if someone accidentally sets an environment variable to allow wildcard routes to the F5 router, it may cause different behavior (well untested previously) in the host admitter https://github.com/smarterclayton/origin/blob/3ac3cd0562bf2f3cc55f80500d6ed410813d1402/pkg/router/controller/host_admitter.go#L131.
There was a problem hiding this comment.
We are planning to deprecate the F5 Router pretty soon, anyway. So its a no factor.
There was a problem hiding this comment.
Deprecate or remove?
There was a problem hiding this comment.
Remove eventually. It will stay until the field is ready to go fully with the container connector solution by F5.
The deprecation notice will be up in 3.9 docs.
smarterclayton
force-pushed
the
router_consistency
branch
from
3ac3cd0 to
2e85474
Compare
Also unify a number of host setup paths. Makes F5 and template more consistent.
Verify that a router running in a namespace does update status if it has permission.
smarterclayton
force-pushed
the
router_consistency
branch
from
2e85474 to
8d3e864
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: smarterclayton 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:
Approvers can indicate their approval by writing |
|
Test disabled until after merge and an image is cut, then i'll have another PR to enable it. |
|
Applying label after discussion with ram |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. |
User can specify --update-status=false on the router to have a router that doesn't write back to routes. The default is true for backwards compatibilty. Allows testing routers when you don't have route status/update.
Also unify a number of host setup paths. Makes F5 and template more consistent.
Question for reviewer - on the F5 plugin we weren't performing a number of "standard" operations like setting the canonical hostname. Was there a concrete reason for that?
@openshift/sig-networking