Repository navigation
Conversation
…r and looking up command registry (v2 legacy)
…mand calls and cleanup warning outputs
|
would like to mention that the CVE check failures applies to the current live branch, when those are updated a fork sync will also solve it. |
There was a problem hiding this comment.
🤖 AI-Assisted Architectural & Compatibility Review
Thank you for putting together this proof of concept! Migrating the CLI plugin RPC layer from Cloud Controller V2 to V3 is a critical and long-awaited step (#3621).
We conducted an adversarial review and empirical test validation of the proposed changes against origin/v8. While the overall direction of using v7action.Actor and configv3.Config is sound, we identified two blocking launch/lifecycle regressions and several public model mapping discrepancies that would break existing plugins relying on backward compatibility:
Key Findings Summary
- [Blocker] Offline Plugin Launch (F1):
PluginRunner.Rununconditionally dials Cloud Controller before starting the RPC listener, failing withNo API endpoint setwhen no target is configured (plugin/runner/runner.go:79). - [Blocker] Lifecycle Service Nil Dereference (F2): Lifecycle commands (
uninstall-plugin) createRpcServicewithnildependencies; plugin uninstall callbacks callingAccessToken()panic on nil dereference (command/plugin/shared/rpc.go:46). - [Should-Fix] Public Model Mapping Regressions:
UserEmail()returns username rather than the JWT email claim (plugin/rpc/cli_rpc_server.go:255).GetSpaceduplicates security groups bound to both running and staging (plugin/rpc/cli_rpc_server.go:618).- Application
Statecasing changes from"started"to"STARTED"(plugin/rpc/mappings.go:381). - Role strings change from legacy constants (
RoleOrgManager) to v3 enums (organization_manager) (plugin/rpc/mappings.go:118). - Unlimited quota limits become
0instead of-1due to null unmarshaling (plugin/rpc/mappings.go:189). Domain.Namereceives full route URLs rather than domain names (plugin/rpc/mappings.go:462).GetAppreturns binding names instead of service instance names (plugin/rpc/mappings.go:471).GetServicesdropsServicePlan.Guid(plugin/rpc/mappings.go:84).GetAppsreports observed live instances instead of desired scale for stopped apps (plugin/rpc/mappings.go:503).GetAppexposes redacted start command[PRIVATE DATA HIDDEN IN LISTS]and omits environment variables (plugin/rpc/cli_rpc_server.go:331).
- [Should-Fix] Concurrency Data Race (F4):
CallCoreCommandshares an unsynchronizedbytes.Bufferacross stdout and stderr interceptors (plugin/rpc/cli_rpc_server.go:53). - [Should-Fix] Credential Persistence (F19): Token refreshed during RPC
AccessToken()is not saved to disk on exit, risking UAA refresh token invalidation (plugin/rpc/cli_rpc_server.go:315).
Detailed inline review comments with citations to current origin/v8 source code and suggested fixes are attached below.
…avoid panics during plugin-uninstall
…lugin GetServices
|
All inline review threads are addressed (replies on each thread). Beyond the review, I synced the branch with
Compatibility notes (worth reading for plugin authors):
Proposed for follow-up PRs (out of scope here):
Still not populated (known gaps): Testing: unit tests for every change (RED → GREEN), |
Thank you for contributing to the CF CLI! Please read the following:
If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
Note: Please create separate PR for every branch (main and v8) as needed.
Description of the Change
While this PR may not be considered for merging, it intends to support #3621 showing how the current v8 plugin implementation may be refactored in order to fully support v3 calls and prepare the project to completely drop any v2 related code. A main branch PR has not been created unless the pr is deemed worthy and a main pr is requested (such a PR would/should probably already have the deprecated methods fully removed).
Changes of note are:
Some caveats:
Why Is This PR Valuable?
It finally drops the dependency of the V2 cc API and allow plugin backward compatibility which may remain unchanged (although they shouldn't) unless they explicitly invoke V2 endpoints. From the CLI scope perspective, it's the next evolving step.
Applicable Issues
#3621
How Urgent Is The Change?
V2 is bound to be fully decommissioned and several foundation installations already have the V2 api disabled. This will allow the v2 api to be fully decommissioned while having the CLI fully compatible (v3 plugin compatibility, if it directly calls v2 endpoints, falls under the responsibility of the plugin maintainers)
Other Relevant Parties
All plugin users/developers