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/common/settings/canonical_config.go b/pkg/controller/common/settings/canonical_config.go index 0c5a675bcc..0e3bf0ba7e 100644 --- a/pkg/controller/common/settings/canonical_config.go +++ b/pkg/controller/common/settings/canonical_config.go @@ -60,9 +60,9 @@ 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 := 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/fields.go b/pkg/controller/kibana/config/fields.go index 9e12cfea6b..cbdffcfe15 100644 --- a/pkg/controller/kibana/config/fields.go +++ b/pkg/controller/kibana/config/fields.go @@ -11,6 +11,7 @@ const ( ElasticSearchHosts = "elasticsearch.hosts" XpackMonitoringUiContainerElasticsearchEnabled = "xpack.monitoring.ui.container.elasticsearch.enabled" XpackLicenseManagementUIEnabled = "xpack.license_management.ui.enabled" // >= 7.6 + 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 31287cb7d1..5eab200236 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" @@ -15,15 +13,24 @@ 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" + "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" ) const ( - // Kibana configuration settings file + // SettingsFilename is the Kibana configuration settings file SettingsFilename = "kibana.yml" - // Environment variable name for the Node options that can be used to increase the Kibana maximum memory limit + // EnvNodeOpts is the environment variable name for the Node options that can be used to increase the Kibana maximum memory limit EnvNodeOpts = "NODE_OPTS" ) +var log = logf.Log.WithName("kibana-config") // CanonicalConfig contains configuration for Kibana ("kibana.yml"), // as a hierarchical key-value configuration. @@ -33,6 +40,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, 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{} @@ -47,7 +62,12 @@ 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 { + // merge the configuration with userSettings last so they take precedence + if err := cfg.MergeWith( + filteredCurrCfg, + versionSpecificCfg, + kibanaTLSCfg, + userSettings); err != nil { return CanonicalConfig{}, err } return CanonicalConfig{cfg}, nil @@ -60,6 +80,7 @@ func NewConfigSettings(client k8s.Client, kb kbv1.Kibana, versionSpecificCfg *se // merge the configuration with userSettings last so they take precedence err = cfg.MergeWith( + filteredCurrCfg, versionSpecificCfg, kibanaTLSCfg, settings.MustCanonicalConfig(elasticsearchTLSSettings(kb)), @@ -78,12 +99,58 @@ 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 + 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, nil + } else if err != nil { + log.Error(err, "Error retrieving kibana config secret", "namespace", kb.Namespace, "kibana_name", kb.Name) + return nil, err + } + rawCfg, exists := secret.Data[SettingsFilename] + if !exists { + 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, err + } + 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, error) { + if cfg == 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, nil + } + filteredCfg, err := settings.NewSingleValue(XpackSecurityEncryptionKey, val) + if err != nil { + log.Error(err, "Error filtering current config") + return nil, err + } + return filteredCfg, nil +} + func baseSettings(kb kbv1.Kibana) map[string]interface{} { return map[string]interface{}{ ServerName: kb.Name, 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 9d1dcd941e..bafd55daa3 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" @@ -26,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: @@ -51,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 @@ -64,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{ @@ -87,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) @@ -115,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"), @@ -151,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{ @@ -165,13 +183,58 @@ 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`)...), + }, + { + 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) { 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) @@ -191,7 +254,190 @@ 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) +} + +// 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{ + 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) +} + +// 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{} + kb := kbv1.Kibana{ + ObjectMeta: metav1.ObjectMeta{ + Name: "testkb", + Namespace: "testns", + }, + } 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 + // 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, 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)) + } + }) + } +} + +func Test_filterExistingConfig(t *testing.T) { + 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) + }) + } +}