Fix: use correct index template name for hidden data streams #673

aleksmaus · 2022-01-27T19:39:20Z

Fix elastic-package where it was not correctly deriving the index template name for the hidden datastreams
and failing in CI:

For example PR elastic/integrations#2563

resulting in error:

asset test: index_template logs-osquery_manager.actions is loaded – osquery_manager.actions
no error details
Expand to view the stacktrace
 could not find expected asset: could not find index_template asset "logs-osquery_manager.actions". Assets loaded:
- .logs-osquery_manager.action_results (type: index_template)
- .logs-osquery_manager.action_results@mappings (type: component_template)
- .logs-osquery_manager.action_results@settings (type: component_template)
- .logs-osquery_manager.action_results@custom (type: component_template)
- .logs-osquery_manager.actions (type: index_template)
- .logs-osquery_manager.actions@mappings (type: component_template)
- .logs-osquery_manager.actions@settings (type: component_template)
- .logs-osquery_manager.actions@custom (type: component_template)
- logs-osquery_manager.result (type: index_template)
- logs-osquery_manager.result@mappings (type: component_template)
- logs-osquery_manager.result@settings (type: component_template)
- logs-osquery_manager.result@custom (type: component_template)

Screenshot after this change, showing it passes the tests now:

elasticmachine · 2022-01-27T20:08:03Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Reason: null
Start Time: 2022-01-28T14:18:55.853+0000
Duration: 29 min 7 sec
Commit: f0dbcca

Test stats 🧪

Test	Results
Failed	0
Passed	471
Skipped	0
Total	471

🤖 GitHub comments

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

/test : Re-trigger the build.

mtojek · 2022-01-27T21:36:17Z

internal/packages/packages.go


 // IndexTemplateName returns the name of the Elasticsearch index template that would be installed
 // for this data stream.
 func (dsm *DataStreamManifest) IndexTemplateName(pkgName string) string {


As this isn't a standard, straightforward logic, would you mind adding some unit tests?

Sure will do

mtojek · 2022-01-27T21:39:05Z

internal/packages/packages.go

 func (dsm *DataStreamManifest) IndexTemplateName(pkgName string) string {
 	if dsm.Dataset == "" {
-		return fmt.Sprintf("%s-%s.%s", dsm.Type, pkgName, dsm.Name)
+		return dsm.IndexTemplateNamePrefix() + fmt.Sprintf("%s-%s.%s", dsm.Type, pkgName, dsm.Name)


What would be the value for a hidden data stream without dataset defined? .logs-aPackage.aDataStream?

Not 100% sure what it suppose to be, but currently the kibana behavour is different from the elatic-package validation. Kibana creates templates with "." dot prefix for the package with hidden datastreams.
As far as I understand the consistent behavior would be in both cases with or without dataset name to have hidden property to influence the template name.

Not 100% sure what it suppose to be,

So do I :) I'm not if it's a bug or works as intended. @ruflin @joshdover Could you please confirm the index template name ("hidden" case)?

So this pre-dates my time on Fleet, but this definitely appears to be intentional. We have this function in Kibana which is used for all asset naming:

https://github.com/elastic/kibana/blob/0ee514b1fe044cdaca253f8f14988fc17635fed3/x-pack/plugins/fleet/server/services/epm/elasticsearch/index.ts#L10-L18

It appears it was added as part of elastic/kibana#86277 over a year ago, though I don't see any linked package-spec changes. Maybe @kevinlog can provide more context.

Thanks for diving into code, Josh! @aleksmaus I think you can adjust function implementation to look similar (baseType vs .baseType) and please hard-link to Kibana in the comment.

Thanks for looking into this. Will update and add comment. Endpoint didn't have issues passing this validation in CI because it lives in a separate repo, maybe skipping this test.

Took a look at kibana code, it looks consistent with this change. Added a comment. Let me know.

mtojek

Left one comment

mtojek · 2022-01-28T14:03:16Z

internal/packages/packages.go

+	return dsm.IndexTemplateNamePrefix() + fmt.Sprintf("%s-%s", dsm.Type, dsm.Dataset)
+}
+
+func (dsm *DataStreamManifest) IndexTemplateNamePrefix() string {


nit: does it have to be exposed?

valid point. doesn't have to.

mtojek · 2022-01-28T14:08:29Z

internal/packages/packages.go

 	}

-	return fmt.Sprintf("%s-%s", dsm.Type, dsm.Dataset)
+	return dsm.IndexTemplateNamePrefix() + fmt.Sprintf("%s-%s", dsm.Type, dsm.Dataset)


I think you can refactor it into:fmt.Sprintf("%s%s-%s", dsm.optionalIndexTemplateNamePrefix(), dsm.Type, dsm.Dataset) to prevent concatenation and fmt.Sprintf separately.

or maybe:

dsm.withIndexTemplateNamePrefix(fmt.Sprintf(...))

Correctly process the hidden property in datastream definition

4336d71

aleksmaus requested review from andrewkroh, james-elastic, ruflin and ycombinator January 27, 2022 19:39

mtojek requested review from a team and removed request for ycombinator January 27, 2022 21:34

mtojek reviewed Jan 27, 2022

View reviewed changes

Add unit test coverage

2224cb1

aleksmaus requested a review from mtojek January 27, 2022 22:27

mtojek reviewed Jan 28, 2022

View reviewed changes

Add comment referring to kibana implementation

769846e

aleksmaus requested review from joshdover and mtojek January 28, 2022 13:56

mtojek reviewed Jan 28, 2022

View reviewed changes

Address code review feedback

f0dbcca

mtojek approved these changes Jan 28, 2022

View reviewed changes

mtojek changed the title ~~Correctly process the hidden property in datastream definition~~ Fix: use correct index template name for hidden data streams Jan 28, 2022

mtojek merged commit bbb7797 into elastic:main Jan 28, 2022

joshdover mentioned this pull request Feb 4, 2022

[Change Proposal] Stop using dot-prefixes for hidden indices & data streams elastic/package-spec#266

Open

1 task

Fix: use correct index template name for hidden data streams #673

Fix: use correct index template name for hidden data streams #673

Uh oh!

Conversation

aleksmaus commented Jan 27, 2022

Uh oh!

elasticmachine commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aleksmaus Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtojek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

elasticmachine commented Jan 27, 2022 •

edited

Loading

aleksmaus Jan 27, 2022 •

edited

Loading