Fix selector for internal router service, so that it is correctly populated#90
Conversation
openshift-ci-robot
added
the
size/XS
| if s.Spec.Selector == nil { | ||
| s.Spec.Selector = map[string]string{} | ||
| } | ||
| s.Spec.Selector["router"] = name |
There was a problem hiding this comment.
What was the exact issue? Service already has 'router' selector and we are overriding the value?
I see that we also set router selector for RouterServiceCloud().
There was a problem hiding this comment.
I'm also missing the issue here... maybe we should add an e2e test in the operator to verify endpoints for the internal service?
There was a problem hiding this comment.
So the issue here is that the extra selector router=router-internal-default is causing issues. The deployment naturally doesn't have that label (name=router-internal-default is only generated here) and so there are no endpoints available for the internal service.
There's a few PRs stacked behind this one, so I'll add an e2e test in a follow up PR specifically to verify the endpoints of the internal service. Thx
There was a problem hiding this comment.
We still want to select a specific router, right? So before this change, the service selected on "app=router" and "router=router-internal-default", which was wrong because the second key-value pair did not match any router; with this change, the service selects on only "app=router", which selects any router that cluster-ingress-operator deploys; but what we want is that the service select on "app=router" and "router=router-default" in order to select the specific router for this service, right? Otherwise the service will round-robin all routers, and so you could end up getting metrics or health for any router when querying a specific router's internal service.
There was a problem hiding this comment.
D'oh, that's true - will fix up. Thanks
openshift-ci-robot
added
size/M
|
gave up waiting a new cluster to test this ... checking with the bot ... |
|
/lgtm |
|
/lgtm |
|
Looks good, thanks! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ironcladlou, Miciah, ramr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
6 participants
Noticed this while fixing up the router tests in origin - the service selector for the internal service adds an extra selector which causes issues. PTAL Thx
/cc @openshift/sig-network-edge