feat(trusted-issuer): expose enable_token_review and apiserver URL - #14
Merged
Conversation
The server has accepted enable_token_review and kubernetes_api_server_url on _add_trusted_issuer / _update_trusted_issuer, and returned them on the TrustedIssuer type, for some time. This SDK carried neither: the request dataclasses had no such fields, and TrustedIssuer.from_dict drops unknown keys through _known(), so a value the server returned was silently discarded. Kubernetes TokenReview was therefore unreachable from Python at all — not awkward, unreachable. Adds the pair to AddTrustedIssuerRequest, UpdateTrustedIssuerRequest and the TrustedIssuer response, plus TRUSTED_ISSUER_FRAGMENT so the GraphQL selection actually asks for them. Verified against a live server, not just by unit test: set both through add_trusted_issuer and read both back through trusted_issuers. Also corrects a stale comment listing spiffe_bundle_endpoint as a valid key_source_type. It has no fetcher server-side and is now rejected at write time, so a caller following that comment gets an error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while auditing the SDKs against the current server surface.
Kubernetes TokenReview was unreachable from Python
The server accepts
enable_token_reviewandkubernetes_api_server_urlon_add_trusted_issuer/_update_trusted_issuerand returns them onTrustedIssuer. This SDK had neither:TrustedIssuer.from_dictpasses through_known(), which drops unknown keys — so even when the server returned them, they were silently discardedTRUSTED_ISSUER_FRAGMENTdidn't select them, so GraphQL never asked in the first placeNot awkward to use — impossible. Online TokenReview validation could not be configured from Python at all.
The fix
The pair added to
AddTrustedIssuerRequest,UpdateTrustedIssuerRequestand theTrustedIssuerresponse, plus the GraphQL fragment.Documented at the field, because two things about them are easy to get wrong:
kubernetes_api_server_urlis required and must be https wheneverenable_token_reviewis true — the server rejects the write otherwise, and on update it validates the merged row, so enabling review without a previously stored URL also fails.Verified against a live server
Not just unit tests — a real server, set and read back:
117 unit tests pass. (The
tests/integrationsuite needs a live server and fails with connection errors without one, unchanged by this PR.)Also corrected
A comment listed
spiffe_bundle_endpointas a validkey_source_type. It has no fetcher server-side and is now rejected at write time, so a caller following that comment gets an error.Note on the Go SDK
The same gap exists there and cannot be fixed in that repo alone:
authorizer-gopinsauthorizer-proto-go v0.2.0-rc.1, whose generated types predate these fields. The proto repo'smainhas them on all three messages — it needs a tag cut, then ago.modbump, then the GraphQL field-selection update. Filed separately rather than half-fixed here.