From f5a8cf7a6e665259ca530af6c999399a03a4c624 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Thu, 5 Dec 2019 18:46:23 -0600 Subject: [PATCH 01/12] WIP --- pkg/controller/kibana/config/fields.go | 1 + pkg/controller/kibana/config/settings.go | 9 +++++++++ pkg/controller/kibana/driver.go | 1 + 3 files changed, 11 insertions(+) diff --git a/pkg/controller/kibana/config/fields.go b/pkg/controller/kibana/config/fields.go index 4a8221710b..1a34ae9fa6 100644 --- a/pkg/controller/kibana/config/fields.go +++ b/pkg/controller/kibana/config/fields.go @@ -10,6 +10,7 @@ const ( ServerHost = "server.host" ElasticSearchHosts = "elasticsearch.hosts" XpackMonitoringUiContainerElasticsearchEnabled = "xpack.monitoring.ui.container.elasticsearch.enabled" + XpackSecurityEncryptionKey = "xpack.security.encryptionKey" ElasticsearchSslCertificateAuthorities = "elasticsearch.ssl.certificateAuthorities" ElasticsearchSslVerificationMode = "elasticsearch.ssl.verificationMode" diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 89a5e336a3..65bca9aac7 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -15,6 +15,8 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" + + "k8s.io/apimachinery/pkg/util/rand" ) // Kibana configuration settings file @@ -105,3 +107,10 @@ func elasticsearchTLSSettings(kb v1beta1.Kibana) map[string]interface{} { return cfg } + +// ensureEncryptionKey ensures a given config contains an encryption key +func ensureEncryptionKey(cfg map[string]interface{}) { + if _, ok := cfg[XpackSecurityEncryptionKey]; !ok { + cfg[XpackSecurityEncryptionKey] = rand.String(64) + } +} diff --git a/pkg/controller/kibana/driver.go b/pkg/controller/kibana/driver.go index 37d34fcb13..8f83ae0d02 100644 --- a/pkg/controller/kibana/driver.go +++ b/pkg/controller/kibana/driver.go @@ -249,6 +249,7 @@ func (d *driver) Reconcile( if results.HasError() { return &results } + // TODO sabo get current secret here? versionSpecificCfg := settings.MustCanonicalConfig(d.settingsFactory(*kb)) kbSettings, err := config.NewConfigSettings(d.client, *kb, versionSpecificCfg) From cfcbb3afbf78cc47e25c0eaff543df42daf4b9f8 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Fri, 6 Dec 2019 17:29:54 -0600 Subject: [PATCH 02/12] WIP --- pkg/controller/kibana/config/reconciler.go | 29 ++++++++++++++++++++-- pkg/controller/kibana/config/settings.go | 2 +- pkg/controller/kibana/driver.go | 3 ++- 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/pkg/controller/kibana/config/reconciler.go b/pkg/controller/kibana/config/reconciler.go index b838f1f826..7ebece0afd 100644 --- a/pkg/controller/kibana/config/reconciler.go +++ b/pkg/controller/kibana/config/reconciler.go @@ -5,16 +5,18 @@ package config import ( - "reflect" - "github.com/elastic/cloud-on-k8s/pkg/about" "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1beta1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/reconciler" + "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/label" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" + "reflect" ) // ReconcileConfigSecret reconciles the expected Kibana config secret for the given Kibana resource. @@ -64,3 +66,26 @@ func ReconcileConfigSecret( } return nil } + +// GetConfig retrieves the canonical config for a given Kibana, if one exists +func GetConfig(client k8s.Client, kb v1beta1.Kibana) (*settings.CanonicalConfig, error) { + var secret corev1.Secret + var cfg *settings.CanonicalConfig + err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) + // how do we want to indicate that nothing is found? + if err != nil && apierrors.IsNotFound(err) { + return cfg, nil + } + rawCfg, exists := secret.Data[SettingsFilename] + if !exists { + // TODO make this an error + return cfg, nil + } + // dict := make(map(string[interface{}])) + // _ = yaml.Unmarshal(rawCfg, dict) + // if err != nil{ + + // } + cfg, _ = settings.ParseConfig(rawCfg) + return cfg, nil +} diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 65bca9aac7..f16478740c 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -29,7 +29,7 @@ type CanonicalConfig struct { } // NewConfigSettings returns the Kibana configuration settings for the given Kibana resource. -func NewConfigSettings(client k8s.Client, kb v1beta1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { +func NewConfigSettings(client k8s.Client, kb v1beta1.Kibana, versionSpecificCfg, currentConfig *settings.CanonicalConfig) (CanonicalConfig, error) { specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1beta1.Config{} diff --git a/pkg/controller/kibana/driver.go b/pkg/controller/kibana/driver.go index 8f83ae0d02..745fc2104e 100644 --- a/pkg/controller/kibana/driver.go +++ b/pkg/controller/kibana/driver.go @@ -250,9 +250,10 @@ func (d *driver) Reconcile( return &results } // TODO sabo get current secret here? + currentCfg, err := config.GetConfig(d.client, *kb) versionSpecificCfg := settings.MustCanonicalConfig(d.settingsFactory(*kb)) - kbSettings, err := config.NewConfigSettings(d.client, *kb, versionSpecificCfg) + kbSettings, err := config.NewConfigSettings(d.client, *kb, versionSpecificCfg, currentCfg) if err != nil { return results.WithError(err) } From c17fefaf6cc6958446d1347e1e59235b133fb0f2 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Thu, 12 Dec 2019 14:42:09 -0600 Subject: [PATCH 03/12] WIP --- pkg/controller/kibana/config/reconciler.go | 29 ++------------------ pkg/controller/kibana/config/settings.go | 31 +++++++++++++++++++++- 2 files changed, 32 insertions(+), 28 deletions(-) diff --git a/pkg/controller/kibana/config/reconciler.go b/pkg/controller/kibana/config/reconciler.go index de5413c3a8..0c5d457fc0 100644 --- a/pkg/controller/kibana/config/reconciler.go +++ b/pkg/controller/kibana/config/reconciler.go @@ -5,18 +5,16 @@ package config import ( + "reflect" + "github.com/elastic/cloud-on-k8s/pkg/about" kbv1 "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/reconciler" - "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/label" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" corev1 "k8s.io/api/core/v1" - apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" - "reflect" ) // ReconcileConfigSecret reconciles the expected Kibana config secret for the given Kibana resource. @@ -66,26 +64,3 @@ func ReconcileConfigSecret( } return nil } - -// GetConfig retrieves the canonical config for a given Kibana, if one exists -func GetConfig(client k8s.Client, kb v1beta1.Kibana) (*settings.CanonicalConfig, error) { - var secret corev1.Secret - var cfg *settings.CanonicalConfig - err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) - // how do we want to indicate that nothing is found? - if err != nil && apierrors.IsNotFound(err) { - return cfg, nil - } - rawCfg, exists := secret.Data[SettingsFilename] - if !exists { - // TODO make this an error - return cfg, nil - } - // dict := make(map(string[interface{}])) - // _ = yaml.Unmarshal(rawCfg, dict) - // if err != nil{ - - // } - cfg, _ = settings.ParseConfig(rawCfg) - return cfg, nil -} diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 20ac326ccf..6ba6331a33 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -15,7 +15,9 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" - + "k8s.io/apimachinery/pkg/types" + apierrors "k8s.io/apimachinery/pkg/api/errors" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/rand" ) @@ -29,7 +31,10 @@ type CanonicalConfig struct { } // NewConfigSettings returns the Kibana configuration settings for the given Kibana resource. +// TODO sabo does it belong here? func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { + + currentConfig, _ := getExistingConfig(client, kb) specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1.Config{} @@ -75,6 +80,30 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se return CanonicalConfig{cfg}, nil } +// getExistingConfig retrieves the canonical config for a given Kibana, if one exists +func getExistingConfig(client k8s.Client, kb kbv1.Kibana) (*settings.CanonicalConfig, error) { + var secret corev1.Secret + var cfg *settings.CanonicalConfig + err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) + // how do we want to indicate that nothing is found? + if err != nil && apierrors.IsNotFound(err) { + return cfg, nil + } + rawCfg, exists := secret.Data[SettingsFilename] + if !exists { + // TODO make this an error + return cfg, nil + } + // dict := make(map(string[interface{}])) + // _ = yaml.Unmarshal(rawCfg, dict) + // if err != nil{ + + // } + cfg, _ = settings.ParseConfig(rawCfg) + return cfg, nil +} + + func baseSettings(kb kbv1.Kibana) map[string]interface{} { return map[string]interface{}{ ServerName: kb.Name, From fc9b85dd874d79ce1db4ec351e17204e0b342aa8 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Mon, 16 Dec 2019 09:45:33 -0600 Subject: [PATCH 04/12] WIP --- pkg/controller/kibana/config/settings.go | 25 ++--- pkg/controller/kibana/config/settings_test.go | 91 +++++++++++++++++++ 2 files changed, 99 insertions(+), 17 deletions(-) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 6ba6331a33..1e868ae0e1 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -7,6 +7,8 @@ package config import ( "path" + "github.com/pkg/errors" + commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" kbv1 "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/association" @@ -15,9 +17,8 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" - "k8s.io/apimachinery/pkg/types" - apierrors "k8s.io/apimachinery/pkg/api/errors" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" ) @@ -34,7 +35,7 @@ type CanonicalConfig struct { // TODO sabo does it belong here? func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { - currentConfig, _ := getExistingConfig(client, kb) + // currentConfig, _ := getExistingConfig(client, kb) specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1.Config{} @@ -83,27 +84,17 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se // getExistingConfig retrieves the canonical config for a given Kibana, if one exists func getExistingConfig(client k8s.Client, kb kbv1.Kibana) (*settings.CanonicalConfig, error) { var secret corev1.Secret - var cfg *settings.CanonicalConfig err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) - // how do we want to indicate that nothing is found? - if err != nil && apierrors.IsNotFound(err) { - return cfg, nil + if err != nil { + return nil, errors.WithStack(err) } rawCfg, exists := secret.Data[SettingsFilename] if !exists { - // TODO make this an error - return cfg, nil + return nil, errors.Errorf("No key %s in secret %s/%s", SettingsFilename, secret.Namespace, secret.Name) } - // dict := make(map(string[interface{}])) - // _ = yaml.Unmarshal(rawCfg, dict) - // if err != nil{ - - // } - cfg, _ = settings.ParseConfig(rawCfg) - return cfg, nil + return settings.ParseConfig(rawCfg) } - func baseSettings(kb kbv1.Kibana) map[string]interface{} { return map[string]interface{}{ ServerName: kb.Name, diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index 9d1dcd941e..59a11cab74 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -14,6 +14,7 @@ import ( ucfg "github.com/elastic/go-ucfg" uyaml "github.com/elastic/go-ucfg/yaml" "github.com/go-test/deep" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -195,3 +196,93 @@ func mkKibana() kbv1.Kibana { kb := kbv1.Kibana{} return kb } + +func Test_getExistingConfig(t *testing.T) { + + testKb := kbv1.Kibana{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testkb", + Namespace: "testns", + }, + } + testValidSecret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(testKb), + Namespace: testKb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: defaultConfig, + }, + } + testNoYaml := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(testKb), + Namespace: testKb.Namespace, + }, + Data: map[string][]byte{ + "notarealkey": []byte(`:-{`), + }, + } + testInvalidYaml := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(testKb), + Namespace: testKb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: []byte(`:-{`), + }, + } + + tests := []struct { + name string + kb kbv1.Kibana + secret corev1.Secret + expectErr bool + expectKey string + }{ + { + name: "happy path", + kb: testKb, + secret: testValidSecret, + expectErr: false, + expectKey: "xpack", + }, + { + name: "no secret exists", + kb: testKb, + secret: corev1.Secret{}, + expectErr: true, + expectKey: "", + }, + { + name: "no kibana.yml exists in secret", + kb: testKb, + secret: testNoYaml, + expectErr: true, + expectKey: "", + }, + { + name: "cannot parse yaml", + kb: testKb, + secret: testInvalidYaml, + expectErr: true, + expectKey: "", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := k8s.WrapClient(fake.NewFakeClient(&tc.secret)) + returned, err := getExistingConfig(client, tc.kb) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + if tc.expectKey != "" { + require.NotNil(t, returned) + assert.True(t, (*ucfg.Config)(returned).HasField("xpack")) + } + }) + } +} From a60dc2e72c29d50950d2689ad5540e120ac4a1f4 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Tue, 17 Dec 2019 10:05:38 -0600 Subject: [PATCH 05/12] Add tests --- pkg/controller/kibana/config/settings.go | 43 +++++++-- pkg/controller/kibana/config/settings_test.go | 92 ++++++++++++++----- 2 files changed, 103 insertions(+), 32 deletions(-) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 1e868ae0e1..071bb11c81 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -7,8 +7,6 @@ package config import ( "path" - "github.com/pkg/errors" - commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" kbv1 "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/association" @@ -18,12 +16,19 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" + logf "sigs.k8s.io/controller-runtime/pkg/log" ) // Kibana configuration settings file -const SettingsFilename = "kibana.yml" +const ( + SettingsFilename = "kibana.yml" + name = "kibana-config" +) + +var log = logf.Log.WithName(name) // CanonicalConfig contains configuration for Kibana ("kibana.yml"), // as a hierarchical key-value configuration. @@ -35,7 +40,8 @@ type CanonicalConfig struct { // TODO sabo does it belong here? func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { - // currentConfig, _ := getExistingConfig(client, kb) + currentConfig := getExistingConfig(client, kb) + specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1.Config{} @@ -50,7 +56,11 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se kibanaTLSCfg := settings.MustCanonicalConfig(kibanaTLSSettings(kb)) if !kb.RequiresAssociation() { - if err := cfg.MergeWith(versionSpecificCfg, kibanaTLSCfg, userSettings); err != nil { + if err := cfg.MergeWith( + currentConfig, + versionSpecificCfg, + kibanaTLSCfg, + userSettings); err != nil { return CanonicalConfig{}, err } return CanonicalConfig{cfg}, nil @@ -63,6 +73,7 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se // merge the configuration with userSettings last so they take precedence err = cfg.MergeWith( + currentConfig, versionSpecificCfg, kibanaTLSCfg, settings.MustCanonicalConfig(elasticsearchTLSSettings(kb)), @@ -82,17 +93,27 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se } // getExistingConfig retrieves the canonical config for a given Kibana, if one exists -func getExistingConfig(client k8s.Client, kb kbv1.Kibana) (*settings.CanonicalConfig, error) { +func getExistingConfig(client k8s.Client, kb kbv1.Kibana) *settings.CanonicalConfig { var secret corev1.Secret err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) - if err != nil { - return nil, errors.WithStack(err) + if err != nil && apierrors.IsNotFound(err) { + log.V(1).Info("Kibana config secret does not exist", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) + return nil + } else if err != nil && !apierrors.IsNotFound(err) { + log.Error(err, "Error retrieving kibana config secret", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) + return nil } rawCfg, exists := secret.Data[SettingsFilename] if !exists { - return nil, errors.Errorf("No key %s in secret %s/%s", SettingsFilename, secret.Namespace, secret.Name) + log.Error(nil, "No kibana config file in secret", "secret_namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) + return nil + } + cfg, err := settings.ParseConfig(rawCfg) + if err != nil { + log.Error(err, "Error parsing existing kibana config in secret", "secret_namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) + return nil } - return settings.ParseConfig(rawCfg) + return cfg } func baseSettings(kb kbv1.Kibana) map[string]interface{} { @@ -101,6 +122,8 @@ func baseSettings(kb kbv1.Kibana) map[string]interface{} { ServerHost: "0", ElasticSearchHosts: []string{kb.AssociationConf().GetURL()}, XpackMonitoringUiContainerElasticsearchEnabled: true, + // this will get overriden if one already exists or is specified by the user + XpackSecurityEncryptionKey: rand.String(64), } } diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index 59a11cab74..0d229e6b6f 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -27,12 +27,14 @@ elasticsearch: - "" server: host: "0" - name: "" + name: "testkb" ssl: enabled: true key: /mnt/elastic-internal/http-certs/tls.key certificate: /mnt/elastic-internal/http-certs/tls.crt xpack: + security: + encryptionKey: thisismyencryptionkey monitoring: ui: container: @@ -52,6 +54,16 @@ elasticsearch: `) func TestNewConfigSettings(t *testing.T) { + defaultKb := mkKibana() + existingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(defaultKb), + Namespace: defaultKb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: []byte("xpack.security.encryptionKey: thisismyencryptionkey"), + }, + } type args struct { client k8s.Client kb func() kbv1.Kibana @@ -65,13 +77,15 @@ func TestNewConfigSettings(t *testing.T) { { name: "default config", args: args{ - kb: mkKibana, + client: k8s.WrappedFakeClient(existingSecret), + kb: mkKibana, }, want: defaultConfig, }, { name: "without TLS", args: args{ + client: k8s.WrappedFakeClient(existingSecret), kb: func() kbv1.Kibana { kb := mkKibana() kb.Spec = kbv1.KibanaSpec{ @@ -88,7 +102,7 @@ func TestNewConfigSettings(t *testing.T) { }, want: func() []byte { cfg, err := settings.ParseConfig(defaultConfig) - require.NoError(t, err) + require.NoError(t, err, "cfg", cfg) removed, err := (*ucfg.Config)(cfg).Remove("server.ssl", -1, settings.Options...) require.True(t, removed) require.NoError(t, err) @@ -116,9 +130,11 @@ func TestNewConfigSettings(t *testing.T) { return kb }, client: k8s.WrapClient(fake.NewFakeClient( + existingSecret, &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: "auth-secret", + Name: "auth-secret", + Namespace: mkKibana().Namespace, }, Data: map[string][]byte{ "elastic": []byte("password"), @@ -152,6 +168,7 @@ func TestNewConfigSettings(t *testing.T) { { name: "with user config", args: args{ + client: k8s.WrappedFakeClient(existingSecret), kb: func() kbv1.Kibana { kb := mkKibana() kb.Spec = kbv1.KibanaSpec{ @@ -172,7 +189,7 @@ func TestNewConfigSettings(t *testing.T) { versionSpecificCfg := settings.MustCanonicalConfig(map[string]interface{}{"elasticsearch.hosts": nil}) got, err := NewConfigSettings(tt.args.client, tt.args.kb(), versionSpecificCfg) if tt.wantErr { - require.NotNil(t, err) + require.Error(t, err) } require.NoError(t, err) @@ -192,8 +209,46 @@ func TestNewConfigSettings(t *testing.T) { } } +// TestNewConfigSettingsCreateEncryptionKey checks that we generate a new key if none is specified +func TestNewConfigSettingsCreateEncryptionKey(t *testing.T) { + client := k8s.WrapClient(fake.NewFakeClient()) + kb := mkKibana() + got, err := NewConfigSettings(client, kb, nil) + require.NoError(t, err) + val, err := (*ucfg.Config)(got.CanonicalConfig).String(XpackSecurityEncryptionKey, -1, settings.Options...) + require.NoError(t, err) + assert.NotEmpty(t, val) +} + +// TestNewConfigSettingsExplicitEncryptionKey tests that we do not override the existing key if one is already specified +func TestNewConfigSettingsExplicitEncryptionKey(t *testing.T) { + kb := mkKibana() + existingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(kb), + Namespace: kb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: []byte("xpack.security.encryptionKey: thisismyencryptionkey"), + }, + } + client := k8s.WrapClient(fake.NewFakeClient(existingSecret)) + got, err := NewConfigSettings(client, kb, nil) + require.NoError(t, err) + var gotCfg map[string]interface{} + require.NoError(t, got.Unpack(&gotCfg)) + val, err := (*ucfg.Config)(got.CanonicalConfig).String(XpackSecurityEncryptionKey, -1, settings.Options...) + require.NoError(t, err) + assert.Equal(t, "thisismyencryptionkey", val) +} + func mkKibana() kbv1.Kibana { - kb := kbv1.Kibana{} + kb := kbv1.Kibana{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testkb", + Namespace: "testns", + }, + } return kb } @@ -234,38 +289,34 @@ func Test_getExistingConfig(t *testing.T) { } tests := []struct { - name string - kb kbv1.Kibana - secret corev1.Secret - expectErr bool + name string + kb kbv1.Kibana + secret corev1.Secret + // an empty string means we should expect a nil return, anything else should be a key in the parsed config expectKey string }{ { name: "happy path", kb: testKb, secret: testValidSecret, - expectErr: false, expectKey: "xpack", }, { name: "no secret exists", kb: testKb, secret: corev1.Secret{}, - expectErr: true, expectKey: "", }, { name: "no kibana.yml exists in secret", kb: testKb, secret: testNoYaml, - expectErr: true, expectKey: "", }, { name: "cannot parse yaml", kb: testKb, secret: testInvalidYaml, - expectErr: true, expectKey: "", }, } @@ -273,15 +324,12 @@ func Test_getExistingConfig(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { client := k8s.WrapClient(fake.NewFakeClient(&tc.secret)) - returned, err := getExistingConfig(client, tc.kb) - if tc.expectErr { - require.Error(t, err) + result := getExistingConfig(client, tc.kb) + if tc.expectKey == "" { + assert.Nil(t, result) } else { - require.NoError(t, err) - } - if tc.expectKey != "" { - require.NotNil(t, returned) - assert.True(t, (*ucfg.Config)(returned).HasField("xpack")) + require.NotNil(t, result) + assert.True(t, (*ucfg.Config)(result).HasField(tc.expectKey)) } }) } From d3165778d42fdc81a91103323def08a2a839e467 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Tue, 17 Dec 2019 10:32:01 -0600 Subject: [PATCH 06/12] Update docs --- docs/kibana.asciidoc | 5 +---- pkg/controller/kibana/config/settings.go | 9 --------- pkg/controller/kibana/driver.go | 4 +--- 3 files changed, 2 insertions(+), 16 deletions(-) diff --git a/docs/kibana.asciidoc b/docs/kibana.asciidoc index 2e5a4647fa..5a38416e19 100644 --- a/docs/kibana.asciidoc +++ b/docs/kibana.asciidoc @@ -180,10 +180,7 @@ spec: [id="{p}-kibana-scaling"] === Scale out a Kibana deployment -You may want to deploy more than one instance of Kibana. -In this case all the instances must share the same encryption key. - -This can be done by setting the `xpack.security.encryptionKey` property using a secure setting as described in the next section. +You may want to deploy more than one instance of Kibana. In this case all the instances must share the same encryption key. If you do not set one, the operator will generate one for you. If you would like to set your own encryption key, this can be done by setting the `xpack.security.encryptionKey` property using a secure setting as described in the next section. Note that while most reconfigurations of your Kibana instances will be carried out in rolling upgrade fashion, all version upgrades will cause Kibana downtime. This is due to the link:https://www.elastic.co/guide/en/kibana/current/upgrade.html[requirement] to run only a single version of Kibana at any given time. diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 071bb11c81..6864c73d62 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -37,9 +37,7 @@ type CanonicalConfig struct { } // NewConfigSettings returns the Kibana configuration settings for the given Kibana resource. -// TODO sabo does it belong here? func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { - currentConfig := getExistingConfig(client, kb) specConfig := kb.Spec.Config @@ -150,10 +148,3 @@ func elasticsearchTLSSettings(kb kbv1.Kibana) map[string]interface{} { return cfg } - -// ensureEncryptionKey ensures a given config contains an encryption key -func ensureEncryptionKey(cfg map[string]interface{}) { - if _, ok := cfg[XpackSecurityEncryptionKey]; !ok { - cfg[XpackSecurityEncryptionKey] = rand.String(64) - } -} diff --git a/pkg/controller/kibana/driver.go b/pkg/controller/kibana/driver.go index 9dd3c94911..5b3c721051 100644 --- a/pkg/controller/kibana/driver.go +++ b/pkg/controller/kibana/driver.go @@ -247,11 +247,9 @@ func (d *driver) Reconcile( if results.HasError() { return &results } - // TODO sabo get current secret here? - currentCfg, err := config.GetConfig(d.client, *kb) versionSpecificCfg := settings.MustCanonicalConfig(d.settingsFactory(*kb)) - kbSettings, err := config.NewConfigSettings(d.client, *kb, versionSpecificCfg, currentCfg) + kbSettings, err := config.NewConfigSettings(d.client, *kb, versionSpecificCfg) if err != nil { return results.WithError(err) } From 36731a2bbde53e3c1f028e87592098d690cc51b5 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Tue, 17 Dec 2019 11:53:44 -0600 Subject: [PATCH 07/12] Add test --- pkg/controller/kibana/config/settings_test.go | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index 0d229e6b6f..babe386d7a 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -220,8 +220,8 @@ func TestNewConfigSettingsCreateEncryptionKey(t *testing.T) { assert.NotEmpty(t, val) } -// TestNewConfigSettingsExplicitEncryptionKey tests that we do not override the existing key if one is already specified -func TestNewConfigSettingsExplicitEncryptionKey(t *testing.T) { +// TestNewConfigSettingsExistingEncryptionKey tests that we do not override the existing key if one is already specified +func TestNewConfigSettingsExistingEncryptionKey(t *testing.T) { kb := mkKibana() existingSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -242,6 +242,25 @@ func TestNewConfigSettingsExplicitEncryptionKey(t *testing.T) { assert.Equal(t, "thisismyencryptionkey", val) } +// TestNewConfigSettingsExplicitEncryptionKey tests that we do not override the existing key if one is already specified in the Spec +// this should not be done since it is a secure setting, but just in case it happens we do not want to ignore it +func TestNewConfigSettingsExplicitEncryptionKey(t *testing.T) { + kb := mkKibana() + key := "thisismyencryptionkey" + cfg := commonv1.NewConfig(map[string]interface{}{ + XpackSecurityEncryptionKey: key, + }) + kb.Spec.Config = &cfg + client := k8s.WrapClient(fake.NewFakeClient()) + got, err := NewConfigSettings(client, kb, nil) + require.NoError(t, err) + var gotCfg map[string]interface{} + require.NoError(t, got.Unpack(&gotCfg)) + val, err := (*ucfg.Config)(got.CanonicalConfig).String(XpackSecurityEncryptionKey, -1, settings.Options...) + require.NoError(t, err) + assert.Equal(t, key, val) +} + func mkKibana() kbv1.Kibana { kb := kbv1.Kibana{ ObjectMeta: metav1.ObjectMeta{ From d9ed1e2d269550a6417fc8f7f52f0d98ef97bbc0 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Tue, 17 Dec 2019 12:00:07 -0600 Subject: [PATCH 08/12] Update log constant --- pkg/controller/kibana/config/settings.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 6864c73d62..9a552f3b75 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -22,13 +22,10 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" ) -// Kibana configuration settings file -const ( - SettingsFilename = "kibana.yml" - name = "kibana-config" -) +// SettingsFilename is the name of the Kibana configuration settings file +const SettingsFilename = "kibana.yml" -var log = logf.Log.WithName(name) +var log = logf.Log.WithName("kibana-config") // CanonicalConfig contains configuration for Kibana ("kibana.yml"), // as a hierarchical key-value configuration. From d333cfd454e3761efca5d32cc59077474130c40b Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Wed, 18 Dec 2019 17:23:57 -0600 Subject: [PATCH 09/12] Add test case for existing secret and spec conflict --- pkg/controller/kibana/config/settings.go | 3 ++- pkg/controller/kibana/config/settings_test.go | 26 +++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 9a552f3b75..d441f2244e 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -51,6 +51,7 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se kibanaTLSCfg := settings.MustCanonicalConfig(kibanaTLSSettings(kb)) if !kb.RequiresAssociation() { + // merge the configuration with userSettings last so they take precedence if err := cfg.MergeWith( currentConfig, versionSpecificCfg, @@ -94,7 +95,7 @@ func getExistingConfig(client k8s.Client, kb kbv1.Kibana) *settings.CanonicalCon if err != nil && apierrors.IsNotFound(err) { log.V(1).Info("Kibana config secret does not exist", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) return nil - } else if err != nil && !apierrors.IsNotFound(err) { + } else if err != nil { log.Error(err, "Error retrieving kibana config secret", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) return nil } diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index babe386d7a..be6f3394b8 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -183,6 +183,32 @@ func TestNewConfigSettings(t *testing.T) { }, want: append(defaultConfig, []byte(`foo: bar`)...), }, + { + name: "test existing secret does not prevent updates to config, e.g. spec takes precedence even if there is a secret indicating otherwise", + args: args{ + client: k8s.WrappedFakeClient(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(defaultKb), + Namespace: defaultKb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: []byte("xpack.security.encryptionKey: thisismyencryptionkey\nlogging.verbose: true"), + }, + }), + kb: func() kbv1.Kibana { + kb := mkKibana() + kb.Spec = kbv1.KibanaSpec{ + Config: &commonv1.Config{ + Data: map[string]interface{}{ + "logging.verbose": false, + }, + }, + } + return kb + }, + }, + want: append(defaultConfig, []byte(`logging.verbose: false`)...), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 3c123c17b323d5631ef0b30cae25772db5afe8cd Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Wed, 18 Dec 2019 21:18:05 -0600 Subject: [PATCH 10/12] Add config filter --- .../common/settings/canonical_config.go | 12 ++++++- pkg/controller/kibana/config/settings.go | 26 +++++++++++++-- pkg/controller/kibana/config/settings_test.go | 33 +++++++++++++++++++ 3 files changed, 67 insertions(+), 4 deletions(-) diff --git a/pkg/controller/common/settings/canonical_config.go b/pkg/controller/common/settings/canonical_config.go index 0c5a675bcc..27b57f0569 100644 --- a/pkg/controller/common/settings/canonical_config.go +++ b/pkg/controller/common/settings/canonical_config.go @@ -62,7 +62,7 @@ func MustCanonicalConfig(cfg interface{}) *CanonicalConfig { // MustNewSingleValue creates a new config holding a single string value. // Convenience constructor, will panic in the unlikely event of errors. func MustNewSingleValue(k string, v string) *CanonicalConfig { - cfg := fromConfig(ucfg.New()) + cfg := NewCanonicalConfig() err := cfg.asUCfg().SetString(k, -1, v, Options...) if err != nil { panic(err) @@ -70,6 +70,16 @@ func MustNewSingleValue(k string, v string) *CanonicalConfig { return cfg } +// NewSingleValue creates a new config holding a single string value. +func NewSingleValue(k string, v string) (*CanonicalConfig, error) { + cfg := fromConfig(ucfg.New()) + err := cfg.asUCfg().SetString(k, -1, v, Options...) + if err != nil { + return nil, errors.WithStack(err) + } + return cfg, nil +} + // ParseConfig parses the given configuration content into a CanonicalConfig. // Expects content to be in YAML format. func ParseConfig(yml []byte) (*CanonicalConfig, error) { diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index d441f2244e..839ff890ed 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -15,6 +15,7 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/common/settings" "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" + "github.com/elastic/go-ucfg" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" @@ -36,7 +37,7 @@ type CanonicalConfig struct { // NewConfigSettings returns the Kibana configuration settings for the given Kibana resource. func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { currentConfig := getExistingConfig(client, kb) - + filteredCurrCfg := filterExistingConfig(currentConfig) specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1.Config{} @@ -53,7 +54,7 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se if !kb.RequiresAssociation() { // merge the configuration with userSettings last so they take precedence if err := cfg.MergeWith( - currentConfig, + filteredCurrCfg, versionSpecificCfg, kibanaTLSCfg, userSettings); err != nil { @@ -69,7 +70,7 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se // merge the configuration with userSettings last so they take precedence err = cfg.MergeWith( - currentConfig, + filteredCurrCfg, versionSpecificCfg, kibanaTLSCfg, settings.MustCanonicalConfig(elasticsearchTLSSettings(kb)), @@ -112,6 +113,25 @@ func getExistingConfig(client k8s.Client, kb kbv1.Kibana) *settings.CanonicalCon return cfg } +// filterExistingConfig filters an existing config for only items we want to preserve between spec changes +// because they cannot be generated deterministically, e.g. encryption keys +func filterExistingConfig(cfg *settings.CanonicalConfig) *settings.CanonicalConfig { + if cfg == nil { + return nil + } + val, err := (*ucfg.Config)(cfg).String(XpackSecurityEncryptionKey, -1, settings.Options...) + if err != nil { + log.V(1).Info("Current config does not contain key", "key", XpackSecurityEncryptionKey, "error", err) + return nil + } + filteredCfg, err := settings.NewSingleValue(XpackSecurityEncryptionKey, val) + if err != nil { + log.Error(err, "Error filtering current config") + return nil + } + return filteredCfg +} + func baseSettings(kb kbv1.Kibana) map[string]interface{} { return map[string]interface{}{ ServerName: kb.Name, diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index be6f3394b8..6960e6a0b7 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -209,6 +209,25 @@ func TestNewConfigSettings(t *testing.T) { }, want: append(defaultConfig, []byte(`logging.verbose: false`)...), }, + { + name: "test existing secret does not prevent removing items from config in spec", + args: args{ + client: k8s.WrappedFakeClient(&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: SecretName(defaultKb), + Namespace: defaultKb.Namespace, + }, + Data: map[string][]byte{ + SettingsFilename: []byte("xpack.security.encryptionKey: thisismyencryptionkey\nlogging.verbose: true"), + }, + }), + kb: func() kbv1.Kibana { + kb := mkKibana() + return kb + }, + }, + want: append(defaultConfig), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -379,3 +398,17 @@ func Test_getExistingConfig(t *testing.T) { }) } } + +func Test_filterExistingConfig(t *testing.T) { + cfg, err := settings.NewCanonicalConfigFrom(map[string]interface{}{ + XpackSecurityEncryptionKey: "value", + "notakey": "notavalue", + }) + require.NoError(t, err) + want, err := settings.NewCanonicalConfigFrom(map[string]interface{}{ + XpackSecurityEncryptionKey: "value", + }) + require.NoError(t, err) + filtered := filterExistingConfig(cfg) + assert.Equal(t, want, filtered) +} From 417cac3f5f954e2465140a8b41f6c05f49a88449 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Fri, 20 Dec 2019 08:25:35 -0600 Subject: [PATCH 11/12] Correct log keys --- pkg/controller/kibana/config/settings.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 839ff890ed..73a337ced9 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -94,20 +94,20 @@ func getExistingConfig(client k8s.Client, kb kbv1.Kibana) *settings.CanonicalCon var secret corev1.Secret err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) if err != nil && apierrors.IsNotFound(err) { - log.V(1).Info("Kibana config secret does not exist", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) + log.V(1).Info("Kibana config secret does not exist", "namespace", kb.Namespace, "kibana_name", kb.Name) return nil } else if err != nil { - log.Error(err, "Error retrieving kibana config secret", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name) + log.Error(err, "Error retrieving kibana config secret", "namespace", kb.Namespace, "kibana_name", kb.Name) return nil } rawCfg, exists := secret.Data[SettingsFilename] if !exists { - log.Error(nil, "No kibana config file in secret", "secret_namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) + log.Error(nil, "No kibana config file in secret", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) return nil } cfg, err := settings.ParseConfig(rawCfg) if err != nil { - log.Error(err, "Error parsing existing kibana config in secret", "secret_namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) + log.Error(err, "Error parsing existing kibana config in secret", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) return nil } return cfg From 767047a36517cf3da6440f97f25813a59ff7f4f8 Mon Sep 17 00:00:00 2001 From: Anya Sabo Date: Fri, 20 Dec 2019 18:02:37 -0600 Subject: [PATCH 12/12] Address feedback --- .../common/settings/canonical_config.go | 2 +- pkg/controller/kibana/config/settings.go | 39 +++++++------ pkg/controller/kibana/config/settings_test.go | 57 ++++++++++++++----- 3 files changed, 67 insertions(+), 31 deletions(-) diff --git a/pkg/controller/common/settings/canonical_config.go b/pkg/controller/common/settings/canonical_config.go index 27b57f0569..0e3bf0ba7e 100644 --- a/pkg/controller/common/settings/canonical_config.go +++ b/pkg/controller/common/settings/canonical_config.go @@ -60,7 +60,7 @@ func MustCanonicalConfig(cfg interface{}) *CanonicalConfig { } // MustNewSingleValue creates a new config holding a single string value. -// Convenience constructor, will panic in the unlikely event of errors. +// It is NewSingleValue but panics rather than returning errors, largely used for convenience in tests func MustNewSingleValue(k string, v string) *CanonicalConfig { cfg := NewCanonicalConfig() err := cfg.asUCfg().SetString(k, -1, v, Options...) diff --git a/pkg/controller/kibana/config/settings.go b/pkg/controller/kibana/config/settings.go index 73a337ced9..4798665d53 100644 --- a/pkg/controller/kibana/config/settings.go +++ b/pkg/controller/kibana/config/settings.go @@ -5,8 +5,6 @@ package config import ( - "path" - commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1" kbv1 "github.com/elastic/cloud-on-k8s/pkg/apis/kibana/v1" "github.com/elastic/cloud-on-k8s/pkg/controller/common/association" @@ -16,10 +14,12 @@ import ( "github.com/elastic/cloud-on-k8s/pkg/controller/kibana/es" "github.com/elastic/cloud-on-k8s/pkg/utils/k8s" "github.com/elastic/go-ucfg" + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/rand" + "path" logf "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -36,8 +36,14 @@ type CanonicalConfig struct { // NewConfigSettings returns the Kibana configuration settings for the given Kibana resource. func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *settings.CanonicalConfig) (CanonicalConfig, error) { - currentConfig := getExistingConfig(client, kb) - filteredCurrCfg := filterExistingConfig(currentConfig) + currentConfig, err := getExistingConfig(client, kb) + if err != nil { + return CanonicalConfig{}, err + } + filteredCurrCfg, err := filterExistingConfig(currentConfig) + if err != nil { + return CanonicalConfig{}, err + } specConfig := kb.Spec.Config if specConfig == nil { specConfig = &commonv1.Config{} @@ -90,46 +96,47 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se } // getExistingConfig retrieves the canonical config for a given Kibana, if one exists -func getExistingConfig(client k8s.Client, kb kbv1.Kibana) *settings.CanonicalConfig { +func getExistingConfig(client k8s.Client, kb kbv1.Kibana) (*settings.CanonicalConfig, error) { var secret corev1.Secret err := client.Get(types.NamespacedName{Name: SecretName(kb), Namespace: kb.Namespace}, &secret) if err != nil && apierrors.IsNotFound(err) { log.V(1).Info("Kibana config secret does not exist", "namespace", kb.Namespace, "kibana_name", kb.Name) - return nil + return nil, nil } else if err != nil { log.Error(err, "Error retrieving kibana config secret", "namespace", kb.Namespace, "kibana_name", kb.Name) - return nil + return nil, err } rawCfg, exists := secret.Data[SettingsFilename] if !exists { - log.Error(nil, "No kibana config file in secret", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) - return nil + err = errors.New("Kibana config secret exists but missing config file key") + log.Error(err, "", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) + return nil, err } cfg, err := settings.ParseConfig(rawCfg) if err != nil { log.Error(err, "Error parsing existing kibana config in secret", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) - return nil + return nil, err } - return cfg + return cfg, nil } // filterExistingConfig filters an existing config for only items we want to preserve between spec changes // because they cannot be generated deterministically, e.g. encryption keys -func filterExistingConfig(cfg *settings.CanonicalConfig) *settings.CanonicalConfig { +func filterExistingConfig(cfg *settings.CanonicalConfig) (*settings.CanonicalConfig, error) { if cfg == nil { - return nil + return nil, nil } val, err := (*ucfg.Config)(cfg).String(XpackSecurityEncryptionKey, -1, settings.Options...) if err != nil { log.V(1).Info("Current config does not contain key", "key", XpackSecurityEncryptionKey, "error", err) - return nil + return nil, nil } filteredCfg, err := settings.NewSingleValue(XpackSecurityEncryptionKey, val) if err != nil { log.Error(err, "Error filtering current config") - return nil + return nil, err } - return filteredCfg + return filteredCfg, nil } func baseSettings(kb kbv1.Kibana) map[string]interface{} { diff --git a/pkg/controller/kibana/config/settings_test.go b/pkg/controller/kibana/config/settings_test.go index 6960e6a0b7..bafd55daa3 100644 --- a/pkg/controller/kibana/config/settings_test.go +++ b/pkg/controller/kibana/config/settings_test.go @@ -358,40 +358,50 @@ func Test_getExistingConfig(t *testing.T) { secret corev1.Secret // an empty string means we should expect a nil return, anything else should be a key in the parsed config expectKey string + expectErr bool }{ { name: "happy path", kb: testKb, secret: testValidSecret, expectKey: "xpack", + expectErr: false, }, { name: "no secret exists", kb: testKb, secret: corev1.Secret{}, expectKey: "", + expectErr: false, }, { name: "no kibana.yml exists in secret", kb: testKb, secret: testNoYaml, expectKey: "", + expectErr: true, }, { name: "cannot parse yaml", kb: testKb, secret: testInvalidYaml, expectKey: "", + expectErr: true, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { client := k8s.WrapClient(fake.NewFakeClient(&tc.secret)) - result := getExistingConfig(client, tc.kb) - if tc.expectKey == "" { - assert.Nil(t, result) + result, err := getExistingConfig(client, tc.kb) + if tc.expectErr { + require.Error(t, err) } else { + require.NoError(t, err) + } + + if tc.expectKey != "" { + require.NotNil(t, result) assert.True(t, (*ucfg.Config)(result).HasField(tc.expectKey)) } @@ -400,15 +410,34 @@ func Test_getExistingConfig(t *testing.T) { } func Test_filterExistingConfig(t *testing.T) { - cfg, err := settings.NewCanonicalConfigFrom(map[string]interface{}{ - XpackSecurityEncryptionKey: "value", - "notakey": "notavalue", - }) - require.NoError(t, err) - want, err := settings.NewCanonicalConfigFrom(map[string]interface{}{ - XpackSecurityEncryptionKey: "value", - }) - require.NoError(t, err) - filtered := filterExistingConfig(cfg) - assert.Equal(t, want, filtered) + tests := []struct { + name string + cfg *settings.CanonicalConfig + want *settings.CanonicalConfig + expectErr bool + }{ + { + name: "happy path", + cfg: settings.MustCanonicalConfig(map[string]interface{}{ + XpackSecurityEncryptionKey: "value", + "notakey": "notavalue", + }), + want: settings.MustCanonicalConfig(map[string]interface{}{ + XpackSecurityEncryptionKey: "value", + }), + expectErr: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + actual, err := filterExistingConfig(tc.cfg) + if tc.expectErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + assert.Equal(t, tc.want, actual) + }) + } }