Add credential vault SDKs and e2e coverage#1023
Conversation
jwx0925
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab0b6735ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jwx0925
added
feature
ChatGPT Codex Connector
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5310a037d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 441b40f2c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // revision and activates it in Credential Proxy. | ||
| func (c *EgressClient) CreateCredentialVault(ctx context.Context, req CredentialVaultCreateRequest) (*CredentialVaultState, error) { | ||
| var resp CredentialVaultState | ||
| if err := c.doRequest(ctx, "POST", "/credential-vault", req, &resp); err != nil { |
There was a problem hiding this comment.
When ConnectionConfig.UseServerProxy is true, resolveEgress points this client at /sandboxes/{id}/proxy/18080, and the lifecycle proxy always forwards to the sidecar over plain http:// while the sidecar returns 426 for Credential Vault POST/PATCH/DELETE unless the inbound transport is TLS or loopback. In remote/Kubernetes proxy deployments, CreateCredentialVault/PatchCredentialVault/DeleteCredentialVault therefore fail even though reads and normal egress policy calls work; add the same fail-fast/direct-transport handling used by the JS adapter before issuing these write requests.
Useful? React with 👍 / 👎.
| requestJson( | ||
| method = "POST", | ||
| operation = "Create credential vault", | ||
| jsonBody = request.toJsonObject(), |
There was a problem hiding this comment.
When connectionConfig.useServerProxy is enabled, this adapter's egressEndpoint is the lifecycle server proxy path, and the server proxy builds the backend target with plain http://; the egress sidecar rejects Credential Vault writes over that path unless the peer is TLS or loopback. In remote/Kubernetes proxy deployments, create/patch/delete vault operations return 426 while reads still work, so the Kotlin SDK should fail fast for proxied write transports or use a direct/allowed egress endpoint.
Useful? React with 👍 / 👎.
| return await _client.PostAsync<CredentialVaultState>( | ||
| "/credential-vault", | ||
| request, | ||
| cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When ConnectionConfig.UseServerProxy is true, EgressBaseUrl points at the lifecycle server proxy, which forwards requests to the egress sidecar as plain HTTP; the sidecar's write gate rejects Credential Vault POST/PATCH/DELETE unless the request is TLS or loopback. This makes CreateAsync, PatchAsync, and DeleteAsync fail with 426 in remote/Kubernetes proxy deployments even though reads and policy operations work, so the C# SDK should fail fast for proxied writes or route them over an allowed transport.
Useful? React with 👍 / 👎.
| fun getBinding(name: String): CredentialBindingMetadata | ||
| } | ||
|
|
||
| interface Egress : CredentialVault { |
There was a problem hiding this comment.
Making the existing public Egress interface extend every CredentialVault method is a source/binary break for downstream policy-only fakes or custom implementations: code compiled against the previous three-method interface no longer implements Egress, even when Credential Vault is unused. Keep the vault contract separate or provide default/compatibility methods so existing consumers that only need getPolicy, patchRules, and deleteRules do not have to implement the new vault surface.
Useful? React with 👍 / 👎.
1 participant
Summary
Validation
bash -n scripts/credential-vault-e2e-target.sh scripts/python-e2e.sh scripts/go-e2e.sh scripts/javascript-e2e.sh scripts/java-e2e.sh.github/workflows/real-e2e.ymlwithyaml.safe_loaduv run ruff check tests/test_credential_vault_e2e.pyuv run pytest tests/test_credential_vault_e2e.py -qGOCACHE=/private/tmp/opensandbox-go-build-cache go test ./... -run TestCredentialVault -count=1 -vcorepack pnpm exec tsc --noEmit --target ES2022 --module NodeNext --moduleResolution NodeNext --allowImportingTsExtensions --strict tests/test_credential_vault_e2e.test.tscorepack pnpm exec eslint tests/test_credential_vault_e2e.test.tscorepack pnpm exec vitest run tests/test_credential_vault_e2e.test.tsmavenLocalwith JDK 17./gradlew test --tests com.alibaba.opensandbox.e2e.CredentialVaultE2ETest -PskipCodeInterpreterE2E=true --rerun-tasksgit diff --checkC# SDK unit tests were not run locally because
dotnetis not installed in this environment.