From d1e725b69153d63621a3cb8dc5b934ffbb0c1f83 Mon Sep 17 00:00:00 2001 From: Ben B Date: Wed, 18 May 2022 00:00:40 +0200 Subject: [PATCH] adapt tls flag usage for upcoming release (#1891) Signed-off-by: Benedikt Bongartz --- apis/v1/jaeger_webhook.go | 55 ++++++++++++++++ apis/v1/jaeger_webhook_test.go | 112 +++++++++++++++++++++++++++++++++ apis/v1/options.go | 1 + 3 files changed, 168 insertions(+) diff --git a/apis/v1/jaeger_webhook.go b/apis/v1/jaeger_webhook.go index 563242e47..cdbad7cf6 100644 --- a/apis/v1/jaeger_webhook.go +++ b/apis/v1/jaeger_webhook.go @@ -3,6 +3,7 @@ package v1 import ( "context" "fmt" + "regexp" esv1 "github.com/openshift/elasticsearch-operator/apis/logging/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -36,6 +37,13 @@ func (j *Jaeger) SetupWebhookWithManager(mgr ctrl.Manager) error { var _ webhook.Defaulter = &Jaeger{} +func (j *Jaeger) objsWithOptions() []*Options { + return []*Options{ + &j.Spec.AllInOne.Options, &j.Spec.Query.Options, &j.Spec.Collector.Options, + &j.Spec.Ingester.Options, &j.Spec.Agent.Options, &j.Spec.Storage.Options, + } +} + // Default implements webhook.Defaulter so a webhook will be registered for the type func (j *Jaeger) Default() { jaegerlog.Info("default", "name", j.Name) @@ -56,6 +64,20 @@ func (j *Jaeger) Default() { } j.Spec.Storage.Elasticsearch.NodeCount = OpenShiftElasticsearchNodeCount(es.Spec) } + + for _, opt := range j.objsWithOptions() { + optCopy := opt.DeepCopy() + if f := getAdditionalTLSFlags(optCopy.ToArgs()); f != nil { + newOpts := optCopy.GenericMap() + for k, v := range f { + newOpts[k] = v + } + + if err := opt.parse(newOpts); err != nil { + jaegerlog.Error(err, "name", j.Name, "method", "Option.Parse") + } + } + } } // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation. @@ -84,6 +106,14 @@ func (j *Jaeger) ValidateUpdate(_ runtime.Object) error { return fmt.Errorf("elasticsearch instance not found: %v", err) } } + + for _, opt := range j.objsWithOptions() { + got := opt.DeepCopy().ToArgs() + if f := getAdditionalTLSFlags(got); f != nil { + return fmt.Errorf("tls flags incomplete, got: %v", got) + } + } + return nil } @@ -110,3 +140,28 @@ func ShouldInjectOpenShiftElasticsearchConfiguration(s JaegerStorageSpec) bool { _, ok := s.Options.Map()["es.server-urls"] return !ok } + +var ( + tlsFlag = regexp.MustCompile("--.*tls.*=") + tlsFlagIdx = regexp.MustCompile("--.*tls") + tlsEnabledExists = regexp.MustCompile("--.*tls.enabled") +) + +// getAdditionalTLSFlags returns additional tls arguments based on the argument +// list. If no additional argument is needed, nil is returned. +func getAdditionalTLSFlags(args []string) map[string]interface{} { + var res map[string]interface{} + for _, arg := range args { + a := []byte(arg) + if tlsEnabledExists.Match(a) { + // NOTE: if flag exists, we are done. + return nil + } + if tlsFlag.Match(a) && res == nil { + idx := tlsFlagIdx.FindIndex(a) + res = make(map[string]interface{}) + res[arg[idx[0]+2:idx[1]]+".enabled"] = "true" + } + } + return res +} diff --git a/apis/v1/jaeger_webhook_test.go b/apis/v1/jaeger_webhook_test.go index 5f7818c56..8207921b3 100644 --- a/apis/v1/jaeger_webhook_test.go +++ b/apis/v1/jaeger_webhook_test.go @@ -11,6 +11,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" ) @@ -127,6 +128,39 @@ func TestDefault(t *testing.T) { }, }, }, + { + name: "missing tls enable flag", + j: &Jaeger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "project1", + }, + Spec: JaegerSpec{ + Storage: JaegerStorageSpec{ + Type: JaegerMemoryStorage, + Options: NewOptions(map[string]interface{}{"stuff.tls.test": "something"}), + }, + }, + }, + expected: &Jaeger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "project1", + }, + Spec: JaegerSpec{ + Storage: JaegerStorageSpec{ + Type: JaegerMemoryStorage, + Options: NewOptions( + map[string]interface{}{ + "stuff.tls.test": "something", + "stuff.tls.enabled": "true", + }, + ), + Elasticsearch: ElasticsearchSpec{ + Name: defaultElasticsearchName, + }, + }, + }, + }, + }, } for _, test := range tests { @@ -142,6 +176,10 @@ func TestDefault(t *testing.T) { } } +func TestValidateDelete(t *testing.T) { + assert.Nil(t, new(Jaeger).ValidateDelete()) +} + func TestValidate(t *testing.T) { tests := []struct { name string @@ -211,6 +249,23 @@ func TestValidate(t *testing.T) { }, err: `elasticsearch instance not found: elasticsearchs.logging.openshift.io "my-es" not found`, }, + { + name: "missing tls options", + current: &Jaeger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "project1", + }, + Spec: JaegerSpec{ + Storage: JaegerStorageSpec{ + Options: NewOptions(map[string]interface{}{ + "something.tls.else": "fails", + }), + Type: JaegerMemoryStorage, + }, + }, + }, + err: `tls flags incomplete, got: [--something.tls.else=fails]`, + }, } for _, test := range tests { @@ -222,6 +277,7 @@ func TestValidate(t *testing.T) { err := test.current.ValidateCreate() if test.err != "" { + assert.NotNil(t, err) assert.Equal(t, test.err, err.Error()) } else { assert.Nil(t, err) @@ -246,3 +302,59 @@ func TestShouldDeployElasticsearch(t *testing.T) { }) } } + +func TestGetAdditionalTLSFlags(t *testing.T) { + tt := []struct { + name string + args []string + expect map[string]interface{} + }{ + { + name: "no tls flag", + args: []string{"--something.else"}, + expect: nil, + }, + { + name: "already enabled", + args: []string{"--something.tls.enabled=true", "--something.tls.else=abc"}, + expect: nil, + }, + { + name: "is disabled", + args: []string{"--tls.enabled=false", "--something.else", "--something.tls.else=abc"}, + expect: nil, + }, + { + name: "must be enabled", + args: []string{"--something.tls.else=abc"}, + expect: map[string]interface{}{ + "something.tls.enabled": "true", + }, + }, + { + // NOTE: we want to avoid something like: + // --kafka.consumer.authentication=tls.enabled=true + name: "enable consumer tls", + args: []string{ + "--es.server-urls=http://elasticsearch:9200", + "--kafka.consumer.authentication=tls", + "--kafka.consumer.brokers=my-cluster-kafka-bootstrap:9093", + "--kafka.consumer.tls.ca=/var/run/secrets/cluster-ca/ca.crt", + "--kafka.consumer.tls.cert=/var/run/secrets/kafkauser/user.crt", + "--kafka.consumer.tls.key=/var/run/secrets/kafkauser/user.key", + }, + expect: map[string]interface{}{ + "kafka.consumer.tls.enabled": "true", + }, + }, + } + + for _, tc := range tt { + t.Run(tc.name, func(t *testing.T) { + got := getAdditionalTLSFlags(tc.args) + if !cmp.Equal(tc.expect, got) { + t.Error("err:", cmp.Diff(tc.expect, got)) + } + }) + } +} diff --git a/apis/v1/options.go b/apis/v1/options.go index a84b6de5c..76a8d1408 100644 --- a/apis/v1/options.go +++ b/apis/v1/options.go @@ -88,6 +88,7 @@ func (o Options) MarshalJSON() ([]byte, error) { } func (o *Options) parse(entries map[string]interface{}) error { + o.json = nil o.opts = make(map[string]interface{}) var err error for k, v := range entries {