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

Enable the Cloud Security Posture Kibana plugin #767

Merged
merged 14 commits into from
Apr 5, 2022
Merged

Enable the Cloud Security Posture Kibana plugin #767

merged 14 commits into from
Apr 5, 2022

Conversation

eyalkraft
Copy link
Contributor

@eyalkraft eyalkraft commented Mar 30, 2022

Enable the Cloud Security Posture Kibana plugin by default when using elastic-package.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Mar 30, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-04-05T14:32:18.738+0000

  • Duration: 26 min 14 sec

Test stats 🧪

Test Results
Failed 0
Passed 658
Skipped 0
Total 658

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@eyalkraft
Copy link
Contributor Author

Maybe this fails because of dependency with this recent change in Elasticsearch?

I believe the Elasticsearch SNAPSHOT binaries don't include this change yet.

@mtojek mtojek requested a review from a team March 30, 2022 15:01
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Could you please fill in the issue description and link to the original issue?

Please take a look at the build artifacts. It looks like Kibana doesn't support it:

Attaching to elastic-package-stack_kibana_1
�[36mkibana_1                     |�[0m [2022-03-30T14:42:56.139+00:00][INFO ][plugins-service] Plugin "metricsEntities" is disabled.
�[36mkibana_1                     |�[0m [2022-03-30T14:42:56.207+00:00][FATAL][root] Error: Unknown configuration key(s): "xpack.cloudSecurityPosture.enabled". Check for spelling errors and ensure that expected plugins are installed.
�[36mkibana_1                     |�[0m     at ensureValidConfiguration (/usr/share/kibana/src/core/server/config/ensure_valid_configuration.js:35:11)
�[36mkibana_1                     |�[0m     at Server.preboot (/usr/share/kibana/src/core/server/server.js:164:5)
�[36mkibana_1                     |�[0m     at Root.preboot (/usr/share/kibana/src/core/server/root/index.js:48:14)
�[36mkibana_1                     |�[0m     at bootstrap (/usr/share/kibana/src/core/server/bootstrap.js:99:9)
�[36mkibana_1                     |�[0m     at Command.<anonymous> (/usr/share/kibana/src/cli/serve/serve.js:216:5)
�[36mkibana_1                     |�[0m 
�[36mkibana_1                     |�[0m  FATAL  Error: Unknown configuration key(s): "xpack.cloudSecurityPosture.enabled". Check for spelling errors and ensure that expected plugins are installed.
�[36mkibana_1                     |�[0m 

Is it generally available in 8.x?

@eyalkraft
Copy link
Contributor Author

Hey, It's not GA yet (but merged to kibana/main).
We plan on releasing it as beta with 8.3

Kibana 8.2-SNAPSHOT should contain the plugin I believe

@mtojek
Copy link
Contributor

mtojek commented Mar 30, 2022

Kibana 8.2-SNAPSHOT should contain the plugin I believe

I can confirm that 8.2 stack booted up without issues.

That complicates things as we need to support another Kibana config variant. I guess we can help with this one. Sadly Kibana can't ignore unknown configuration properties.

Please tell me how urgent is this change as we have other stuff on the board?

@eyalkraft
Copy link
Contributor Author

Is this a blocker for merging the integration?
If so, it is urgent for us. We can open the PR to support the additional variant if you are short in time - so you will only need to review.

If this is not a blocker for merging the integration, We can work with elastic-package just fine configuring the kibana.yml in our own local profiles.
This flag is only required for installing our integration.

@mtojek
Copy link
Contributor

mtojek commented Mar 31, 2022

Then it's a blocker as correct installation of the integration is required as part of the validation process. If you have spare time to work on this, then please assign us as reviewers.

AFAIR here are the required steps:

  1. Prepare an additional config file based on kibana_config_8x.yml, for example kibana_config_80.yml.
  2. Register the file in here.
  3. Add new entries to managedProfileFiles.
    This map keeps tracking of all configs and it's used to create files in ~/.elastic-package/profiles. Probably you will have to add virtual records for any non-existing 8.0 configs too. We don't want to duplicate config files for Elasticsearch and Elastic Agent in the source code. You can find all config variants here by searching for STACK_VERSION_VARIANT.

As we can see, the CI failed already for many steps but succeeded for 8x, as of now it's 8.2.0-SNAPSHOT, which means that it can boot up correctly with the plugin. We just need to strip the entry from the other Kibana config.

@mtojek mtojek marked this pull request as draft March 31, 2022 07:44
@eyalkraft eyalkraft self-assigned this Mar 31, 2022
@eyalkraft
Copy link
Contributor Author

/test

@eyalkraft eyalkraft marked this pull request as ready for review April 4, 2022 13:36
@eyalkraft
Copy link
Contributor Author

  • Added internal/profile/_static/kibana_config_80.yml file for stack versions 8.0 to 8.1
  • Registered the new file (and virtual files for elasticsearch and elastic-agent) in profile/static.go
  • Added the new entries to managedProfileFiles in profile/profile.go
  • Modified stack/variants.go to support the additional variant
  • Added stack/variants_test.go to validate and clarify the behavior of stack/variants.go

@eyalkraft eyalkraft requested a review from mtojek April 4, 2022 13:44
@mtojek mtojek requested a review from a team April 4, 2022 15:15
Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

Left a small comment around selectStackVersion. Otherwise, it's ready for shipping. Well done!

Comment on lines 22 to 27
if strings.HasPrefix(version, "8.") {
if len(version) > 2 && (int(version[2])-'0') < 2 {
return "80"
}
return "8x"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that you can also parse the version as a semver version (the library is already used in the project) and check the major and minor. It might be more to catch :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!
I wasn't too comfortable with this as well.

@eyalkraft eyalkraft requested a review from mtojek April 5, 2022 07:01
@@ -6,20 +6,34 @@ package stack

import (
"fmt"
"strings"

"github.com/Masterminds/semver/v3"
Copy link
Contributor

Choose a reason for hiding this comment

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

just a small comment, I see you imported the "v3". Is there anything missing in the github.com/Masterminds/semver as we are using the base release? If not, would you mind switching to that version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
I had some trouble finding the correct v1 syntax for the range constraint as it differs from the v3 a bit.

if strings.HasPrefix(version, "8.") {
return "8x"
if v, err := semver.NewVersion(version); err == nil {
if checkVersion(v, "8.0-0 - 8.1-0") {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

much clearer now!

func stackVariantAsEnv(version string) string {
return fmt.Sprintf("STACK_VERSION_VARIANT=%s", selectStackVersion(version))
}

func selectStackVersion(version string) string {
if strings.HasPrefix(version, "8.") {
return "8x"
if v, err := semver.NewVersion(version); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, you can convert these if-conditions into a configurationVariantMap (key: constraint, value: config variant).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@eyalkraft eyalkraft requested a review from mtojek April 5, 2022 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants