From 643cb75c5e7b4383c16a8421aff51e52ef65eb02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraci=20Paix=C3=A3o=20Kr=C3=B6hling?= Date: Tue, 21 Jan 2020 15:57:55 +0100 Subject: [PATCH] Include the Log Out option when a custom menu is used (#867) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Juraci Paixão Kröhling --- deploy/crds/jaegertracing.io_jaegers_crd.yaml | 4 + pkg/apis/jaegertracing/v1/jaeger_types.go | 4 + .../jaegertracing/v1/zz_generated.deepcopy.go | 7 +- .../jaegertracing/v1/zz_generated.openapi.go | 7 + pkg/strategy/controller.go | 68 +++++--- pkg/strategy/controller_test.go | 162 ++++++++++++++---- 6 files changed, 191 insertions(+), 61 deletions(-) diff --git a/deploy/crds/jaegertracing.io_jaegers_crd.yaml b/deploy/crds/jaegertracing.io_jaegers_crd.yaml index a792abc0a..84d51ab7d 100644 --- a/deploy/crds/jaegertracing.io_jaegers_crd.yaml +++ b/deploy/crds/jaegertracing.io_jaegers_crd.yaml @@ -9439,6 +9439,10 @@ spec: type: string sar: type: string + skipLogout: + description: SkipLogout tells the operator to not automatically + add a "Log Out" menu option to the custom Jaeger configuration + type: boolean type: object resources: description: ResourceRequirements describes the compute resource diff --git a/pkg/apis/jaegertracing/v1/jaeger_types.go b/pkg/apis/jaegertracing/v1/jaeger_types.go index f6f708624..0b4e1adb2 100644 --- a/pkg/apis/jaegertracing/v1/jaeger_types.go +++ b/pkg/apis/jaegertracing/v1/jaeger_types.go @@ -261,6 +261,10 @@ type JaegerIngressOpenShiftSpec struct { // +optional HtpasswdFile string `json:"htpasswdFile,omitempty"` + + // SkipLogout tells the operator to not automatically add a "Log Out" menu option to the custom Jaeger configuration + // +optional + SkipLogout *bool `json:"skipLogout,omitempty"` } // JaegerAllInOneSpec defines the options to be used when deploying the query diff --git a/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go b/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go index a2e3c04d1..afe7fd41e 100644 --- a/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go +++ b/pkg/apis/jaegertracing/v1/zz_generated.deepcopy.go @@ -385,6 +385,11 @@ func (in *JaegerIngesterSpec) DeepCopy() *JaegerIngesterSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JaegerIngressOpenShiftSpec) DeepCopyInto(out *JaegerIngressOpenShiftSpec) { *out = *in + if in.SkipLogout != nil { + in, out := &in.SkipLogout, &out.SkipLogout + *out = new(bool) + **out = **in + } return } @@ -406,7 +411,7 @@ func (in *JaegerIngressSpec) DeepCopyInto(out *JaegerIngressSpec) { *out = new(bool) **out = **in } - out.Openshift = in.Openshift + in.Openshift.DeepCopyInto(&out.Openshift) if in.Hosts != nil { in, out := &in.Hosts, &out.Hosts *out = make([]string, len(*in)) diff --git a/pkg/apis/jaegertracing/v1/zz_generated.openapi.go b/pkg/apis/jaegertracing/v1/zz_generated.openapi.go index 3281daaf4..cca26cd6e 100644 --- a/pkg/apis/jaegertracing/v1/zz_generated.openapi.go +++ b/pkg/apis/jaegertracing/v1/zz_generated.openapi.go @@ -1196,6 +1196,13 @@ func schema_pkg_apis_jaegertracing_v1_JaegerIngressOpenShiftSpec(ref common.Refe Format: "", }, }, + "skipLogout": { + SchemaProps: spec.SchemaProps{ + Description: "SkipLogout tells the operator to not automatically add a \"Log Out\" menu option to the custom Jaeger configuration", + Type: []string{"boolean"}, + Format: "", + }, + }, }, }, }, diff --git a/pkg/strategy/controller.go b/pkg/strategy/controller.go index 5a448995a..22b906455 100644 --- a/pkg/strategy/controller.go +++ b/pkg/strategy/controller.go @@ -2,7 +2,6 @@ package strategy import ( "context" - "encoding/json" "fmt" "strings" @@ -195,6 +194,7 @@ func normalizeUI(spec *v1.JaegerSpec) { } enableArchiveButton(uiOpts, spec.Storage.Options.Map()) disableDependenciesTab(uiOpts, spec.Storage.Type, spec.Storage.Dependencies.Enabled) + enableDocumentationLink(uiOpts, spec) enableLogOut(uiOpts, spec) if len(uiOpts) > 0 { spec.UI.Options = v1.NewFreeForm(uiOpts) @@ -233,40 +233,64 @@ func disableDependenciesTab(uiOpts map[string]interface{}, storage string, depsE } } +func enableDocumentationLink(uiOpts map[string]interface{}, spec *v1.JaegerSpec) { + if !viper.IsSet("documentation-url") { + return + } + + // if a custom menu has been specified, do not add the link to the documentation + if _, ok := uiOpts["menu"]; ok { + return + } + + e := map[string]interface{}{ + "label": "About", + "items": []interface{}{map[string]interface{}{ + "label": "Documentation", + "url": viper.GetString("documentation-url"), + }}, + } + uiOpts["menu"] = []interface{}{e} +} + func enableLogOut(uiOpts map[string]interface{}, spec *v1.JaegerSpec) { if (spec.Ingress.Enabled != nil && *spec.Ingress.Enabled == false) || spec.Ingress.Security != v1.IngressSecurityOAuthProxy { return } - if _, ok := uiOpts["menu"]; ok { + if spec.Ingress.Openshift.SkipLogout != nil && *spec.Ingress.Openshift.SkipLogout == true { return } - docURL := viper.GetString("documentation-url") - - menuStr := fmt.Sprintf(`[ - { - "label": "About", - "items": [ - { - "label": "Documentation", - "url": "%s" - } - ] - }, - { - "label": "Log Out", - "url": "/oauth/sign_in", - "anchorTarget": "_self" + var menuArray []interface{} + if m, ok := uiOpts["menu"]; ok { + menuArray = m.([]interface{}) + } + + for _, v := range menuArray { + converted, ok := v.(map[string]interface{}) + if !ok { + // not a map, skip + return } - ]`, docURL) - menuArray := make([]interface{}, 2) + // if it has a URL entry, and if the entry contains "/oauth/sign_in", skip + url := fmt.Sprintf("%v", converted["url"]) + // this is very naive, but will work for most cases, as that's how the OpenShift OAuth Proxy + // build the URL. If needed, this can be a list of patterns in the future + if strings.Contains(url, "/oauth/sign_in") { + return + } + } - json.Unmarshal([]byte(menuStr), &menuArray) + logout := map[string]interface{}{ + "label": "Log Out", + "url": "/oauth/sign_in", + "anchorTarget": "_self", + } - uiOpts["menu"] = menuArray + uiOpts["menu"] = append(menuArray, logout) } func unknownStorage(typ string) bool { diff --git a/pkg/strategy/controller_test.go b/pkg/strategy/controller_test.go index 46f0af1c6..da73231ec 100644 --- a/pkg/strategy/controller_test.go +++ b/pkg/strategy/controller_test.go @@ -13,14 +13,14 @@ import ( ) func TestNewControllerForAllInOneAsDefault(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForAllInOneAsDefault"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) ctrl := For(context.TODO(), jaeger) assert.Equal(t, ctrl.Type(), v1.DeploymentStrategyAllInOne) } func TestNewControllerForAllInOneAsExplicitValue(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForAllInOneAsExplicitValue"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Strategy = v1.DeploymentStrategyDeprecatedAllInOne // same as 'all-in-one' ctrl := For(context.TODO(), jaeger) @@ -28,7 +28,7 @@ func TestNewControllerForAllInOneAsExplicitValue(t *testing.T) { } func TestNewControllerForProduction(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForProduction"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Strategy = v1.DeploymentStrategyProduction jaeger.Spec.Storage.Type = "elasticsearch" @@ -37,14 +37,14 @@ func TestNewControllerForProduction(t *testing.T) { } func TestUnknownStorage(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestNewControllerForProduction"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Storage.Type = "unknown" normalize(context.Background(), jaeger) assert.Equal(t, "memory", jaeger.Spec.Storage.Type) } func TestElasticsearchAsStorageOptions(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestElasticsearchAsStorageOptions"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Strategy = v1.DeploymentStrategyProduction jaeger.Spec.Storage.Type = "elasticsearch" jaeger.Spec.Storage.Options = v1.NewOptions(map[string]interface{}{ @@ -135,13 +135,13 @@ func TestStorageMemoryOnlyUsedWithAllInOneStrategy(t *testing.T) { } func TestSetSecurityToNoneByDefault(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestSetSecurityToNoneByDefault"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) normalize(context.Background(), jaeger) assert.Equal(t, v1.IngressSecurityNoneExplicit, jaeger.Spec.Ingress.Security) } func TestSetSecurityToNoneWhenExplicitSettingToNone(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestSetSecurityToNoneWhenExplicitSettingToNone"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Ingress.Security = v1.IngressSecurityNoneExplicit normalize(context.Background(), jaeger) assert.Equal(t, v1.IngressSecurityNoneExplicit, jaeger.Spec.Ingress.Security) @@ -151,14 +151,14 @@ func TestSetSecurityToOAuthProxyByDefaultOnOpenShift(t *testing.T) { viper.Set("platform", "openshift") defer viper.Reset() - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestSetSecurityToOAuthProxyByDefaultOnOpenShift"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) normalize(context.Background(), jaeger) assert.Equal(t, v1.IngressSecurityOAuthProxy, jaeger.Spec.Ingress.Security) } func TestSetSecurityToNoneOnNonOpenShift(t *testing.T) { - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestSetSecurityToNoneOnNonOpenShift"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Ingress.Security = v1.IngressSecurityOAuthProxy normalize(context.Background(), jaeger) @@ -170,7 +170,7 @@ func TestAcceptExplicitValueFromSecurityWhenOnOpenShift(t *testing.T) { viper.Set("platform", "openshift") defer viper.Reset() - jaeger := v1.NewJaeger(types.NamespacedName{Name: "TestAcceptExplicitValueFromSecurityWhenOnOpenShift"}) + jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"}) jaeger.Spec.Ingress.Security = v1.IngressSecurityNoneExplicit normalize(context.Background(), jaeger) @@ -404,31 +404,20 @@ func TestNormalizeUIDependenciesTab(t *testing.T) { } } -func TestMenuWithSignOut(t *testing.T) { - viper.SetDefault("documentation-url", "https://www.jaegertracing.io/docs/latest") - defer viper.Reset() - +func TestMenuWithLogOut(t *testing.T) { + spec := &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityOAuthProxy}} uiOpts := map[string]interface{}{} - enableLogOut(uiOpts, &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityOAuthProxy}}) + enableLogOut(uiOpts, spec) assert.Contains(t, uiOpts, "menu") expected := []interface{}{ - map[string]interface{}{ - "label": "About", - "items": []interface{}{ - map[string]interface{}{ - "label": "Documentation", - "url": "https://www.jaegertracing.io/docs/latest", - }, - }, - }, map[string]interface{}{ "label": "Log Out", "url": "/oauth/sign_in", "anchorTarget": "_self", }, } - assert.Equal(t, uiOpts["menu"], expected) + assert.Equal(t, expected, uiOpts["menu"]) } func TestMenuWithCustomDocURL(t *testing.T) { @@ -438,7 +427,8 @@ func TestMenuWithCustomDocURL(t *testing.T) { defer viper.Reset() uiOpts := map[string]interface{}{} - enableLogOut(uiOpts, &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityOAuthProxy}}) + spec := &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityOAuthProxy}} + enableDocumentationLink(uiOpts, spec) assert.Contains(t, uiOpts, "menu") expected := []interface{}{ @@ -451,28 +441,124 @@ func TestMenuWithCustomDocURL(t *testing.T) { }, }, }, + } + assert.Equal(t, expected, uiOpts["menu"]) +} + +func TestMenuNoLogOutIngressSecurityNone(t *testing.T) { + uiOpts := map[string]interface{}{} + spec := &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityNoneExplicit}} + enableLogOut(uiOpts, spec) + assert.NotContains(t, uiOpts, "menu") +} + +func TestMenuNoLogOutExistingMenuWithSkipOption(t *testing.T) { + // prepare + internalLink := map[string]interface{}{ + "label": "Some internal links", + "items": []interface{}{ + map[string]interface{}{ + "label": "The internal link", + "url": "http://example.com/internal", + }, + }, + } + uiOpts := map[string]interface{}{ + "menu": []interface{}{internalLink}, + } + trueVar := true + spec := &v1.JaegerSpec{ + Ingress: v1.JaegerIngressSpec{ + Security: v1.IngressSecurityOAuthProxy, + Openshift: v1.JaegerIngressOpenShiftSpec{ + SkipLogout: &trueVar, + }, + }, + } + + // test + enableLogOut(uiOpts, spec) + + // verify + assert.Len(t, uiOpts["menu"], 1) + expected := []interface{}{internalLink} + assert.Equal(t, expected, uiOpts["menu"]) +} + +func TestCustomMenuGetsLogOutAdded(t *testing.T) { + // prepare + internalLink := map[string]interface{}{ + "label": "Some internal links", + "items": []interface{}{ + map[string]interface{}{ + "label": "The internal link", + "url": "http://example.com/internal", + }, + }, + } + uiOpts := map[string]interface{}{ + "menu": []interface{}{internalLink}, + } + spec := &v1.JaegerSpec{ + Ingress: v1.JaegerIngressSpec{ + Security: v1.IngressSecurityOAuthProxy, + }, + } + + // test + enableLogOut(uiOpts, spec) + + // verify + expected := []interface{}{ + internalLink, map[string]interface{}{ "label": "Log Out", "url": "/oauth/sign_in", "anchorTarget": "_self", }, } - assert.Equal(t, uiOpts["menu"], expected) -} - -func TestMenuNoSignOutIngressSecurityNone(t *testing.T) { - uiOpts := map[string]interface{}{} - enableLogOut(uiOpts, &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityNoneExplicit}}) - assert.NotContains(t, uiOpts, "menu") + assert.Len(t, uiOpts["menu"], 2) + assert.Equal(t, expected, uiOpts["menu"]) } -func TestMenuNoSignOutExistingMenu(t *testing.T) { +func TestCustomMenuGetsLogOutSkipped(t *testing.T) { + // prepare + internalLink := map[string]interface{}{ + "label": "Some internal links", + "items": []interface{}{ + map[string]interface{}{ + "label": "The internal link", + "url": "http://example.com/internal", + }, + }, + } + logout := map[string]interface{}{ + "label": "Custom Log Out", + "url": "https://example.com/custom/path/to/oauth/sign_in", + "anchorTarget": "_self", + } uiOpts := map[string]interface{}{ - "menu": []interface{}{}, + "menu": []interface{}{ + internalLink, + logout, + }, } - enableLogOut(uiOpts, &v1.JaegerSpec{Ingress: v1.JaegerIngressSpec{Security: v1.IngressSecurityOAuthProxy}}) - assert.Contains(t, uiOpts, "menu") - assert.Len(t, uiOpts["menu"], 0) + spec := &v1.JaegerSpec{ + Ingress: v1.JaegerIngressSpec{ + Security: v1.IngressSecurityOAuthProxy, + }, + } + + // test + enableLogOut(uiOpts, spec) + + // verify + expected := []interface{}{ + internalLink, + logout, + } + assert.Len(t, uiOpts["menu"], 2) + assert.Equal(t, expected, uiOpts["menu"]) } func assertHasAllObjects(t *testing.T, name string, s S, deployments map[string]bool, daemonsets map[string]bool, services map[string]bool, ingresses map[string]bool, routes map[string]bool, serviceAccounts map[string]bool, configMaps map[string]bool) {