Bugzilla – Full Text Bug Listing
| Summary: | Plan verifier to allow third party plugin to veto a provisioning plan | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Equinox | Reporter: | Krzysztof Daniel <krzysztof.daniel> | ||||||||||||||||||||||||||||||||||
| Component: | p2 | Assignee: | P2 Inbox <equinox.p2-inbox> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||||||||||||||
| Priority: | P3 | CC: | dj.houghton, john.arthorne, jpitman, krzysztof.daniel, natalia.bartol, pascal, peter, pquiring, pwebster, rderwin, Szymon.Brandys | ||||||||||||||||||||||||||||||||||
| Version: | 3.4.2 | ||||||||||||||||||||||||||||||||||||
| Target Milestone: | 3.6.1 | ||||||||||||||||||||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||||||||||||||||||||
| OS: | Windows XP | ||||||||||||||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
Created attachment 167706 [details]
Patch v 01 - for core.update
Created attachment 167880 [details]
Patch v03 - small corrections
Created attachment 168367 [details]
Patch_v04
Improved version of patch.
Could you please be a bit more specific about the use case? The intention? The case here is to allow Eclipse-based products to verify the provisioning plan created by P2, and block the installation if it leads to corrupted configuration. Some products may have a set of "core" plugins, that should not be modified, especially not uninstalled, but P2 is not aware of it. Included patch introduces new extension point to P2 and Classic Updater plugins, where a verifier plugin can be plugged in. The extension is optional - if verifier plugin is not present in configuration and cannot provide the extension - then the verification is simply omitted and installation process works as usual. Additionally there is a system property, that can enable or disable the verification process. Current patch is created for 3.4.2 version, I am going to prepare another one for 3.6. (In reply to comment #4) > Could you please be a bit more specific about the use case? The intention? Basically this is a simple hook that allows a third party to examine a provisioning plan and veto it before it gets executed by the engine. The interest is mainly in the controlling/overriding what the dropins reconciler does to the system. I can see other potential uses for it though, such as enforcing some externally defined policy (e.g., user may not install/uninstall X). There are clearly a number of different places such a hook can go (in the planner, in the engine, in the UI, in the reconciler) with different implications in each case. This is too big of a change for 3.6 but something to explore in 3.7. > The case here is to allow Eclipse-based products to verify the provisioning
> plan created by P2, and block the installation if it leads to corrupted
> configuration. Some products may have a set of "core" plugins, that should not
> be modified, especially not uninstalled, but P2 is not aware of it.
If there are such plug-ins, they should simply be marked as such in the profile. Should there not be enough diversity in the markup authorized in the roots, then we should extend this.
The issue with the proposed approach is that it only cover the case where the agent modifying the install runs your verification extension. When I take an off-the-shelf director/installer and modify this install, I can still run the risk of breaking it.
Given that you have the opportunity of running your own code, instead of failing when a plan is being executed, I would look into enforcing the constraints by setting properties in the profile when the previous installation has occurred.
Created attachment 168557 [details]
Add Verifier to SimplePlanner (3.6)
After analyzing 3.6 sources, I would attach Verififer code to p2.director SimplePlanner Class in generatingProvisioningPlan method. I'm attaching my initial proposition of patch.
Verifier plugin is loaded with Declarative Service.
Verifier algorithm will analyze requestChanges and requestSideEffects. That would be an additional verification, cause one is already done in computeAcutalChangeRequest method.
If verifier catches any dangerous operations, it will set RequestStatus of InstallableUnit to IStatus.ERROR.
Verifier will not attach any messages, just change status.
Further processing will remain as is - user-readable messages are attached in PlanAnalyzer class.
The latest patch does not allow for disabling the verifier via command line. We talked about this more and I wanted to be precise about the direction. The current recommendation is : - reconciler constructs a profile change request - reconciler calls the verifier and passes them the request - verifier modifies the change request - the request is passed to the planner and execution/installation continues as per normal In the verifier, you have access to the repositories so I would suggest: - get the metadata repositories from the metadata repository manager - get all known repos - figure out which ones you are interested in (file location? contents? repo property?) - for each IU being added if it is a root then removal the optional flag There is current code in the reconciler which says "if I have an IU and it is from the dropins and also strict then that means that it used to be installed via dropins but now it is from the UI so clear all the dropin related properties". We should be able to work around this by introducing a new IU property which means something "strict from dropins" and then use that to filter out the case where we don't want to remove the dropins information from the IU. (we just can't add another inclusion rule value because that is used by the engine, planner, etc) Pascal, what are your thoughts about this approach? Created attachment 169502 [details]
Version 1 - patch for 3.6
I've prepared two versions of patch for 3.6. Verifier is used as Declarative Service. I introduced porperty eclipse.p2.verifyPlan to enable and disable the Verifier.
Version 1. (changed bundles: org.eclipse.equinox.p2.director, org.eclipse.update.core):
Verifier plugin is loaded in SimplePlanner and verifies requestChanges and requestSideEffects in generated plan. If any dangerous operation like uninstallation of some "core" bundles or version downgrade is discovered - the status of installable unit in this plan is set to ERROR.
This approach is really simple, and works good when user interface is used for installation. Plan is analyzed in PlanAnalyzer class, where user-readable messages are added to final report.
What I don't understand is why PlanAnalyzer is not used in reconciler.dropins.
The plan returned from planner initially has OK status (code 0), no matter if requestChanges have ERROR status or not. This is responsibility of PlanAnalyzer to check statuses of all requestChanges and sideEffects and set the general plan status to ERROR if there is a problem with modified units.
No using PlanAnalyzer in ProfileSynchronizer.synchronize method causes plan that contains ERRORS to be passed to Engine.perform method. What is more, no information is logged, cause the overall status was OK.
This is why I decided to prepare second version of patch:
Version 2. (changed bundles: org.eclipse.equinox.p2.reconciler.dropins, org.eclipse.equinox.p2.operations, org.eclipse.update.core):
reconciler.dropins bundle is modified and in ProfileSynchronizer.synchronize method the generated plan is passed to PlanAnalyzer. Next, the status of the ResolutionResult returned from PlanAnalyzer is checked. If status is set to ERROR or CANCEL then this status is returned and properly added to log file. This way, we don't call engine.perform on the invalid plan.
Verifier plugin is loaded in PlanAnalyzer.computeResolutionResult method. Verifier checks requestChanges and requestSideEffects and can set state to ERROR as well as add apropriate messages to ResolutionResult. In case of errors - this messages appear in log file, so user will have detailed information about what went wrong with installation.
-------------------------
For me version 2. seems to be much better. However if there is any reason why PlanAnalyzer should not be used in reconciler.dropins then there is version 1.
Created attachment 169503 [details]
Version2 - patch for 3.6
Created attachment 169587 [details]
Plan verifier - patch for 3.6
I've realized that my previous approach was incorrect. Please just ignore my previous comment :)
Patch for 3.6 attached.
Changed bundles: org.eclipse.update.core, org.eclipse.equinox.p2.director
Verifier is loaded as Declarative Service. I introduced porperty eclipse.p2.verifyPlan to enable and disable the Verifier.
Verifier plugin is loaded in SimplePlanner and verifies requestChanges and
requestSideEffects in generated plan. If any dangerous operation like
uninstallation of some "core" bundles is discovered then PlannerStatus is set to ERROR and detailed messages are attached. This messages appear in installation dialog when user interface is used or in .log when using dropins.
Created attachment 169753 [details]
Updated patch for 3.6 stream
Previous patch was pretty close. Just two small changes:
- unget the verifier service after using it
- The bundle context was checked for null after it was already referenced
Created attachment 169827 [details]
patch
Updated patch.
- unget the service reference after we are finished with it in the UM code
- call Boolean#valueOf instead of Boolean.parseBoolean. (which checks for a System property)
Can we not use system property to turn on the verifier? Currently it is eclipse.p2.verifyPlan, and by default verifier is disabled. Can we assume that if there is a bundle that provides verifier service then we should use it? For now Eclipse-based product has to add new property to config.ini to enable the verifier. Currently the verifier will run if the system property is set to true and someone has provided a verifier implementation. The default behaviour (re: enabled/disabled) can be discussed.
John and I talked about the potential API in the plan verifier and agree that a better direction (more cleaner approach) might be something like:
public abstract class PlanVerifier {
public abstract IStatus verify(IProvisioningPlan plan);
}
Or if you need access to other information (the Maps that you were passing in before) then maybe something like this:
public abstract class PlanVerifier {
public abstract IStatus verify(PlannerStatus status);
}
or downcast the arg in the implementation to see if we have a PlannerStatus:
public abstract class PlanVerifier {
public abstract IStatus verify(IStatus status);
}
We should also consider adding a progress monitor to the methods since the code being executed is third-party code and can be long-running.
(In reply to comment #17) > public abstract class PlanVerifier { > public abstract IStatus verify(IProvisioningPlan plan); > } I like this one the best. It is the most generic and allows the verifier to access other details in the plan if applicable in the future. A client can access the details via plan.getStatus(), casting to PlannerStatus to obtain the request changes and side-effects. Created attachment 170210 [details]
patch
I agree. This is good idea to use:
public abstract class PlanVerifier {
public abstract IStatus verify(IProvisioningPlan plan);
}
I'm attaching patch with changed PlanVerifier class. I also prefer to keep verifier enabled by default so I changed system property to eclipse.p2.disablePlanVerifier.
I'm not sure if progress monitor is needed here - verification should not consume much time comparing to plan generation.
Created attachment 170403 [details]
patch
Updated patch.
- uses the new API
- runs the verifier in a SafeRunnable
- the verifier is enabled by default
- the System property to disable it is equinox.p2.verifyPlan=false
I didn't add progress monitoring but if we eventually want to release this into the main Eclipse stream then this will have to be considered.
Created attachment 170406 [details]
patch
Patch with the changes above but the System property set to be eclipse.p2.verifyPlan. (eclipse instead of equinox)
Created attachment 172682 [details]
patch
Same patch but with debug/tracing information added.
Created attachment 172911 [details]
Updated patch for Update Manager
- unget the verifier service when done
- updated copyrights
- verifier API is unchanged
Created attachment 173229 [details]
PlanVerifier moved from internal packages
Can we move PlanVerifier class from org.eclipse.equinox.internal.provisional.p2.director to org.eclipse.equinox.p2.planner? It shouldn't be in internal package. The same situation is in org.eclipse.update.core.
I'm attaching patch that includes the latest patches attached by DJ and John and moves PlanVerifier to org.eclipse.equinox.p2.planner (in org.eclipse.equinox.p2.director bundle) and to org.eclipse.update.configuration package (in org.ecipse.update.core bundle).
(In reply to comment #24) > Can we move PlanVerifier class from > org.eclipse.equinox.internal.provisional.p2.director to > org.eclipse.equinox.p2.planner? It shouldn't be in internal package. The same > situation is in org.eclipse.update.core. No. We don't add API to maintenance branches such as 3.4.x and 3.6.x. Since think this isn't a good long term solution (see comment #7), we are not currently considering API for 3.7 either. This verifier is only a short term solution until the client (IM) can express their dependencies properly by installing via the p2 API or director. That way the dependencies will be present in the p2 metadata, and any kind of agent manipulating the install will honour them without requiring any 3rd party verifier to be present. I think we should go ahead and release this in 3.6.x only. It adds no API, and no effect if no verifier is installed. After trying several other options this is the approach with the smallest possible p2 change to allow the client code to verify their additional constraints. It will have no impact for anyone who doesn't have a verifier present and it will be gone in 3.7. Released to R36x branch. As per John's note in comment #25, we will not be releasing this into HEAD. Closing. |
Created attachment 167704 [details] Patch v 01 The issue is very visible in the Eclispe based products which have all plugins marked as optional. This patch should open P2 for verification - if particular feature/plugin should be installed into the product.