Skip to content

Fix google_cloud/Vertex AI trusted endpoints, grpc surface, and the blueprints - #5103

Open
tonypiazza wants to merge 8 commits into
opensearch-project:mainfrom
tonypiazza:fix/5079-vertex-trusted-endpoints
Open

tonypiazza wants to merge 8 commits into
opensearch-project:mainfrom
tonypiazza:fix/5079-vertex-trusted-endpoints

Conversation

@tonypiazza

@tonypiazza tonypiazza commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Description

Addresses both items in #5079, folds in the blueprint corrections previously opened as #5104, and applies @ylwu-amzn's review feedback.

The google_cloud connector shipped in 3.9 could not be used by following its own documentation. The code half and the docs half of that are the same defect seen from two sides, so they are here together rather than as PRs that each look arbitrary alone.

Code

1. Vertex AI endpoints were unreachable with the default allowlist.

No default in plugins.ml_commons.trusted_connector_endpoints_regex matched a Vertex AI URL, so the connector could not reach *-aiplatform.googleapis.com without an operator widening the regex first. Adds a default covering both host forms Vertex serves:

^https://([a-z0-9][a-z0-9-]*-)?aiplatform\.googleapis\.com/.*$

The region prefix is hyphen-joined (us-central1-aiplatform.googleapis.com), not a dot-separated subdomain, and the global endpoint has no prefix at all, so a dot-separated pattern matches only the global form. The prefix must start with an alphanumeric because a DNS label cannot begin with a hyphen; ([a-z0-9-]+-)? would have admitted --aiplatform and -us-aiplatform. Neither is resolvable, so this was not exploitable, but the allowlist should only describe hosts that can exist.

2. io.grpc.Context is genuinely required, and the build comment said otherwise.

ml-algorithms excluded grpc-context with the comment "not needed here". The class is needed: the OAuth2 refresh path initializes google-http-client's OpenCensus instrumentation, which references it. It resolves from the transport-grpc module, declared optional in plugin/build.gradle but bundled in every OpenSearch distribution because modules are mandatory. The exclusion itself is correct, since bundling grpc-context would reintroduce the jar-hell clash with the module's grpc-api. Corrects the comment to record that reasoning, and translates NoClassDefFoundError/ExceptionInInitializerError at the token-minting chokepoint into an MLException naming the module.

3. _predict/stream returned a raw 500 on a non-streaming channel.

RestMLPredictionStreamAction cast the channel to StreamingRestChannel unguarded. On a node not configured for HTTP streaming the channel is a plain RestChannel, so the call failed with:

class_cast_exception: RestController$ResourceHandlingHttpChannel cannot be cast to
                      class org.opensearch.rest.StreamingRestChannel

That names internal channel classes and says nothing about the missing configuration. RestMLExecuteStreamAction already guards the identical cast and responds with a validation error, so this applies the same check and reuses its message verbatim, leaving the two streaming endpoints consistent.

4. The streaming integTest cluster could not start.

-Dstreaming=true installed transport-reactor-netty4 and arrow-flight-rpc but not arrow-base. The plugin CLI rejects arrow-flight-rpc without it ("Missing plugin [arrow-base], dependency of [arrow-flight-rpc]"), so the cluster could not come up even for someone who had built the plugins from a core checkout. Installed before arrow-flight-rpc, since install order decides whether the dependency check passes.

Documentation

All five Vertex AI blueprints. Four are google_cloud and exist on 3.9 and later; gcp_vertexai_connector_embedding_blueprint.md is the legacy protocol: http one and is also present on 2.19 and 3.8.

5. Every google_cloud blueprint returned 403 on its first real step.

plugins.ml_commons.connector.vertexai_enabled defaults to false and ConnectorProtocolValidator#validateProtocolEnabled rejects a google_cloud connector while it is off. All four went from "add trusted endpoint" straight to connectors/_create. The check also runs on connector update, model register and model update, so there is no path around it. The blueprints now set the flag and quote the actual message.

6. Every blueprint told the reader to replace the trusted-endpoint allowlist.

ML_COMMONS_TRUSTED_CONNECTOR_ENDPOINTS_REGEX is a Setting.listSetting with 12 defaults. Each blueprint instructed a PUT carrying a single Vertex pattern, which replaces the list rather than appending, so following any of them silently broke SageMaker, OpenAI, Cohere, DeepSeek, AI Studio Gemini and all three Bedrock endpoints in the same cluster.

The same instruction appears in 35 blueprints and 2 tutorials repo-wide, all assigning a list shorter than the defaults. That is pre-existing and vendor-wide, and should be a separate PR rather than added here.

7. Version-qualified, per @ylwu-amzn's review.

Removing the endpoint step outright was wrong: the new default only exists from the release carrying this PR. Readers on 3.9 following these docs would get no Vertex pattern at all. For the legacy blueprint the gap is permanent rather than temporary, since 2.19 and 3.8 will never gain the default and that file exists specifically to serve versions predating the google_cloud protocol.

All five now state which versions the default covers and give a check-then-append recipe for the rest. The check is necessary because a plain GET _cluster/settings omits settings that were never explicitly set, so a reader cannot otherwise see the list they are about to replace.

8. Streaming prerequisites, per @ylwu-amzn's review.

stream_enabled is necessary but not sufficient. _predict/stream also needs the transport-reactor-netty4, arrow-base and arrow-flight-rpc plugins, which ship with OpenSearch but are not installed by default, plus http.type and opensearch.experimental.feature.transport.stream.enabled in opensearch.yml. Both are static, so _cluster/settings rejects them and a restart is required. The blueprint links the Predict Stream API prerequisites rather than duplicating a platform-level list that would rot.

9. Four further blueprint defects.

  • The streaming blueprint omitted plugins.ml_commons.stream_enabled, which also defaults to false, so its step 4 could not succeed either.
  • The batch blueprint contradicted itself, saying "It defines three actions" while the connector section correctly said only batch_predict is declared and that declaring the other two breaks the derived URL.
  • The batch blueprint named the wrong source for the terminal task state, crediting remote_job.status_field and remote_job.status_regex.*. Neither half holds: status_field defaults to ["status", "Status", "TransformJobStatus"], none of which is the field Vertex reports, and status_regex.completed is (complete|completed|partiallyCompleted), which never matches JOB_STATE_SUCCEEDED. GetTaskTransportAction uses the connector's batch_job_status mapping when declared and only falls back otherwise.
  • The Gemini blueprint said token_uri is "restricted to Google token endpoints (*.googleapis.com over HTTPS)". validateTokenUri requires exactly oauth2.googleapis.com, over HTTPS, on the default port. The same overstatement on the documentation site is being corrected in Correct token_uri validation rules and timing for the google_cloud connector documentation-website#13164.

Verification

Unit tests: 12 added across four classes (MLCommonsSettingsTests 46 to 50, GoogleCredentialProviderTest 11 to 17, GoogleConnectorExecutorTest 5 to 6, RestMLPredictionStreamActionTests 14 to 15). All 88 tests in those classes pass.

fromConnector previously had no coverage at all: its only production caller is the lazy getter in GoogleConnectorExecutor, and every executor test pre-injects a provider. Adds the service-account happy path, a malformed private key, both ADC branches, the lazy-build-and-cache path, and a test pinning that an invalid token_uri surfaces as an unwrapped IllegalArgumentException.

The guard in item 3 is verified in both directions. Its regression test was confirmed to fail without the fix, with the original ClassCastException; the existing 14 tests in that file could not catch it, because prepareRequest returns a RestChannelConsumer and the cast happens inside that lambda, while every test only asserted the lambda was non-null. And on a cluster that is configured for streaming, _predict/stream against a deployed google_cloud model returns 200 with an SSE frame and does not take the guard's branch at all, failing instead at the upstream call for want of credentials. So the guard fires only on the misconfiguration it describes.

Items 1, 5 and 6 were verified on a live single-node cluster running a plugin build predating the new default, which reproduces the pre-3.10 state the docs now describe:

  • include_defaults=true reports the 12 defaults; persistent and transient are absent until set.
  • Assigning a single-element list leaves 1 effective pattern and drops the defaults entirely.
  • In that state, creating an OpenAI connector fails with 400 "Connector URL is not matching the trusted connector endpoint regex".
  • Re-sending the 12 defaults plus the Vertex pattern yields 13, after which the OpenAI connector creates successfully and a google_cloud ADC connector using a ${parameters.location} Vertex host also creates successfully.
  • With vertexai_enabled false, creation returns 403 "The Vertex AI (google_cloud) connector is not enabled."

Three of the blueprints were then walked end to end against real Vertex AI on clean single-node clusters, following each one as written and setting only the flags its step 1 specifies:

  • Gemini: connector, model group, register, deploy, inference. Returned a real Gemini response with finishReason: STOP.
  • Embedding: same path, returning a 768-dimension vector. text-embedding-004 was confirmed still current beforehand with a direct Vertex call.
  • Streaming: on a cluster with the three streaming plugins and the static settings, _predict/stream produced 15 SSE frames terminating with is_last: true and no error frames.

None of them needed an endpoint change, which is the claim items 5 and 6 rest on.

The streaming walk also exercised item 3 unintentionally. A first attempt had the security plugin installed but http.type set to reactor-netty4 rather than reactor-netty4-secure, which yields a genuinely non-streaming channel. The response was a 400 carrying "Unable to initiate request / response streaming over non-streaming channel". Before this PR that configuration returned a 500 ClassCastException naming internal channel classes, so the guard improves a misconfiguration that arises in practice rather than only in a test.

Two limits on all of the above. It was manual rather than automated. And only Option B (ADC) was exercised, because service-account key creation is blocked by policy on the account available to me, so Option A remains unit-tested only. I can share the exact reproduction steps if that would be useful.

Unrelated to the code here, but found while walking them: the Gemini and embedding blueprints both name their model group vertexai_model_group, so following both in one cluster fails at step 3 of the second with a duplicate-name error. Each blueprint on its own is unaffected, so I have left it alone.

Connector#validateConnectorURL resolves ${parameters.*} through StringSubstitutor before testing the URL, so every action URL in all five blueprints was checked to resolve to a host matching the new default, with each referenced parameter declared in the blueprint's own parameters block.

Known gaps, stated rather than implied

There is still no streaming integration test, and streaming has never run in CI. No workflow passes -Dstreaming=true, so nothing here is covered by automation.

Item 4 is verified locally against an OpenSearch core checkout at 3.10.0: all three plugins assemble as 3.10.0-SNAPSHOT, integTest -Dstreaming=true brings the cluster up with http.type=reactor-netty4 and arrow-flight-rpc initialising rather than merely loading ("Arrow Flight server started. Listening at [grpc+tcp://127.0.0.1:9400]"), and integration tests execute against it (3 run, 0 skipped, 0 failures). The netty allocator system properties and the arrow --add-opens argument in that block are both genuinely required; the node refuses to start without -Dio.netty.allocator.numDirectArenas=1.

Making that automated is blocked upstream: arrow-base and arrow-flight-rpc are published as 3.10.0-SNAPSHOT maven artifacts under org.opensearch.plugin, but transport-reactor-netty4 is not, so CI would have to build it from core.

There is also no integration test for this connector, so items 5 and 6 were caught by reading rather than by CI. A REST IT that creates a google_cloud connector on a cluster with only vertexai_enabled set and asserts 200 would have failed on both, and needs no credentials or network egress to do it.

On the scope of this PR

PR-Agent flags multiple themes and that is fair. Items 1, 2, 5, 6, 7 and 9 are all one story: defects in google_cloud as shipped in 3.9, where the doc fix depends on the code default, which is why #5104 was folded in here rather than landing separately and briefly wrong.

Items 3 and 4 affect all streaming models, not just google_cloud, and item 8 sits in a google_cloud blueprint but describes platform-level prerequisites. All three were found while verifying that the streaming blueprint actually works, and each is small: an instanceof guard mirroring a sibling, one plugin added to a test cluster, and one doc note. I kept them here because splitting an eleven-line guard into its own PR seemed like more reviewer overhead than less, but I am happy to break them out if you would rather review them apart.

Related Issues

Resolves #5079
Supersedes #5104

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created. Not applicable, no API changes.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created. Correct token_uri validation rules and timing for the google_cloud connector documentation-website#13164 covers the token_uri rules and timing. The documentation site does not publish an authoritative default list: remote-models/index.md shows a five-pattern example and notes that the setting replaces the whole list, so item 1 needs no companion change there.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Addresses both items in opensearch-project#5079.

1. Vertex AI endpoints were unreachable with the default allowlist.

No default in plugins.ml_commons.trusted_connector_endpoints_regex matched a
Vertex AI URL, so the google_cloud connector shipped in 3.9 could not reach
*-aiplatform.googleapis.com without an operator widening the regex first. Adds
a default covering both host forms Vertex serves:

  ^https://([a-z0-9-]+-)?aiplatform\.googleapis\.com/.*$

Note the region prefix is hyphen-joined (us-central1-aiplatform.googleapis.com),
not a dot-separated subdomain, and the global endpoint has no prefix at all. A
dot-separated pattern matches only the global form. Tests cover the regional
host, the global host, and rejection of a lookalike suffix
(...googleapis.com.evil.example).

2. io.grpc.Context is genuinely required, and the build comment said otherwise.

ml-algorithms excluded grpc-context with the comment "not needed here". The
class is needed: the OAuth2 refresh path initializes google-http-client's
OpenCensus instrumentation, which references it. It resolves from the
transport-grpc module, declared optional in plugin/build.gradle.

OpenSearch bundles transport-grpc in every distribution, including the min
distro, because modules are mandatory (distribution/build.gradle copies all
:modules:* subprojects). So the dependency is satisfied on any supported
install, and the exclusion itself is correct: bundling grpc-context would
reintroduce the jar-hell clash with the module's grpc-api on every normal
install. Corrects the comment to record that reasoning.

Also translates NoClassDefFoundError/ExceptionInInitializerError at the single
token-minting chokepoint into an MLException naming the module. This only fires
on an unsupported image with modules/ stripped, where the raw failure is an
opencensus stack trace naming neither google_cloud nor the missing module.

Test coverage for GoogleCredentialProvider.fromConnector

fromConnector had no test coverage: its only production caller is the lazy
getter in GoogleConnectorExecutor, and every executor test pre-injects a
provider, so the branch never ran. The validateTokenUri guard added in opensearch-project#5041
was covered only by direct calls to the helper. Adds coverage for the
service-account happy path, a malformed private key, both ADC branches, the
lazy-build-and-cache path in GoogleConnectorExecutor.credentialProvider(), and
a test pinning the current behavior that an invalid token_uri surfaces as an
unwrapped IllegalArgumentException rather than an MLException.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
@github-actions

github-actions Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit c46e99f)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 Multiple PR themes

Sub-PR theme: Add Vertex AI host to default trusted connector endpoints regex

Relevant files:

  • common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java
  • common/src/test/java/org/opensearch/ml/common/settings/MLCommonsSettingsTests.java

Sub-PR theme: Wrap missing io.grpc.Context error and expand GoogleCredentialProvider tests

Relevant files:

  • ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/GoogleCredentialProvider.java
  • ml-algorithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/GoogleCredentialProviderTest.java
  • ml-algorithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/GoogleConnectorExecutorTest.java

Sub-PR theme: Update Vertex AI blueprints to enable connector setting and clarify trusted endpoints

Relevant files:

  • docs/remote_inference_blueprints/gcp_vertexai_batch_blueprint.md
  • docs/remote_inference_blueprints/gcp_vertexai_connector_embedding_blueprint.md
  • docs/remote_inference_blueprints/gcp_vertexai_embedding_blueprint.md
  • docs/remote_inference_blueprints/gcp_vertexai_gemini_blueprint.md
  • docs/remote_inference_blueprints/gcp_vertexai_streaming_blueprint.md

⚡ No major issues detected

@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 21:15 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 21:15 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 21:15 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 21:15 — with GitHub Actions Waiting
@tonypiazza tonypiazza changed the title Fix google_cloud/Vertex AI trusted endpoints and grpc dependency surface Fix Vertex AI trusted endpoints and grpc dependency surface Sep 26, 2026
@github-actions

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Tighten regex to reject invalid region prefixes

The regex [a-z0-9-]+ allows a hyphen at the start or end of the region prefix (e.g.,
https://-aiplatform.googleapis.com/...), which is not a valid hostname segment. Use
a-z0-9 or [a-z0-9][a-z0-9-] to ensure the prefix starts and
ends with an alphanumeric character, preventing malformed hostnames from matching.

common/src/main/java/org/opensearch/ml/common/settings/MLCommonsSettings.java [300]

-"^https://([a-z0-9-]+-)?aiplatform\\.googleapis\\.com/.*$",
+"^https://([a-z0-9][a-z0-9-]*-)?aiplatform\\.googleapis\\.com/.*$",
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that [a-z0-9-]+ allows a leading hyphen in the region prefix, which is not a valid hostname segment. The improved regex [a-z0-9][a-z0-9-]*- ensures the prefix starts with alphanumeric and ends with a hyphen before aiplatform. This is a minor security/correctness improvement to the URL validation regex.

Low
Avoid relying on real credential refresh in test

getAccessToken() on GoogleCredentialProvider calls credentials.refreshIfExpired()
before returning the token value, but the mock only stubs getApplicationDefault. The
adc object was created with GoogleCredentials.create(...) before the static mock
opened, so refreshIfExpired() will execute real code against a static token — this
is fine, but getTokenValue() is called on the result of
credentials.getAccessToken(), which may return null for a GoogleCredentials created
via create() without a prior refresh. Verify that
GoogleCredentials.create(AccessToken) makes getAccessToken() return the provided
token without a refresh, or mock the credentials object directly to avoid relying on
internal behavior.

ml-algorithms/src/test/java/org/opensearch/ml/engine/algorithms/remote/GoogleCredentialProviderTest.java [212-213]

-mocked.when(GoogleCredentials::getApplicationDefault).thenReturn(adc);
-assertEquals("ya29.adc-token", GoogleCredentialProvider.fromConnector(adcConnector()).getAccessToken());
+GoogleCredentials adc = mock(GoogleCredentials.class);
+AccessToken token = new AccessToken("ya29.adc-token", new Date(Long.MAX_VALUE));
+when(adc.getAccessToken()).thenReturn(token);
+try (MockedStatic<GoogleCredentials> mocked = mockStatic(GoogleCredentials.class)) {
+    mocked.when(GoogleCredentials::getApplicationDefault).thenReturn(adc);
+    assertEquals("ya29.adc-token", GoogleCredentialProvider.fromConnector(adcConnector()).getAccessToken());
+}
Suggestion importance[1-10]: 4

__

Why: The suggestion raises a valid concern about relying on internal behavior of GoogleCredentials.create(), but the test was intentionally designed with the adc object created before the static mock opens (as noted in the comment). The improved_code restructures the test significantly by moving mock creation inside the try block, which changes the test's intent. The concern about getAccessToken() returning null is plausible but may not actually occur given the static token setup.

Low

@tonypiazza tonypiazza changed the title Fix Vertex AI trusted endpoints and grpc dependency surface Fix google_cloud/Vertex AI trusted endpoints and grpc dependency surface Sep 26, 2026
…fault

Follow-up to the regex added earlier in this PR, from a PR-Agent suggestion on
opensearch-project#5103.

"([a-z0-9-]+-)?" let the optional region prefix start with a hyphen, so these
matched the default allowlist:

  https://--aiplatform.googleapis.com/v1/predict
  https://-us-aiplatform.googleapis.com/v1/predict

A DNS label cannot begin with a hyphen, so neither host can resolve and this was
not exploitable. The allowlist should still describe only well-formed hostnames.
Requiring the prefix to start with an alphanumeric closes it:

  ^https://([a-z0-9][a-z0-9-]*-)?aiplatform\.googleapis\.com/.*$

Note the suggestion's own example, "-aiplatform", was already rejected: the old
group required at least two characters, so a single leading hyphen never
matched. The leak needed a longer prefix.

The tightened pattern still clears validateRegexSafety. The character before the
closing paren is a hyphen rather than a quantifier, so the shallow
nested-quantifier check does not fire, and there are no backreferences.

Regional and global Vertex hosts still match, and the existing
host-suffix-spoofing rejection is unaffected.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit d0787a3

@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 22:43 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 22:43 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 22:43 — with GitHub Actions Waiting
@tonypiazza
tonypiazza requested a deployment to ml-commons-cicd-env-require-approval September 26, 2026 22:43 — with GitHub Actions Waiting
@tonypiazza

Copy link
Copy Markdown
Contributor Author

@ylwu-amzn pushed d0787a34 addressing the PR-Agent regex suggestion.

([a-z0-9-]+-)? let the optional region prefix start with a hyphen, so https://--aiplatform.googleapis.com/... and https://-us-aiplatform.googleapis.com/... matched the default allowlist. Neither can resolve, since a DNS label cannot begin with a hyphen, so this was not exploitable, but the allowlist should only describe well-formed hostnames. It is now ^https://([a-z0-9][a-z0-9-]*-)?aiplatform\.googleapis\.com/.*$ with tests for both rejections. The suggestion's own example -aiplatform was already rejected: the old group required at least two characters, so a single leading hyphen never matched.

On the second suggestion, I left the ADC test as is. GoogleCredentials.create(AccessToken) returns the token without a refresh, which is what the passing test shows, and adc is built before the static mock opens deliberately, since mocking the GoogleCredentials statics also stubs create(). Moving construction inside the try block would defeat that.

This also needs the backport 3.9 label. The google_cloud connector ships in 3.9 and item 1 leaves it unreachable with the default allowlist. I don't have permission to add labels.

Every google_cloud blueprint fails if followed step by step, and two of them
describe behaviour the code does not implement.

1. The connector feature flag was never mentioned.

plugins.ml_commons.connector.vertexai_enabled defaults to false, and
ConnectorProtocolValidator#validateProtocolEnabled rejects a google_cloud
connector with 403 while it is off. All four blueprints went from "add trusted
endpoint" straight to connectors/_create, so following any of them produced a
403 on the first real step. Folded the setting into the existing cluster-settings
call rather than adding a step, and said what happens without it.

The check also runs on connector update, model register and model update, so
there is no path around it.

2. The streaming blueprint also omitted plugins.ml_commons.stream_enabled.

It too defaults to false. RestMLPredictionStreamAction rejects the request
before any work with "Streaming is currently disabled", so step 4 of that
blueprint could not succeed either. That blueprint therefore needed both flags.

3. The batch blueprint contradicted itself about actions.

The intro said "It defines three actions" and listed batch_predict,
batch_predict_status and cancel_batch_predict, while the connector section
correctly said only batch_predict is defined and that declaring the other two
breaks the derived URL. Reworded the intro to list the three operations and to
state up front that only batch_predict is declared.

4. The batch blueprint described the wrong source for the task state.

Step 5 said the terminal task state is derived from the cluster-wide
remote_job.status_field and remote_job.status_regex.* settings, "which recognize
the Vertex JOB_STATE_* values by default". Neither half holds.
remote_job.status_field defaults to ["status", "Status", "TransformJobStatus"],
none of which is the state field Vertex reports, and
remote_job.status_regex.completed is "(complete|completed|partiallyCompleted)",
which does not match JOB_STATE_SUCCEEDED.

GetTaskTransportAction takes the connector's batch_job_status mapping when one is
declared and only falls back to those settings otherwise, which is what the same
blueprint already said two sections earlier. Step 5 now points at
batch_job_status and notes that the block is required rather than optional.

5. The Gemini blueprint overstated what token_uri accepts.

It said token_uri is "restricted to Google token endpoints (*.googleapis.com over
HTTPS)". validateTokenUri requires exactly oauth2.googleapis.com, over HTTPS, on
the default port; us-central1-aiplatform.googleapis.com is rejected, and there is
a test for that. Replaced with the actual rule and the default value.

The legacy gcp_vertexai_connector_embedding_blueprint.md is not affected: it
predates this connector, uses "protocol": "http" with a hand-managed
Authorization header, and validateProtocolEnabled only gates google_cloud.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
plugins.ml_commons.trusted_connector_endpoints_regex is a Setting.listSetting
with 12 default patterns. Every Vertex AI blueprint instructed a PUT carrying a
single Vertex pattern, which replaces that list rather than adding to it, so
following any of them silently broke SageMaker, OpenAI, Cohere, DeepSeek, AI
Studio Gemini and all three Bedrock endpoints in the same cluster.

The first commit in this PR adds a default pattern for the Vertex AI host, which
makes the step unnecessary as well as harmful. Removed it from all five
blueprints and replaced it with a note that the host is trusted by default, plus
the replace-not-append caveat for clusters that have overridden the setting.

Connector#validateConnectorURL resolves ${parameters.*} through StringSubstitutor
before testing the URL, so the placeholder hosts in these blueprints are resolved
first. Verified that every action URL in all five blueprints resolves to a host
matching the new default, that each referenced parameter is declared, and that
the malformed-prefix and suffix-spoofing cases are still rejected.

gcp_vertexai_connector_embedding_blueprint.md is included even though it predates
the google_cloud protocol and needs no feature flag, because it carried the same
allowlist-replacing PUT. Its section now also states why the flag does not apply
to it.

The same replace-not-append instruction appears in 37 blueprint docs repo-wide.
That is pre-existing and vendor-wide, and will be a separate PR.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
@tonypiazza tonypiazza changed the title Fix google_cloud/Vertex AI trusted endpoints and grpc dependency surface Fix google_cloud/Vertex AI trusted endpoints, grpc surface, and the blueprints Sep 27, 2026
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit c46e99f

@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 27, 2026 03:50 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 27, 2026 03:50 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 27, 2026 03:50 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 27, 2026 03:50 — with GitHub Actions Failure
@tonypiazza

Copy link
Copy Markdown
Contributor Author

@ylwu-amzn @akolarkunnu @jiapingzeng Could one of you take a look when you get a chance?

@ylwu-amzn you have the most context here: this builds on the token_uri port pin in #5041 and the connector from #4921. The validateTokenUri rules are unchanged. The only GoogleCredentialProvider change is translating a missing io.grpc.Context into an MLException naming the module, plus test coverage for fromConnector, which had none because every executor test pre-injects a provider.

The substance is that the connector could not be used by following its own blueprints: no default trusted endpoint matched a Vertex host, vertexai_enabled defaults to false and no blueprint mentioned it, and every blueprint instructed a PUT that replaces the trusted-endpoint list rather than adding to it. Full reasoning is in the description.

}
}
```
No cluster-settings change is needed. The Vertex AI host is already matched by the default

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This default only exists from the release shipping this PR, but readers on 2.x/3.9 follow these blueprints too and will now hit "Connector URL is not matching the trusted connector endpoint regex". Suggest a version qualifier ("Starting in 3.10 ...; on earlier versions, append a Vertex pattern to your current list") in all five blueprints, or backport the regex to 3.9.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 81440ca0b, with the recipe's placeholder hardened in 47012ba7b. All five blueprints now state that the default covers 3.10 and later, and give a check-then-append recipe for earlier versions.

Worth noting for this file specifically: it is present on 2.19 and 3.8 as well, and those branches will never gain the default, so it needed qualifying permanently rather than only until a backport lands. Its version note is therefore inverted relative to the other four, since its stated purpose is to serve versions predating the google_cloud protocol.

The recipe reads the effective list before writing it, because a plain GET _cluster/settings omits settings that were never explicitly set, so a reader could not otherwise see the list they are about to replace.

Comment on lines 10 to 25
## 1. Enable the connector and streaming

The `google_cloud` connector and the streaming API are both opt-in, and both default to `false`.
Without `plugins.ml_commons.connector.vertexai_enabled`, creating the connector in step 2 fails
with `403`. Without `plugins.ml_commons.stream_enabled`, the `_predict/stream` call in step 4
fails with "Streaming is currently disabled".

```json
PUT /_cluster/settings
{
"persistent": {
"plugins.ml_commons.trusted_connector_endpoints_regex": [
"^https://.*-aiplatform\\.googleapis\\.com/.*$"
]
"plugins.ml_commons.connector.vertexai_enabled": true,
"plugins.ml_commons.stream_enabled": true
}
}
```

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still not enough for step 4: _predict/stream also needs the transport-reactor-netty4 and arrow-flight-rpc plugins, http.type: reactor-netty4, and opensearch.experimental.feature.transport.stream.enabled: true (static, restart required). Without them the call fails with "The engine does not support HTTP streaming". Suggest linking the prerequisites in the doc site's predict-stream.md.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added in 7ac4215bf, linking the Predict Stream API prerequisites rather than duplicating a platform-level list that would rot.

Two corrections from verifying it:

The failure is not "The engine does not support HTTP streaming". With stream_enabled true and http.type left at netty4, _predict/stream returns a 500 ClassCastException naming internal channel classes, because RestMLPredictionStreamAction cast to StreamingRestChannel unguarded. RestMLExecuteStreamAction already guards the same cast and responds with a validation error, so this PR mirrors that check and reuses its message verbatim.

The linked page also omits arrow-base. opensearch-plugin install arrow-flight-rpc fails with "Missing plugin [arrow-base], dependency of [arrow-flight-rpc]". That affects 3.7 through main, since arrow-flight-rpc has declared extendedPlugins = ['arrow-base'] from 3.7.0; 3.3 to 3.6 are correct as written because arrow-base did not exist yet.

Verified after the change: streaming works end to end against real Vertex AI, 15 SSE frames terminating with is_last: true and no error frames.

Addresses @ylwu-amzn's review comment on opensearch-project#5103.

Deleting the trusted-endpoint step outright was wrong. It assumed the new default
allowlist pattern exists wherever the blueprint is read, and it only lands in
3.10. A reader on 3.9 following main's docs would get no Vertex pattern at all
and hit "Connector URL is not matching the trusted connector endpoint regex".
For gcp_vertexai_connector_embedding_blueprint.md the gap is permanent rather
than temporary: that file is present on 2.19 and 3.8, which will never gain the
default, and its stated purpose is to serve versions predating the google_cloud
protocol, so its audience is exactly the readers for whom the default is absent.

All five blueprints now state which versions the default covers and give a
check-then-append recipe for the rest. The check is necessary because a plain
GET _cluster/settings omits settings that were never explicitly set, so a reader
cannot otherwise see the list they are about to replace.

Verified against a live single-node 3.10.0-SNAPSHOT cluster running a plugin
build that predates the default, which reproduces the pre-3.10 state the new
text addresses:

  - include_defaults=true reports the 12 defaults; persistent and transient are
    absent until set.
  - Assigning a single-element list leaves 1 effective pattern and drops the
    defaults entirely.
  - In that state, creating an OpenAI connector fails with 400 "Connector URL is
    not matching the trusted connector endpoint regex".
  - Re-sending the 12 defaults plus the Vertex pattern yields 13, after which the
    OpenAI connector creates successfully and a google_cloud ADC connector using
    a ${parameters.location} Vertex host also creates successfully.
  - With vertexai_enabled false, connector creation returns 403 "The Vertex AI
    (google_cloud) connector is not enabled." The blueprints now quote that
    message rather than only the status code.

One correction found while verifying: combining filter_path with
flat_settings=true returns an empty object, because the flattened key is a single
dotted string and **. matches nested levels only. The documented GET therefore
omits flat_settings.

Step 1 keeps its number and absorbs the trusted-endpoint material under a
retitled heading, so nothing renumbers. Checked first that no cross-reference
points at step 1 and that no anchor links target these files; the only numeric
references are to steps 2 and 4.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
Addresses @ylwu-amzn's second review comment on opensearch-project#5103.

RestMLPredictionStreamAction cast the channel to StreamingRestChannel without
checking. On a node that is not configured for HTTP streaming the channel is a
plain RestChannel, so the call failed with a 500:

  class_cast_exception: RestController$ResourceHandlingHttpChannel cannot be
  cast to class org.opensearch.rest.StreamingRestChannel

That names internal channel classes and says nothing about the missing
configuration. RestMLExecuteStreamAction already guards the same cast and
responds with a validation error, so this applies the identical check and reuses
its message verbatim, leaving the two streaming endpoints consistent.

Reproduced on a single-node 3.9.0 cluster with plugins.ml_commons.stream_enabled
true and http.type left at netty4: create a google_cloud ADC connector, register
and deploy a remote model, then POST _predict/stream. Registration and deploy
need no GCP credentials because neither makes an upstream call.

The existing 14 tests in RestMLPredictionStreamActionTests could not catch this.
prepareRequest returns a RestChannelConsumer and the cast happens inside that
lambda, but every test only asserts the lambda is non-null, so the cast never
executed. The new test drives handleRequest with a FakeRestChannel, which is a
plain RestChannel, and asserts a 400 carrying the validation message. Confirmed
it fails with the original ClassCastException when the guard is reverted.

The streaming blueprint also now states that stream_enabled alone is not enough,
because step 4 additionally needs the transport-reactor-netty4, arrow-base and
arrow-flight-rpc plugins and two static settings, and links the Predict Stream
API prerequisites rather than duplicating a platform-level list that would rot.

One gap found while verifying this, outside this repo and not worked around here:
the documented prerequisites omit arrow-base. Installing arrow-flight-rpc without
it fails with "Missing plugin [arrow-base], dependency of [arrow-flight-rpc]".
The JVM options that arrow-flight-rpc requires, including
-Dio.netty.allocator.numDirectArenas=1, are documented correctly in that page's
step 3.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
The -Dstreaming=true cluster installed transport-reactor-netty4 and
arrow-flight-rpc but not arrow-base. The plugin CLI rejects arrow-flight-rpc
without it:

  Missing plugin [arrow-base], dependency of [arrow-flight-rpc]

So the streaming cluster could not start at all. Adds arrow-base with the same
lookup and the same missing-artifact error as its two neighbours, installed
before arrow-flight-rpc because install order decides whether the dependency
check passes. All three live under plugins/ in OpenSearch core, so the existing
path pattern applies unchanged.

Verified end to end against a local OpenSearch core checkout at 3.10.0:

  - All three plugins assemble from core as 3.10.0-SNAPSHOT zips, which is what
    the version filter in this block matches on.
  - integTest -Dstreaming=true now brings the cluster up: the node reaches
    "started", all four plugins load, http.type is reactor-netty4, and
    arrow-flight-rpc initialises rather than merely loading, logging "Arrow
    Flight server started. Listening at [grpc+tcp://127.0.0.1:9400]".
  - Integration tests execute against that cluster: 3 run, 0 skipped, 0 failures.
  - Adding the block does not affect normal builds, which still configure
    cleanly, and -Dstreaming=true without OPENSEARCH_CORE_PATH still fails with
    the existing message.

So the prerequisite set in this block is sufficient to bring up a working
streaming cluster, not merely necessary. The netty allocator system properties
and the arrow --add-opens argument were already here and are both required; the
node refuses to start without -Dio.netty.allocator.numDirectArenas=1.

This still does not make streaming testable in CI. No workflow passes
-Dstreaming=true, and transport-reactor-netty4 is not published as a
3.10.0-SNAPSHOT maven artifact, unlike arrow-base and arrow-flight-rpc which
are, so CI would have to build it from core. -Dstreaming=true therefore remains
opt-in and local, and there is still no streaming integration test.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
The append recipe used "<each pattern returned above>" as the placeholder for the
existing patterns. That is a conventional docs placeholder, but it is also a
syntactically valid string, so a reader who copied the block verbatim would
assign a two-element list and drop the defaults. That is precisely the defect
this PR is fixing, reintroduced by the instructions meant to prevent it.

Replaced with an instruction in the value position, which cannot be mistaken for
a usable pattern. The surrounding prose already told the reader to include every
pattern the response returned; this makes ignoring it fail loudly rather than
silently.

Verified every JSON request body in all five blueprints still parses. The one
block that does not is a pre-existing truncated response example containing an
ellipsis inside an embedding vector, unchanged by this commit.

Signed-off-by: Tony Piazza <tony.piazza@aiven.io>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 47012ba.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

PathLineSeverityDescription
plugin/build.gradle350highNew build plugin added to the integration test cluster: arrow-base is now resolved from OPENSEARCH_CORE_PATH and installed before arrow-flight-rpc. Per mandatory review policy, all build plugin additions must be flagged for maintainer verification regardless of apparent legitimacy.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 30, 2026 02:28 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 30, 2026 02:28 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 30, 2026 02:28 — with GitHub Actions Failure
@tonypiazza
tonypiazza had a problem deploying to ml-commons-cicd-env-require-approval September 30, 2026 02:28 — with GitHub Actions Failure
@tonypiazza

Copy link
Copy Markdown
Contributor Author

@ylwu-amzn pushed 47012ba7b. Both of your review comments are answered inline; the updated description has the full detail. Three things need you.

On the Code-Diff-Analyzer block. The flagged arrow-base addition is not a new external dependency. It is an official OpenSearch core plugin resolved from the same OPENSEARCH_CORE_PATH checkout as transport-reactor-netty4 and arrow-flight-rpc, which that block already pulled the same way, so the artifact source is unchanged. It is also not optional: arrow-flight-rpc has declared extendedPlugins = ['arrow-base'] since 3.7.0, so the plugin CLI refuses to install it unless arrow-base is present first. That block could not start a cluster before this change rather than being loosened by it, and it only executes under -Dstreaming=true, which no workflow passes, so it cannot affect CI or any shipped artifact.

  1. skip-diff-analyzer label if you are satisfied with the above, then re-run the failed action. The High finding is a hard merge block and I cannot apply labels.
  2. backport 3.9 label. The connector shipped in 3.9 and its own blueprints do not work there, so these docs fixes matter most on that branch.
  3. A heads up rather than an ask. RestMLRemoteInferenceIT and RestMLGuardrailsIT fail because OpenAI deprecated gpt-3.5-turbo-instruct, which is hardcoded in four test files. It is repo-wide, unrelated to this PR, and will fail every PR until those are updated.

Happy to walk through any of it.

@tonypiazza

Copy link
Copy Markdown
Contributor Author

@ylwu-amzn thanks for re-running it. Attempt 2 failed identically because the label has to go on first: the analyzer re-evaluates the same diff otherwise, so there is nothing new in its output.

The order it wants is skip-diff-analyzer label, then re-run failed actions. Quoting its own note:

You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions.

The finding is the arrow-base line in plugin/build.gradle. Recapping the reasoning so it is in one place: arrow-base is an official OpenSearch core plugin resolved from the same OPENSEARCH_CORE_PATH checkout as transport-reactor-netty4 and arrow-flight-rpc, which that block already pulled the same way, so the artifact source is unchanged. It is also not optional, since arrow-flight-rpc has declared extendedPlugins = ['arrow-base'] since 3.7.0 and the plugin CLI refuses to install it otherwise. That block could not start a cluster before this change. It only executes under -Dstreaming=true, which no workflow passes, so it cannot affect CI or any shipped artifact.

No rush on the backport 3.9 label, that one is independent.

This branch had an error being deployed

1 failed deployment
ml-commons-cicd-env-require-approval — 47012ba7 Deployed Sep 30, 2026 by tonypiazza via Build and Test MLCommons Plugin on Windows (21) #14570
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants