Ensure Kibana encryption key is specified #2278
Conversation
| @@ -42,7 +54,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( | |||
There was a problem hiding this comment.
Can the following happen?
- user sets some
foo:barsetting in their Kibana manifest - that setting is now part of the secret
- they remove it from their Kibana manifest
- we still inherit it because we merge from the existing secret
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 loggingThen 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -42,7 +54,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( | |||
There was a problem hiding this comment.
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?
|
Confirmed via experimentation and conversations with the Kibana team that the priority order for settings is CLI > secure settings > yaml. For example, a user specifying a secure setting as currently recommended overrides whatever we set in the kibana.yml. So we should be good there as is. I will think about how to solve the issue with not overwriting the config even when we want to (probably by only extracting specific keys from the existing config) and update the PR. |
|
This is ready for another look please |
| @@ -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 | |||
| } | |||
There was a problem hiding this comment.
Looks like MustNewSingleValue can now just be a call to NewSingleValue + panic on error?
| return nil | ||
| } else if err != nil { | ||
| log.Error(err, "Error retrieving kibana config secret", "namespace", kb.Namespace, "kibana_name", kb.Name) | ||
| return nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmmm that makes sense. I'll change it to return an error here and exit the reconcile loop
| if err != nil { | ||
| log.Error(err, "Error parsing existing kibana config in secret", "namespace", secret.Namespace, "secret_name", secret.Name, "key", SettingsFilename) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
If we end up erroring out above, we might also error out on those 2.
Fixes #1738
Reads the existing Kibana config secret (if any) and re-uses that, otherwise we generate a new encryption key.
I tested and it does not seem to break our current guidance of using a secure settings secret. Kibana seems a lot looser than Elasticsearch in this regard. So users following the current standard should be able to upgrade seamlessly (though with a restart since the config changed). We should add something to the release notes mentioning it is not necessary.