Skip to content

migrate rpc server to v3 cc api and prepare for future plugin interface refactoring - #3751

Open
jcvrabo wants to merge 42 commits into
cloudfoundry:v8from
jcvrabo:v8
Open

jcvrabo wants to merge 42 commits into
cloudfoundry:v8from
jcvrabo:v8

Conversation

@jcvrabo

@jcvrabo jcvrabo commented Mar 31, 2026 •

Copy link
Copy Markdown
Member

Thank you for contributing to the CF CLI! Please read the following:

  • Please make sure you have implemented changes in line with the contributing guidelines
  • We're not allowed to accept any PRs without a signed CLA, no matter how small.
    If your contribution falls under a company CLA but your membership is not public, expect delays while we confirm.
  • All new code requires tests to protect against regressions.
  • Contributions must be made against the appropriate branch. See the contributing guidelines
  • Contributions must conform to our style guide. Please reach out to us if you have questions.

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:

  • It keeps the RPC protocol and plugin implementation unchanged, with the exception of adding an additional method to CliConnection which now returns an initialized cfclient that may be used by newly implemented (or updated) plugins instead of the existing Get* methods (which are now tagged as deprecated.
  • The internal implementation of the existing Get* methods make use of v7action.Actor to get the required information, and when methods do not exist, it makes use of the Actor's CloudControllerClient to get the additional data. Because the current Get methods are meant to be deprecated, no new Actor methods were added to fully support the CliConnection Get methods.
  • CallCoreCommand makes use of the existing command_parser (used to run commands by main) instead of the legacy command registry and simplifies the output capture which can be retrieved by the plugin.
  • It uses the configv3.Config object already initialized at the start to get configuration values instead of reading it again later with the corerepository implementation.
  • No V2 code is used, and the legacy command architecture is eligible to be removed (not done in this pr as it falls out of scope and it would add too much clutter to the PR review).

Some caveats:

  • Listing org users or space users will always return the isAdmin attribute as false. Supporting this attribute would require calling uaa for every listed user to check if it would have the cloud_controler.admin scope. Seems unnecessary and a high price to pay.
  • The domain.name value when listing routes currently returns the full route url as the current action is not returning the domain name. This may be updated to properly support the domain name
  • When returning the buildpack for an App, since the legacy model returns a single buildpack, the first buildpack of the app current droplet will be returned. This may be updated to return a comma separated list of buildpacks instead.
  • The plugin related commands (install, uninstall, repo, etc.) were not reviewed and still work as before. These may also be reviewed.
  • The provided CfClient is created only with the access token, which is only suitable for immediate commands and not for longer processing plugins. For that some changes in the RPC protocol would have to be done so that the refresh token could also be obtained by the plugin. As it is, plugin implementations could handle some refresh method which would call the rpc AccessToken method whenever required.

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

@jcvrabo jcvrabo mentioned this pull request Mar 31, 2026
@jcvrabo

jcvrabo commented Mar 31, 2026

Copy link
Copy Markdown
Member Author

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.

@Gerg
Gerg self-requested a review October 13, 2026 14:51
@anujc25
anujc25 self-requested a review October 14, 2026 18:26
@Gerg
Gerg requested a review from vuil October 22, 2026 21:24

@Gerg Gerg left a comment •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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

  1. [Blocker] Offline Plugin Launch (F1): PluginRunner.Run unconditionally dials Cloud Controller before starting the RPC listener, failing with No API endpoint set when no target is configured (plugin/runner/runner.go:79).
  2. [Blocker] Lifecycle Service Nil Dereference (F2): Lifecycle commands (uninstall-plugin) create RpcService with nil dependencies; plugin uninstall callbacks calling AccessToken() panic on nil dereference (command/plugin/shared/rpc.go:46).
  3. [Should-Fix] Public Model Mapping Regressions:
  4. [Should-Fix] Concurrency Data Race (F4): CallCoreCommand shares an unsynchronized bytes.Buffer across stdout and stderr interceptors (plugin/rpc/cli_rpc_server.go:53).
  5. [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.

Comment thread plugin/runner/runner.go Resolved
Comment thread plugin/runner/runner.go Resolved
Comment thread command/plugin/shared/rpc.go Resolved
Comment thread plugin/rpc/cli_rpc_server.go Resolved
Comment thread plugin/rpc/cli_rpc_server.go Resolved
Comment thread plugin/rpc/mappings.go Resolved
Comment thread plugin/rpc/mappings.go Resolved
Comment thread plugin/rpc/mappings.go Resolved
Comment thread plugin/cli_connection.go Resolved
Comment thread main.go Resolved
@jcvrabo

jcvrabo commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

All inline review threads are addressed (replies on each thread). Beyond the review, I synced the branch with upstream/v8 (this fixes check-cves), and fixed several things the review didn't cover:

  • Build: plugin/pluginfakes/fake_cli_connection.go wasn't regenerated after adding CfClient(), so go build ./... failed for anyone importing pluginfakes (c9c2b8aaa). plugin/cli_connection.go was also re-indented with tabs.
  • Legacy fallback: The last three v7 commands that still fell back to the legacy main (list-plugin-repos, repo-plugins, remove-plugin-repo) are now ported to v7 (pluginaction + configv3). Plugins calling them through CliCommand run them in-process again, as upstream did through the legacy registry, instead of re-entering the legacy main with the host's arguments. (eba7304c3) With that, nothing triggers the legacy-main fallback anymore, so it's removed (62b1d947c): util/command_parser no longer calls cf/cmd.Main, and cf/cmd (with the legacy command registry) is no longer linked into the binary. The legacy cf/... code stays in the repo for a separate removal. This also removes the path through which a plugin's CliCommand re-entered the legacy main with the host's os.Args (re-launching the plugin).
  • Warnings on stdout: Get* RPC methods used to print Warning: … on stdout. Legacy suppressed all output for these calls, so this would have corrupted plugin stdout. Warnings now go to stderr through UI.DisplayWarnings, the same as v7 commands. (12a026907)
  • Instance state in GetApp().Instances had the same casing regression as the app state (RUNNING vs running) and is lowercased too (ceefa4b8d).
  • Lint: Marking the Get* methods deprecated makes staticcheck flag the two test plugins that still call them on purpose. Those 15 calls now carry //nolint:staticcheck with a reason. stopPlugin explicitly ignores the Kill/Wait errors. With this, the branch adds no lint issues compared to upstream/v8 (and it removes 4). (4dfbdda42)

Compatibility notes (worth reading for plugin authors):

  1. CliCommand output is now v7 output. Upstream's CallCoreCommand ran the legacy v6 commands, so plugins received v6 output. Core commands called from plugins now produce v7 output (different tables, headers and wording). Plugins that scrape CliCommand output may need changes; the structured alternatives are the Get* models or CfClient(). Stdout and stderr are still captured together, as before.
  2. Install/uninstall callbacks have no Cloud Controller access. In CLI-MESSAGE-UNINSTALL, Get* and CliCommand return errors, and AccessToken() returns the stored token without refreshing it. Upstream gave these callbacks full access through the legacy registry. This is deliberate: the hook is for cleaning up the plugin's own files and notifying outside services.
  3. CfClient() on the public CliConnection interface breaks third-party code that implements that interface (custom fakes), and adds github.com/cloudfoundry/go-cfclient/v3 v3.0.0-alpha.20 (an alpha release, which is stable and production ready... I have requested already for a final release and even though it was agreed, it is still waiting) to every plugin's dependency graph. The client is valid only until the current access token expires (documented on the method).

Proposed for follow-up PRs (out of scope here):

  • Token refresh inside CfClient() (a RefreshToken RPC or, preferably, an RPC-backed oauth2.TokenSource, which needs a go-cfclient option).
  • Blocking plugin lifecycle commands (install-plugin/uninstall-plugin) from CliCommand, so a running plugin can't replace or delete installed plugins, itself included.
  • Applying leading global options (e.g. -v verbose request logging) to plugin runs. They are currently only skipped to find the command.
  • Removing the now-unreachable legacy cf/... code.

Still not populated (known gaps): GetApp DetectedStartCommand, PackageUpdatedAt and StagingFailedReason; GetApps route Domain.OwningOrganizationGuid and Domain.Shared. GetOrgUsers/GetSpaceUsers IsAdmin is always false, which already matched legacy.

Testing: unit tests for every change (RED → GREEN), -race on plugin/rpc and plugin/runner, and smoke runs of a built cf against a fixture plugin and a local plugin repository. Not run: the CF-backed integration suites.

@jcvrabo
jcvrabo requested a review from Gerg November 1, 2026 22:15

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants