Repository navigation
feat(api): add managed custom-domain TLS types - #12
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
530d589 to
7e1347c
Compare
|
@codex review |
![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: 7e1347c8a2
ℹ️ 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".
| type: object | ||
| description: OAuth provider API response envelope | ||
| required: | ||
| - provider | ||
| - endpoint | ||
| - status_code | ||
| - data | ||
| properties: | ||
| provider: | ||
| type: string | ||
| enum: [google, github, microsoft, apple] | ||
| endpoint: | ||
| type: string | ||
| status_code: | ||
| type: integer | ||
| minimum: 100 | ||
| maximum: 599 | ||
| data: | ||
| description: Raw JSON value returned by the OAuth provider's API | ||
| additionalProperties: true | ||
| description: Raw response from the OAuth provider's API |
There was a problem hiding this comment.
The response is now described and returned as the provider's raw JSON, but this schema still restricts it to an object. For endpoints with a top-level array—most notably the documented GitHub /user/repos example—the generated transport passes the list to CallOAuthProviderAPIResponse200.from_dict(), where dict(src_dict) raises instead of returning the JSON through Auth.call_oauth_api; scalar or null responses fail similarly. The previous test fixture even exercised a repository list before being changed to an artificial wrapper object. Model the response as an arbitrary JSON value rather than type: object.
Useful? React with 👍 / 👎.
| mode: BYOCFrontendCustomDomainTLSConfigMode | ||
| certificate_pem: str | ||
| private_key_pem: str |
There was a problem hiding this comment.
Existing custom-domain clients import FrontendCustomDomainTLSConfig and can construct it with only certificate_pem and private_key_pem, because mode previously defaulted to byoc. This regeneration deletes that module and class, replaces it with BYOCFrontendCustomDomainTLSConfig, and makes mode mandatory, so every existing BYOC integration fails at import time (or at construction after changing the import) even though BYOC remains supported. Retain a compatibility alias/model with the prior constructor behavior while adding the managed variant.
Useful? React with 👍 / 👎.
7e1347c to
679db2b
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
679db2b to
c0cc37b
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
c0cc37b to
57a3843
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Vendor the managed custom-domain TLS schemas from the current Hosting bundle: the create request uses the flat FrontendCustomDomainTLSConfig with managed and byoc modes, responses expose failure_reason, and the domain and PEM fields carry length limits. Drop the discriminated create request types that Hosting no longer defines.
Take the custom-domain TLS area from the final Hosting bundle: responses expose routing_target_hostname and deprecate required_routing_record, the routing record drops zone_apex_record_type, verification_status includes failed, and createFrontendCustomDomain returns a typed 409 with the caller's ownership record. Pin the new shapes in native tests.
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 077de0f5fa
ℹ️ 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".
| certificate_pem: str | Unset = UNSET | ||
| private_key_pem: str | Unset = UNSET | ||
| certificate_chain_pem: str | Unset = UNSET |
There was a problem hiding this comment.
When a caller selects BYOC, these optional fields make FrontendCustomDomainTLSConfig(mode="byoc") a well-typed request that serializes only the mode, even though the documented create contract requires both certificate and key; conversely, managed mode accepts and serializes forbidden PEM fields. The surrounding OpenAPI not rules are not represented in the generated constructor, so invalid combinations survive type checking and fail only at the API. Express the create-time modes as a native discriminated oneOf, as the project-config schema already does, so generation produces variant-specific constructors.
AGENTS.md reference: AGENTS.md:L3-L5
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not changing. This SDK vendors the hosting wire contract verbatim and doesn't hand-edit generated code, so contract shape is decided in Kong/volcano-hosting#1021. Main's create schema is already flat with mode required, so widening the enum to [managed, byoc] keeps every existing generated client source-compatible, including the CLI's. A oneOf would be a breaking change for them. The server enforces the mode-specific rules and the schema descriptions state them. None of the SDKs exposes a public custom-domain facade over this generated type.
Restore the BYOC default on the custom-domain TLS mode and take the final Hosting wording for verification_status, the ownership failure reason, and createFrontendCustomDomain takeover rules. Regenerate the client: FrontendCustomDomainTLSConfig now defaults mode to byoc on construction and always sends it, while decoding still requires it.
Take Hosting's manifest TLS contract: BYOCProjectConfigFrontendCustomDomainTLSConfig no longer requires mode, and ProjectConfigFrontendCustomDomainTLSConfig drops its mode discriminator and documents the BYOC default. Regenerate the client so a manifest TLS block without mode decodes as BYOC and round-trips without adding one, while mode: managed still selects the managed variant.
Take the openapi-python-client 0.29.1 bump and regenerate the client from the vendored spec, so the custom-domain TLS models pick up the new generator output.
marckong
left a comment
marckong
left a comment
There was a problem hiding this comment.
PR Review
Verdict: approve
Reviewed head: cb1ab5182b304bac044b3488c235924fa14e43d6
Overview
The PR regenerates the private Python wire models and custom-domain create operation for managed TLS, ownership-conflict records, failure states, and manifest TLS. No confirmed diff-scoped defect was found.
Verification
- Read the complete diff and exact-head request/response models, enum checks, union decoding, operation status mapping, and tests. Traced optional-field omission with UNSET, BYOC constructor defaults, managed serialization, mode-less manifest BYOC fallback, and 409 conflict decoding while other failures retain Error.
- Independently parsed and compared all nine affected schemas with Hosting #1021; each matches structurally.
- Parsed all 998 Python source files with Python 3.14
ast.parse; all passed. The system Python 3.9 could not parse pre-existing pattern matching, consistent with the SDK's declared Python >=3.11 requirement; validation was repeated with a supported interpreter. No PR module was imported or executed. - Captured GitHub metadata shows passing Python 3.11–3.14 test jobs, quality/mutation gates, CodeQL, and security checks. I did not rerun the native suite, generator freshness, package checks, or live acceptance locally.
Findings
No findings.
Lane completeness
Completed contract/compatibility, correctness, security, resource/performance, maintainability, and test coverage passes.
Strengths and notes
Tests cover request round trips, lifecycle DNS records, unknown failure categories, omitted ownership fields, legacy routing records, manifest variants, and error/status mapping. Generated code remains private and handwritten public facades do not depend on these changed transport models.
2 participants
Vendors the managed custom-domain TLS contract from Kong/volcano-hosting#1021, so SDK users get typed managed and BYOC custom domains.
What changes
tls.modeismanagedorbyocand defaults tobyoc.modestays required and the TLS config stays a flat object, so clients already generated from the BYOC-only contract still compile. Managed requests send no certificate fields. BYOC requests send a certificate and key, plus an optional chain.verification_status(pending,verifiedorfailed),failure_reasonfor managed domains (provider,certificate,ownershiporinternal),verification_records, androuting_target_hostname.required_routing_recordis deprecated, and the server no longer returns it.FrontendCustomDomainConflictError. This isErrorplus an optionalrequired_record. A managed request for a hostname that another account has reserved without proving ownership getscode: ownership_verification_requiredand the_volcano.<host>TXT record to publish.custom_domain.tlsis aoneOfonmode, either{mode: managed}or{mode: byoc}with certificate and key.Only the custom-domain TLS part of the hosting spec is vendored. Unrelated hosting drift is left to the regular OpenAPI sync. Generated code stays private under
volcano_sdk._generated, and no handwritten code depends on the changed models.Review guide
openapi/openapi.yaml: the TLS schemas and the custom-domain operations.src/volcano_sdk/_generated/: regenerated. Don't review it line by line.src/volcano_sdk/_tests/test_managed_tls_contract.py: request and response round-trips using fixtures from the hosting docs. Covers the 409 conflict decoding, 400 and 503 still decoding asError, and thebyocdefault.Validation
uv run --locked poe checkspasses: policy, audit, generated-code freshness, ruff, mypy, basedpyright, the full test suite at 100% coverage, and the package checks.Merge before the hosting stack (Kong/volcano-hosting#1020 → #1167 → #1168 → #1576 → #1021). Part of VOL-314.