[Fleet] Add support for "Edit Package Policy" extensions using latest version of a package #114914
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing upgrade state for edit policy view
kpollich
added
v8.0.0
release_note:skip
|
Pinging @elastic/fleet (Team:Fleet) |
botelastic
bot
added
Team:APM
|
Pinging @elastic/apm-ui (Team:apm) |
|
Pinging @elastic/uptime (Team:uptime) |
2 tasks
joshdover
reviewed
Oct 14, 2021
| @@ -193,6 +193,7 @@ export class Plugin implements IPlugin<PluginSetup, PluginStart, SetupPlugins, S | |||
| registerExtension({ | |||
| package: 'endpoint', | |||
| view: 'package-policy-edit', | |||
| useLatestPackageVersion: true, | |||
There was a problem hiding this comment.
I'm not sure this is a requirement for the Endpoint package since Elastic Agent has the ability to dynamically run different versions of the endpoint binary. That said, I can't really think of any downsides here since the user is already going to be pushing out updates to all of their endpoints.
Let me figure out the best person to talk to about this.
There was a problem hiding this comment.
I spent time working through by going through the following steps.
- Create a
0.3.0package using what's currently published onepr.elastic.co - Start package registry locally with version
0.3.1and point to my local registry - Attempt to edit the existing
0.3.0policy
What I've ran into is that... the policies seem to be auto updating. I do not have it set to keep policies up to date.
Has synthetics auto upgrading been added to main already? Is it on by default for all packages right now?
Because of the auto upgrading, I wasn't able to yet validate the POC without auto upgrading. Please let me know if there are steps I can take to turn off the auto upgrading.
|
@dominiqueclarke - Ah yeah it looks like we have a bug here that's causing any package flagged as cc @joshdover - my understanding of the above is correct, right? A package flagged as Bug is with this conditional, for reference: |
ogupte
approved these changes
Oct 15, 2021
Thank you for investigating. If it's okay with you, I'd like to rereview after the bug fix goes in. |
|
@elasticmachine merge upstream |
|
This worked as expected when editing an older policy, it successfully updated to a new version. However, when trying to edit that updated policy again, I'm getting this error, even though it's on the latest version.
|
|
@elasticmachine merge upstream |
kpollich
commented
Oct 19, 2021
| @@ -345,29 +363,6 @@ export const EditPackagePolicyForm = memo<{ | |||
|
|
|||
| const { error } = await savePackagePolicy(); | |||
| if (!error) { | |||
| if (isUpgrade) { | |||
There was a problem hiding this comment.
My change to return a bad request in the case of general failures, and instead allowing only validation errors to be considered non-fatal errors in 61bea60 revealed an interesting issue:
This API request to /upgrade the package policy was always failing before, we just didn't return an error-coded HTTP response. We returned a 200 with success: false in the body. However, the actual "upgrade" process has been and remains successful to this date. Why is that?
What we were actually doing here in the policy editor was:
- Run the dry run to get the new "proposed" shape of the package policy w/ any new inputs/streams/variables, and any existing values merged into them
- Use the new "proposed" package policy as our backing data model for the policy editor form
- Save the policy, including the new values and package version
- Call the
/upgradeendpoint, and receive an error because the policy has already been saved with the new package version
So, the /upgrade endpoint in its non-dry-run context was essentially useless in this workflow. And it is largely useless overall because we don't support editing at the time of upgrade. A consumer would have to make their edits to the outdated package policy prior to upgrading, which doesn't make sense because it'd result in an invalid package policy. We'd be adding values for variables defined in the new version of the package, then validating them against the outdated version of the package.
Instead, by removing this /upgrade call, we have a workflow that stops at #3 above. So our functionality is entirely the same as it was before, we've just done away with an erroneous call at the end of the process.
It seems to me like maybe we should formalize this and re-do some of the ergonomics of our upgrade process. The only purpose of the non-dry-run upgrade endpoint, at this point, would to be to perform an upgrade with no edits, then allow the user to make edits to their policy after the upgrade is complete. It seems like this is not a realistic user experience based on our implementation in Fleet, so it doesn't make sense to me to even support it. Instead, we should focus on the path of "Generate proposed upgraded policy -> Allow for edits to proposed policy -> Persist proposed policy".
I know this is long but I would love to get some thoughts here. Happy to sync offline as well.
There was a problem hiding this comment.
The actual upgrade service method still serves a purpose in upgrading package policies automatically, to be clear. We want to attempt an upgrade and fail if there are any conflicts. I am just proposing that we do away with the API endpoint that serves that same purpose.
There was a problem hiding this comment.
Instead, we should focus on the path of "Generate proposed upgraded policy -> Allow for edits to proposed policy -> Persist proposed policy".
This makes sense to me. It seems this endpoint is only useful in the context of attempting to automate an upgrade without any user intervention. If that's the case, then I think we should remove the ability to specify any edits / input vars in the upgrade endpoint and instead only use it for attempting to upgrade a policy automatically without any edits.
It'd then make sense to move the logic for executing a dry run + getting the new proposed policy to a separate endpoint.
To make sure I'm understanding correctly, if we followed this logic, we'd end up with something like:
POST /package-policy/<id>/upgradefor attempting automated upgrades without edits. Does not accept any inputs or request body at all. Will fail if there's a conflict.GET /package-policy/<id>/upgradefor executing a dry run and getting the "proposed package policy"PUT /package-policy/<id>for saving an existing policy, potentially with a higher version. Accepts all input fields. Will fail if validation fails for the given package version.
There was a problem hiding this comment.
Yes that sounds like a great improvement for the layout of this particular API.
POST
/package-policy/<id>/upgradefor attempting automated upgrades without edits. Does not accept any inputs or request body at all. Will fail if there's a conflict.
This is how the POST /package-policy/<id>/upgrade endpoint works today.
GET /package-policy/<id>/upgradefor executing a dry run and getting the "proposed package policy"
This is the current functionality of the POST /package-policy/<id>/upgrade { dryRun: true } API call
PUT /package-policy/<id>for saving an existing policy, potentially with a higher version. Accepts all input fields. Will fail if validation fails for the given package version.
This is existing functionality as well. So the only change here should be moving away from the dryRun: true request parameter, and creating a distinct GET /upgrade endpoint wired up to that particular service method instead.
I created #115570 to capture this piece of work. I tagged it as technical debt for now.
|
@dominiqueclarke - I just tracked down and pushed a fix for that issue w/ the default name always appearing when editing policies. Everything should be functional here, now. |
dominiqueclarke
approved these changes
Oct 19, 2021
|
There is one improvement here that I've identified in #115638 to avoid displaying the "Upgrade" UI when the policy is actually up to date. |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / general / X-Pack Saved Object Tagging Functional Tests.x-pack/test/saved_object_tagging/functional/tests/dashboard_integration·ts.saved objects tagging - functional tests dashboard integration editing allows to select tags for an existing dashboardStandard OutStack TraceMetrics [docs]Public APIs missing comments
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @kpollich |
kibanamachine
added a commit
to kibanamachine/kibana
that referenced
this pull request
Oct 20, 2021
… version of a package (elastic#114914) * Add support for extensions using latest version of a package and forcing upgrade state for edit policy view * Fix isUpgrade flag on integrations UI version of edit page * Treat non-validation errors as general failures in server and UI * Fix tests + don't call upgrade API when saving * fix i18n * Fix default name always appearing when editing package policies via extension UI * Opt security solution plugin out of new extension option Co-authored-by: Kibana Machine <[email protected]>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
kibanamachine
added a commit
that referenced
this pull request
Oct 20, 2021
… version of a package (#114914) (#115843) * Add support for extensions using latest version of a package and forcing upgrade state for edit policy view * Fix isUpgrade flag on integrations UI version of edit page * Treat non-validation errors as general failures in server and UI * Fix tests + don't call upgrade API when saving * fix i18n * Fix default name always appearing when editing package policies via extension UI * Opt security solution plugin out of new extension option Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kyle Pollich <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #114785
Ref #112831
Summary
Allows fleet UI extensions to opt into targeting the latest version of a package during edits, rather than the version specified on the current package policy.
How to Test
syntheticspackage by specifying a version in the "Add Integration" URL, e.g.http://localhost:5601/{BASE_PATH}/app/fleet/integrations/synthetics-0.1.0/add-integration
Address any conflicts and save
Note the policy has been upgraded to
0.3.0