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.

Sign up for GitHub

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

Ensure Kibana encryption key is specified #2278

Merged
merged 20 commits into from
Dec 30, 2019
Merged

Ensure Kibana encryption key is specified #2278

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f5a8cf7
WIP
anyasabo Dec 6, 2019
9382da0
Merge branch 'master' into kibanakey
anyasabo Dec 6, 2019
cfcbb3a
WIP
anyasabo Dec 6, 2019
7939dc1
Merge master
anyasabo Dec 10, 2019
0eb2530
Merge branch 'master' into kibanakey
anyasabo Dec 12, 2019
c17fefa
WIP
anyasabo Dec 12, 2019
b0c6b56
Merge branch 'master' into kibanakey
anyasabo Dec 13, 2019
fc9b85d
WIP
anyasabo Dec 16, 2019
971c1b4
Merge branch 'master' into kibanakey
anyasabo Dec 16, 2019
a60dc2e
Add tests
anyasabo Dec 17, 2019
d316577
Update docs
anyasabo Dec 17, 2019
a71842a
Merge branch 'master' into kibanakey
anyasabo Dec 17, 2019
36731a2
Add test
anyasabo Dec 17, 2019
d9ed1e2
Update log constant
anyasabo Dec 17, 2019
0ba45f1
Merge master
anyasabo Dec 18, 2019
d333cfd
Add test case for existing secret and spec conflict
anyasabo Dec 18, 2019
3c123c1
Add config filter
anyasabo Dec 19, 2019
417cac3
Correct log keys
anyasabo Dec 20, 2019
767047a
Address feedback
anyasabo Dec 21, 2019
0b9fc95
Merge master
anyasabo Dec 21, 2019
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
5 changes: 1 addition & 4 deletions docs/kibana.asciidoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
12 changes: 11 additions & 1 deletion pkg/controller/common/settings/canonical_config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,24 @@ 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)
}
return cfg
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like MustNewSingleValue can now just be a call to NewSingleValue + panic on error?


// 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) {
Expand Down
1 change: 1 addition & 0 deletions pkg/controller/kibana/config/fields.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
65 changes: 63 additions & 2 deletions pkg/controller/kibana/config/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,19 @@ 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"
"k8s.io/apimachinery/pkg/util/rand"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

// Kibana configuration settings file
// SettingsFilename is the name of the Kibana configuration settings file
const SettingsFilename = "kibana.yml"

var log = logf.Log.WithName("kibana-config")

// CanonicalConfig contains configuration for Kibana ("kibana.yml"),
// as a hierarchical key-value configuration.
type CanonicalConfig struct {
Expand All @@ -28,6 +36,8 @@ 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{}
Expand All @@ -42,7 +52,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(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the following happen?

  1. user sets some foo:bar setting in their Kibana manifest
  2. that setting is now part of the secret
  3. they remove it from their Kibana manifest
  4. we still inherit it because we merge from the existing secret

Copy link
Contributor Author

@anyasabo anyasabo Dec 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but that seems okay? Deleting the secret will re-generate the secret with a new encryption key, so there at least is a way to do that if that is the goal.

Copy link
Contributor

@sebgl sebgl Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What I meant was this also apply to any other settings.
If I set:

apiVersion: kibana.k8s.elastic.co/v1
kind: Kibana
metadata:
  name: kibana-sample
spec:
  version: 7.5.0
  count: 1
  elasticsearchRef:
    name: "elasticsearch-sample"
  config:
    logging.verbose: true # temporarily enable verbose logging

Then 1 hour later once I'm done with looking at Kibana logs I remove my extra config and reapply the Kibana manifest without verbose logging enabled:

apiVersion: kibana.k8s.elastic.co/v1
kind: Kibana
metadata:
  name: kibana-sample
spec:
  version: 7.5.0
  count: 1
  elasticsearchRef:
    name: "elasticsearch-sample"

I think the operator code would still merge the new empty user-provided config with the current config which does have logging.verbose: true and that's not what I would expect.

Please correct me if I'm wrong, I may be misreading the way we do the config merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like we have two setting types: for some we want to always flow from the resource (like logging.verbose) and apply our default to it and for others we want to persist whatever was generated the first time (like the xpack.security.encryptionKey).

Should we just get the specific settings that we know should be persisted from the secret?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that's a good point and a big miss on my part Seb, good catch.

Should we just get the specific settings that we know should be persisted from the secret?

I think this makes sense.

filteredCurrCfg,
versionSpecificCfg,
kibanaTLSCfg,
userSettings); err != nil {
return CanonicalConfig{}, err
}
return CanonicalConfig{cfg}, nil
Expand All @@ -55,6 +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(
filteredCurrCfg,
versionSpecificCfg,
kibanaTLSCfg,
settings.MustCanonicalConfig(elasticsearchTLSSettings(kb)),
Expand All @@ -73,12 +89,57 @@ 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 {
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)
anyasabo marked this conversation as resolved.
Show resolved Hide resolved
return nil
} else if err != nil {
log.Error(err, "Error retrieving kibana config secret", "kibana_namespace", kb.Namespace, "kibana_name", kb.Name)
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect we return the error here. Otherwise it means a failing Get call here (network reason or whatever) could end up with a successful Update later, and unexpected settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm that makes sense. I'll change it to return an error here and exit the reconcile loop

}
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)
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
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we end up erroring out above, we might also error out on those 2.

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,
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),
anyasabo marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
Loading