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.

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

[config] stop merging arrays from multiple configs when using the dev cli #162842

Closed
mistic opened this issue Jul 31, 2023 · 4 comments · Fixed by #163928
Closed

[config] stop merging arrays from multiple configs when using the dev cli #162842

mistic opened this issue Jul 31, 2023 · 4 comments · Fixed by #163928
Assignees
Labels
Team:Operations Team label for Operations Team

Comments

@mistic
Copy link
Member

mistic commented Jul 31, 2023

Pierre merged #161884 in order to fix this when reading from raw config files.
However in dev cli this problem is still happening. We've tracked it down to this line https://github.com/elastic/kibana/blob/main/src/cli/serve/serve.js#L138 which is doing the merge via a simple lodash.merge call. The fix for that would be to use the same merge function / logic implemented in the mentioned PR above.

The problem could be replicated by using this config file https://github.com/elastic/kibana/blob/main/config/serverless.security.yml#L11 and then the following start dev command:

node scripts/kibana --dev --no-dev-config --no-dev-credentials --serverless=security --csp.strict=false --csp.warnLegacyBrowsers=false --elasticsearch.password=changeme --elasticsearch.username=kibana_system --env.name=development --logging.appenders.deprecation={"type":"console","layout":{"type":"json"}} --logging.loggers=[{"name":"elasticsearch.deprecation","level":"all","appenders":["deprecation"]}] --xpack.securitySolutionServerless.productTypes=[{"product_line":"security","product_tier":"essentials"}]
@delanni
Copy link
Contributor

delanni commented Aug 15, 2023

Found a fairly simple way of solving this by a customizer (https://lodash.com/docs/#mergeWith) function - #163928

delanni added a commit that referenced this issue Aug 15, 2023
## Summary
Change config merging behaviour, so that arrays are not
merged/concatenated but replaced.

Closes: #162842 

Related to: #161884

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Aug 15, 2023
## Summary
Change config merging behaviour, so that arrays are not
merged/concatenated but replaced.

Closes: elastic#162842

Related to: elastic#161884

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 819d304)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Aug 15, 2023
## Summary
Change config merging behaviour, so that arrays are not
merged/concatenated but replaced.

Closes: elastic#162842

Related to: elastic#161884

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 819d304)
hop-dev pushed a commit to hop-dev/kibana that referenced this issue Aug 16, 2023
## Summary
Change config merging behaviour, so that arrays are not
merged/concatenated but replaced.

Closes: elastic#162842 

Related to: elastic#161884

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
bryce-b pushed a commit that referenced this issue Aug 22, 2023
## Summary
Change config merging behaviour, so that arrays are not
merged/concatenated but replaced.

Closes: #162842 

Related to: #161884

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@delanni
Copy link
Contributor

delanni commented Sep 6, 2023

@mistic can you help me decide if we want to backport this to older kibana versions? I think I was afraid to backport it because this is breaking the CLI's behavior, which is a public API of sorts.

@mistic
Copy link
Member Author

mistic commented Sep 7, 2023

@delanni I think we can backport this at least to 8.10 as the original PR from Pierre is on that branch

@delanni
Copy link
Contributor

delanni commented Sep 8, 2023

I think main was on 8.10 at the time when I merged this, so we're good. I just checked and the code is there in 8.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Operations Team label for Operations Team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants