-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Add toggle for experimental synthetic _source
support in Fleet data streams
#140132
[Fleet] Add toggle for experimental synthetic _source
support in Fleet data streams
#140132
Conversation
@@ -85,7 +86,7 @@ export const onPackagePolicyPostCreateCallback = async ( | |||
* Callback to handle deletion of PackagePolicies in Fleet | |||
*/ | |||
export const removeCspRulesInstancesCallback = async ( | |||
deletedPackagePolicy: DeletePackagePoliciesResponse[number], | |||
deletedPackagePolicy: DeepReadonly<DeletePackagePoliciesResponse[number]>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixes a type issue that was revealed when we added an array field to the PackagePolicyPackage
type that gets used in the DeletePackagePoliciesResponse
type.
I think updating this type to expect a DeepReadOnly
is actually just correct based on the usage of this callback.
const varConfigEntry = packagePolicyInputStream.vars?.[varName]; | ||
const value = varConfigEntry?.value; | ||
const frozen = varConfigEntry?.frozen ?? false; | ||
<> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this diff is whitespace due to introducing a fragment at the top level. GitHub's ignore whitespace setting for this PR is probably your friend here 🙂
features: { | ||
type: 'nested', | ||
properties: { | ||
synthetic_source: { type: 'boolean' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt it was better to be explicit about what features exist here, rather than just accepting any set of key/values. We'll need to expand this with additional settings moving forward.
); | ||
|
||
// Delete the experimental features map from the package policy so it doesn't get persisted | ||
delete packagePolicy.package.experimental_data_stream_features; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We pass the experimental data stream features array on the actual package policy object that comes back through the package policy create/update APIs. This is great for getting the data into our service here, but we don't actually want to persist that experimental features array on the package policy object.
This function "hoists" the experimental features array up out of the package policy to store it at a higher level, so we need to clean it up at the end.
Pinging @elastic/fleet (Team:Fleet) |
x-pack/plugins/fleet/server/services/package_policies/experimental_datastream_features.ts
Show resolved
Hide resolved
mappings: { | ||
...componentTemplate.template.mappings, | ||
_source: { | ||
mode: featureMapEntry.features.synthetic_source ? 'synthetic' : 'stored', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nchaulet here we determine the value for the synthetic source property.
Although looking at this now I think we can optimize to not perform a PUT if the value is unchanged.
@kpollich Are missing some code also to handle the upgrade of a package, I think the experiment will be loss on package update here no? maybe the same for force reinstall too? |
@@ -873,6 +907,10 @@ class PackagePolicyService implements PackagePolicyServiceInterface { | |||
namespace: newPolicy.namespace ?? 'default', | |||
description: newPolicy.description ?? '', | |||
enabled: newPolicy.enabled ?? true, | |||
package: { | |||
...newPP.package!, | |||
experimental_data_stream_features: newPolicy.package?.experimental_data_stream_features, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we persist the experimental features on the package policy, or populate that from the package saved object? it seems to me that that feature is shared by all the package policy using this package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only have the value on the package policy here so we can pass it easily to and from the policy editor. It gets persisted on the epm-package
saved object.
Putting it on packagePolicy.package
here just made it easiest to access when rendering the policy editor. Open to other approaches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting it on packagePolicy.package here just made it easiest to access when rendering the policy editor. Open to other approaches.
let's consider you have two package policy using nginx package (package policy 1, package policy 2)
- you edit package policy 1 to enable the experiment
- you visit package policy 2 the experiment is shown as disabled but in reality it's enabled no?
I think we should fetch the installation in the package policy editor.
it is clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added logic to packagePolicyService.get
in this PR to provide the experimental info on packagePolicy.package
in the API response for package policies. We do fetch from the installation saved object and hydrate the resulting API response body with the experimental feature data.
It might make a little more sense to return it separately somehow, or to have a separate API request for the policy editor to fetch the installation on its own, though. I could see an argument for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we fetch it in the API response there is probably no need to persist on the package policy saved object no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we don't want to persist it on the package policy saved object. It shouldn't be saved there at all. We are delete
ing the property before it gets saved. If it's being saved then it's a bug in this PR.
Yeah I think we need some logic in our upgrade API to pull the values of these experimental opt-ins. I will take a look at this. |
Force install is an interesting case. I would expect a force reinstall to wipe out any existing package level settings like this, so I think this is working as intended. Curious to hear if there are objections to that approach. |
…kibana into 140095-synthetic-source-toggle
let experimentalFeatures: ExperimentalDataStreamFeature[] | undefined; | ||
|
||
if (packagePolicySO.attributes.package?.name) { | ||
const installation = await soClient.get<Installation>( | ||
PACKAGES_SAVED_OBJECT_TYPE, | ||
packagePolicySO.attributes.package?.name | ||
); | ||
|
||
if (installation && !installation.error) { | ||
experimentalFeatures = installation.attributes?.experimental_data_stream_features; | ||
} | ||
} | ||
|
||
const response = { | ||
id: packagePolicySO.id, | ||
version: packagePolicySO.version, | ||
...packagePolicySO.attributes, | ||
}; | ||
|
||
// If possible, return the experimental features map for the package policy's `package` field | ||
if (experimentalFeatures && response.package) { | ||
response.package.experimental_data_stream_features = experimentalFeatures; | ||
} | ||
|
||
return response; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nchaulet here's where we fetch the installation and include it in the package policy object returned from packagePolicyService.get()
I think I found another path where we need to handle that, when you bump |
Upgrade path has been updated to handle these new settings in 50f2edd..
Taking a look at this next. |
@elasticmachine merge upstream |
@nchaulet Can you clarify the code path where |
I removed the "Review our docs" link from this UI for now since we don't have docs to link to for these experimental features. When we take on the other two features it'd probably make sense to reach out to the docs team to get something together then, but for now with just the one-off synthetic source toggle that's mainly going to be used by integration maintainers to test the feature I think it's fine to remove the link. |
@elasticmachine merge upstream |
I ran through this process and everything worked as expected
All of this worked as expected after a few fixes I made in d8d751a I've also added a few tests for the experimental data stream settings utilities. |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
History
To update your PR or re-run it, just comment with: cc @kpollich |
Summary
Closes #140095
Adds a "synthetic source" toggle to the policy editor for all data streams.
In cases where a data stream contains mappings that are incompatible with synthetic source, the errors will be bubbled directly to the user and the policy will not be save-able. e.g.
TODO