Skip to content
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.

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

Add link to openshift console #1142

Merged
merged 12 commits into from
Aug 4, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
66 changes: 66 additions & 0 deletions pkg/consolelink/main.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package consolelink

import (
"fmt"

consolev1 "github.com/openshift/api/console/v1"
routev1 "github.com/openshift/api/route/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
)

// RouteAnnotation used to annotate the link with the route name
var RouteAnnotation = "consolelink.jaegertracing.io/route"

// Get returns an ConsoleLink specification for the current instance
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Get returns an ConsoleLink specification for the current instance
// Get returns a ConsoleLink specification for the current instance

func Get(jaeger *v1.Jaeger, route *routev1.Route) *consolev1.ConsoleLink {
// If ingress is not enable there is no reason for create a console link
if jaeger.Spec.Ingress.Enabled != nil && *jaeger.Spec.Ingress.Enabled == false {
return nil
}

return &consolev1.ConsoleLink{
ObjectMeta: metav1.ObjectMeta{
Name: jaeger.Namespace + ".jaeger-consolelink-" + jaeger.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

This name could be improved: this is a ConsoleLink object, so, we don't need consolelink in the name. The ConsoleLink resides in a specific namespace, so, the namespace doesn't need to be part of the name either. It's also using dots, which is uncommon for names. Here's how it turns up in a real case: observability.jaeger-consolelink-simplest.

I'd say that you can just use the jaeger.Name, like we have for the Route:

$ kubectl get routes -n observability
NAME       HOST/PORT                                 PATH   SERVICES         PORT    TERMINATION   WILDCARD
simplest   simplest-observability.apps-crc.testing          simplest-query   <all>   reencrypt     None

For reference, here's how the some built-in console links look like:

$ kubectl get consolelinks --all-namespaces
NAME                                        TEXT                URL                                               MENU   AGE
observability.jaeger-consolelink-simplest   Jaeger [simplest]   https://simplest-observability.apps-crc.testing          8m14s
openshift-blog                              OpenShift Blog      https://blog.openshift.com                               19d
openshift-learning-portal                   Learning Portal     https://learn.openshift.com/?ref=webconsole              19d

Namespace: jaeger.Namespace,
Labels: map[string]string{
"app.kubernetes.io/instance": jaeger.Name,
"app.kubernetes.io/managed-by": "jaeger-operator",
},
Annotations: map[string]string{
RouteAnnotation: route.Name,
},
},
Spec: consolev1.ConsoleLinkSpec{
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
Location: consolev1.NamespaceDashboard,
Link: consolev1.Link{
Text: "Jaeger [" + jaeger.Name + "]",
},
NamespaceDashboard: &consolev1.NamespaceDashboardSpec{
Namespaces: []string{
jaeger.Namespace,
},
},
},
}
}

// UpdateHref returns an ConsoleLink with the href value derived from the route
jpkrohling marked this conversation as resolved.
Show resolved Hide resolved
func UpdateHref(routes []routev1.Route, links []consolev1.ConsoleLink) []consolev1.ConsoleLink {
var updated []consolev1.ConsoleLink
mapRoutes := make(map[string]string)
for _, route := range routes {
mapRoutes[route.Name] = route.Spec.Host
}
for _, cl := range links {
routeName := cl.Annotations[RouteAnnotation]
// Only append it if we can found the route
if host, ok := mapRoutes[routeName]; ok {
cl.Spec.Href = fmt.Sprintf("https://%s", host)
updated = append(updated, cl)
}
//TODO: log if not found the route associated with the link
}
return updated
}
58 changes: 58 additions & 0 deletions pkg/consolelink/main_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package consolelink

import (
"testing"

consolev1 "github.com/openshift/api/console/v1"
corev1 "github.com/openshift/api/route/v1"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"

v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
)

func TestConsoleLinkGet(t *testing.T) {
jaegerName := "TestConsoleLinkJaeger"
jaegerNamespace := "TestNS"
routerNamer := "TestConsoleLinkRoute"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
routerNamer := "TestConsoleLinkRoute"
routerName := "TestConsoleLinkRoute"


jaeger := v1.NewJaeger(types.NamespacedName{Name: jaegerName, Namespace: jaegerNamespace})
route := &corev1.Route{
TypeMeta: metav1.TypeMeta{
Kind: "Route",
APIVersion: "route.openshift.io/v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: routerNamer,
},
}

link := Get(jaeger, route)
assert.Equal(t, jaeger.Namespace+".jaeger-consolelink-"+jaeger.Name, link.Name)
assert.Contains(t, link.Annotations, RouteAnnotation)
assert.Equal(t, routerNamer, link.Annotations[RouteAnnotation])
}

func TestUpdateHref(t *testing.T) {
jaegerName := "TestConsoleLinkJaeger"
jaegerNamespace := "TestNS"
routerNamer := "TestConsoleLinkRoute"
jaeger := v1.NewJaeger(types.NamespacedName{Name: jaegerName, Namespace: jaegerNamespace})
route := corev1.Route{
TypeMeta: metav1.TypeMeta{
Kind: "Route",
APIVersion: "route.openshift.io/v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: routerNamer,
},
}

link := Get(jaeger, &route)
assert.Equal(t, link.Spec.Href, "")
route.Spec.Host = "namespace.somehostname"
newLinks := UpdateHref([]corev1.Route{route}, []consolev1.ConsoleLink{*link})
assert.Equal(t, "https://"+route.Spec.Host, newLinks[0].Spec.Href)

}
5 changes: 5 additions & 0 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package controller

import (
consolev1 "github.com/openshift/api/console/v1"
routev1 "github.com/openshift/api/route/v1"
"sigs.k8s.io/controller-runtime/pkg/manager"

Expand All @@ -17,6 +18,10 @@ func AddToManager(m manager.Manager) error {
return err
}

if err := consolev1.Install(m.GetScheme()); err != nil {
return err
}

// Registry just the ImageStream - adding osimagev1.AddToScheme(..) causes
// the SecretList to be registered again, which resulted in
// https://github.com/kubernetes-sigs/controller-runtime/issues/362
Expand Down
65 changes: 65 additions & 0 deletions pkg/controller/jaeger/consolelink.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package jaeger

import (
"context"

osconsolev1 "github.com/openshift/api/console/v1"
log "github.com/sirupsen/logrus"
"go.opentelemetry.io/otel/global"
"sigs.k8s.io/controller-runtime/pkg/client"

v1 "github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
"github.com/jaegertracing/jaeger-operator/pkg/inventory"
"github.com/jaegertracing/jaeger-operator/pkg/tracing"
)

func (r *ReconcileJaeger) applyConsoleLinks(ctx context.Context, jaeger v1.Jaeger, desired []osconsolev1.ConsoleLink) error {
tracer := global.TraceProvider().GetTracer(v1.ReconciliationTracer)
ctx, span := tracer.Start(ctx, "applyConsoleLinks")
defer span.End()

opts := []client.ListOption{
client.InNamespace(jaeger.Namespace),
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the comment right before this review, I had two instances with the same name in different namespaces. One thing that was weird, is that only one console link was created. Turns out, console links are cluster-scoped, so, this logic here will make one instance remove another instance's console link. You can see this when running make run-debug and have two instances in the cluster:

DEBU[0008] Reconciling Jaeger                            execution="2020-07-31 14:32:38.767272647 +0000 UTC" instance=simplest namespace=myapp
DEBU[0009] creating console link                         consoleLink=myapp.jaeger-consolelink-simplest instance=simplest namespace=myapp
DEBU[0009] deleting console link                         consoleLink=observability.jaeger-consolelink-simplest instance=simplest namespace=
DEBU[0009] Reconciling Jaeger completed                  execution="2020-07-31 14:32:38.767272647 +0000 UTC" instance=simplest namespace=myapp

DEBU[0009] Reconciling Jaeger                            execution="2020-07-31 14:32:39.084445352 +0000 UTC" instance=simplest namespace=observability
DEBU[0009] creating console link                         consoleLink=observability.jaeger-consolelink-simplest instance=simplest namespace=observability
DEBU[0009] deleting console link                         consoleLink=myapp.jaeger-consolelink-simplest instance=simplest namespace=
DEBU[0009] Reconciling Jaeger completed                  execution="2020-07-31 14:32:39.084445352 +0000 UTC" instance=simplest namespace=observability

Note how the namespace in the "deleting console link" log entries is empty, which was the clue that led me to check the CRD's scope:

$ kubectl get crds consolelinks.console.openshift.io -o jsonpath='{.spec.scope}'
Cluster

A couple of things to keep in mind then:

  1. when the operator isn't running in cluster admin mode, it shouldn't even attempt to create a cluster object. Try running the operator with lower permissions, and see what happens
  2. when the user running the operator does have cluster admin rights, the logic in this controller shouldn't filter entries by the namespace. Rather, it should instead check the app.kubernetes.io/instance and app.kubernetes.io/managed-by labels. If the instance in the label isn't the same in the current reconciliation, leave it alone (don't update, don't delete).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm ok with 1), We should check for cluster admin before trying to create cluster object.

  1. Is not clear to me, I can fetch all consolelinks but checking for app.kubernetes.io/instance? In the case of two instances with same name in different namespaces, I'll have two of them with the same name... and we will have the same problem... or am I missunderstanding something? I was thinking on adding a new label with the namespace, so I can filter by namespace AND jaeger instance name. Does that make sense?

client.MatchingLabels(map[string]string{
"app.kubernetes.io/instance": jaeger.Name,
"app.kubernetes.io/managed-by": "jaeger-operator",
}),
}
list := &osconsolev1.ConsoleLinkList{}
if err := r.rClient.List(ctx, list, opts...); err != nil {
return tracing.HandleError(err, span)
}

inv := inventory.ForConsoleLinks(list.Items, desired)
for _, d := range inv.Create {
jaeger.Logger().WithFields(log.Fields{
"consoleLink": d.Name,
"namespace": d.Namespace,
}).Debug("creating console link")
if err := r.client.Create(ctx, &d); err != nil {
return tracing.HandleError(err, span)
}
}

for _, d := range inv.Update {
jaeger.Logger().WithFields(log.Fields{
"consoleLink": d.Name,
"namespace": d.Namespace,
}).Debug("updating console link")
if err := r.client.Update(ctx, &d); err != nil {
return tracing.HandleError(err, span)
}
}

for _, d := range inv.Delete {
jaeger.Logger().WithFields(log.Fields{
"consoleLink": d.Name,
"namespace": d.Namespace,
}).Debug("deleting console link")
if err := r.client.Delete(ctx, &d); err != nil {
return tracing.HandleError(err, span)
}
}

return nil
}
Loading