Skip to content

Fix selector for internal router service, so that it is correctly populated #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Dec 19, 2018
Merged

Fix selector for internal router service, so that it is correctly populated #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/manifests/manifests.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ func (f *Factory) RouterServiceInternal(cr *ingressv1alpha1.ClusterIngress) (*co
if s.Spec.Selector == nil {
s.Spec.Selector = map[string]string{}
}
s.Spec.Selector["router"] = name
Copy link

@pravisankar pravisankar Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor

@ironcladlou ironcladlou Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also missing the issue here... maybe we should add an e2e test in the operator to verify endpoints for the internal service?

Copy link
Contributor Author

@ramr ramr Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@Miciah Miciah Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ramr ramr Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

D'oh, that's true - will fix up. Thanks

s.Spec.Selector["router"] = "router-" + cr.Name

return s, nil
}
Expand Down
63 changes: 63 additions & 0 deletions test/e2e/operator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,69 @@ func TestDefaultClusterIngressExists(t *testing.T) {
}
}

func TestRouterServiceInternalEndpoints(t *testing.T) {
cl, ns, err := getClient()
if err != nil {
t.Fatal(err)
}

ci := &ingressv1alpha1.ClusterIngress{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: ns, Name: "default"}, ci); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get default ClusterIngress: %v", err)
}

// Wait for the router deployment to exist.
deployment := &appsv1.Deployment{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: "openshift-ingress", Name: fmt.Sprintf("router-%s", ci.Name)}, deployment); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get default router deployment: %v", err)
}

// check if service exists and has endpoints
svc := &corev1.Service{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: "openshift-ingress", Name: fmt.Sprintf("router-internal-%s", ci.Name)}, svc); err != nil {
return false, nil
}
return true, nil
})
if err != nil {
t.Fatalf("failed to get internal router service: %v", err)
}

endpoints := &corev1.Endpoints{}
err = wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) {
if err := cl.Get(context.TODO(), types.NamespacedName{Namespace: "openshift-ingress", Name: fmt.Sprintf("router-internal-%s", ci.Name)}, endpoints); err != nil {
return false, nil
}

nready := 0
for i := range endpoints.Subsets {
es := &endpoints.Subsets[i]
nready = nready + len(es.Addresses)
}
if nready > 0 {
return true, nil
}

return false, nil
})
if err != nil {
t.Fatalf("failed to get internal router service endpoints: %v", err)
}
}

// TODO: Use manifest factory to build expectations
// TODO: Find a way to do this test without mutating the default ingress?
func TestClusterIngressUpdate(t *testing.T) {
Expand Down