feat(dns): add ResourceRecordSet, completing Cloud DNS - #173
Merged
Conversation
Service Directory was absent from the plugin. These three types are the whole of its v1 API: a namespace contains services, a service contains the endpoints a resolve call actually returns. The API rejects a wildcard at every level - "locations/-" answers "Unsupported location", "namespaces/-" answers "Could not parse namespace name" - so services and endpoints are discovered by walking the collections above them, following nextPageToken at each level. A dropped namespace would otherwise hide every service and endpoint under it. The walk is registered from the package init rather than an init of its own: Go runs init functions in filename order and "list.go" sorts before "resources.go", so an override registered there would be silently replaced by the generic registration. base.ParentResourceConfig gains GrandParentType/GrandParentPropertyName for APIs three collections deep. Read, update and delete rebuild the whole path from the native ID, so create was the one operation with nothing but the declared properties to route with. clean-environment.sh sweeps leftover namespaces: the service and endpoint fixtures build one as a prerequisite, and conformance Destroy only removes the resource under test. --filter is server-side for this command and Service Directory rejects "~", so the prefix is matched with grep. Verified against europe-central2: every case crud 8/8 (Replace skipped, no -replace fixture) and discovery 4/4, on released formae and on formae main.
Schemas and provisioners only; fixtures and conformance runs follow.
Instances are project-scoped - a Spanner instance's region is its config, not
a path segment. Two envelopes the config-driven engine cannot express are built
by a request transformer: create is {instanceId, instance:{...}}, with the id
beside the object rather than in a query parameter, and patch carries its field
mask in the body as fieldMask rather than in the query string.
A database has no name field on create; the id goes into a CREATE DATABASE
statement, quoted with backticks for GoogleSQL and double quotes for
PostgreSQL. dropDatabase is a plain DELETE on the resource URL, so the generic
engine handles it.
Backup schedules sit two collections deep and are synchronous. Spanner rejects
a wildcard for both nested collections - instances/-/databases and
databases/-/backupSchedules answer 400 "Invalid List... request" - so
databases and schedules are discovered by walking the collections above them.
All three cases are crud 8/8 and discovery 4/4 against europe-central2. Two failures found and fixed on the way, both real: displayName is rejected outside 4-30 characters, which the fixtures broke and the schema now states as a constraint rather than a comment. backupSchedules.patch accepts exactly three mask paths - encryption_config, retention_duration and spec.cron_spec.text - so UpdateMaskFromBody could not express it: the mask reaches a field inside spec, and that transformer lists the body's top-level fields. The mask is fixed instead, leaving out encryption_config so an update never rewrites a server value nobody declared. The nested cron fields GCP fills in (timeZone, creationWindow) are marked as provider defaults; without that a schedule reads back with two fields no forma declared and Verify fails. clean-environment.sh sweeps Spanner instances. Unlike most leftovers these are billed by the hour: the database and backup-schedule fixtures each build one as a prerequisite and conformance Destroy only removes the resource under test, so every run of those two cases leaves one running. Deleting the instance cascades its databases and schedules, so one sweep covers all three cases. ci.yml and nightly.yml both give spanner-* a 15 minute per-operation timeout. Running both phases back to back under one fixed FORMAE_TEST_RUN_ID, the way CI does, showed no name collision on the surviving instance, so no phase split.
Covers the two request envelopes, the DDL quoting per dialect, both directions of the config id normalisation, the native-ID parser at all three depths, and that the parent-walking List survives registration.
Cloud SQL had instances and databases; these are the three remaining
collections with a full create and delete.
The fix comes first in importance. Every sqladmin mutation answers with an
Operation whose targetLink names the *instance*, and extractSQLNativeID
consulted targetLink before anything else - so a nested resource was stored
under its instance's native ID, two resources shared one id, and the next sync
read the instance and reconciled the nested resource away as absent. That
defect is not new: GCP::SQL::Database has been registered for some time with no
conformance case, so nothing ever exercised the path. testdata/cloudsql-database.pkl
now covers it, and it passes.
Each of the three new types is addressed differently, and none of them by a
name a forma chooses:
User delete takes the name as a query parameter against the collection
while get takes it as a path segment, so delete is hand-written.
password is write-only and createOnly, as AlloyDB's user does.
SslCert addressed by a server-generated sha1Fingerprint, which arrives
nested under clientCert.certInfo on insert and top-level on read.
The private key is returned once and dropped rather than persisted.
BackupRun addressed by a server-generated id, which arrives as
backupContext.backupId on the create Operation. No absolute expiry,
so unlike Spanner's and Bigtable's backups its fixture cannot rot.
All three are discovered by walking the instances: discovery lists with no
properties and sqladmin has no wildcard.
MySQL's host is deliberately not modelled - it is part of a user's identity but
a DeleteRequest carries no properties to read it back from, so supporting it
means encoding it in the native ID.
cloudsql-user is crud 8/8 and discovery 4/4; cloudsql-database is crud 8/8 with
discovery running. The ssl-cert and backup-run cases are written but unrun.
GCP::SQL::Database had no parent-walking List, so discovery - which lists with no properties - asked a collection URL with no instance in it and found nothing. Its new conformance case failed with "resource did not appear in inventory", which is the second defect that one fixture has surfaced in a type that shipped without any coverage. Rather than add a fourth copy of the same walker, the three already in this package are now one. The only thing that differed between them was how an item names itself: users name sslCerts sha1Fingerprint databases name backupRuns id userProvisioner is left with just Delete, which is the one verb the generic engine genuinely cannot express for a user.
…ention sslCerts.insert answers with the certificate itself, so this code called the create synchronous and skipped polling. The certificate really is usable the moment insert returns - but Cloud SQL serialises operations per instance, so reporting the create done while its operation is still running means the next mutation is issued into a busy queue. The conformance Destroy answered 409 "Operation failed because another operation was already in progress". Certificates are now polled like every other sqladmin type. Only the native ID comes from an unusual place, and so does the operation: insert nests it beside the certificate as operation.name rather than putting it at the top level. Separately, that 409 is now retryable across the whole package, alongside the existing "is being accessed by other users" case. Both are sqladmin saying "not yet" rather than "no", and every nested type shares its instance's operation queue - so this helps user, database and backupRun too, not just the case that exposed it.
Cloud SQL does not remove a deleted backup run. The record survives as a tombstone and a get answers 200 with status "DELETED" rather than 404, so the generic Read reported the resource as still present: the conformance OOB-delete step timed out waiting for it to leave inventory. Read now reports NotFound for a tombstone, and the instance walker skips them so discovery does not offer long-dead backups as unmanaged resources to import - the quieter of the two failures, and the one no test here would have caught. The list filter is deliberately a separate function from the id extractor: a create response carries no status yet and still has to yield an id.
Cloud DNS had only ManagedZone. These three cover the rest of what the v1 API
can create and delete, bar ResourceRecordSet, whose delete path takes a
two-segment {name}/{type} identifier unlike anything else in the plugin.
Cloud DNS does not agree with itself about what an identifier is called: a
managed zone and a policy use "name", a response policy "responsePolicyName",
a rule "ruleName". A forma declares "name" for all of them and paired
request/response transformers translate at the API boundary, so three
spellings stay inside the plugin instead of leaking into every forma. The
native-ID extractor accepts all three, because a list item is the only place an
id appears during discovery.
Two more things the generic engine had to be told: the rule collection is
"rules" in the URL but "responsePolicyRules" in a list response, and a rule
is discovered by walking the response policies - discovery lists with no
properties and there is no wildcard for that segment.
Verification runs through debug-conformance.yml rather than locally: the local
service-account key is 403 on every DNS call, while CI's identity creates DNS
zones for the existing managed-zone case.
… kind Two failures from the first debug-conformance run against real GCP. A response policy was never discovered. base.extractNativeIDFromItem reads itemMap["name"] and gives up when it is empty, before it ever calls the API's own native-ID extractor - and a response policy names itself "responsePolicyName". Every item was skipped and Discover timed out with "resource did not appear in inventory". Worth knowing generally: any GCP collection whose items do not use "name" is invisible to the generic List, no matter what the plugin's extractor supports. A small custom List fixes it here rather than changing base under four in-flight branches. Separately, Cloud DNS stamps every local-data record with a "kind", which failed Verify, Sync and Update as a property no forma wrote. It is now declared a provider default, like Spanner's cronSpec fields.
The listing found the rule every time - "Received 1 resources for GCP::DNS::ResponsePolicyRule" on each pass - and discovery still finished with nothing discovered, while the conformance Discover step timed out. Cloud DNS answers a rule read with ruleName, dnsName, localData and behavior, and nothing naming the owning response policy: that lives only in the URL. So a discovered rule arrived without "responsePolicy", a required createOnly property, and was dropped rather than entering inventory. TransformContext carries no parent, so this cannot be a response transformer - the policy has to come from the native ID, which means a Read override.
MaterializedView's schema module already existed with no provisioner, recorded in knownParityGaps - and it described a resource the API does not have. It demanded a cluster, though materialized views are instance-scoped, and omitted query, which is required. Rewritten to match the API; the parity test then refused to pass until its known-gap entry was removed, which is what that test is for. The three new types deliberately bypass BigtableProvisioner. It derives the create-id query parameter by trimming a trailing "s" and appending "_id", which is right for instances and clusters but would ask for "appProfile_id" where the API wants "appProfileId". They use the generic engine's CreateIDParam, which sends exactly what it is given. Two fixes to existing code fell out of writing the fixtures: Bigtable::Table declared instance as a plain String, so a table could only ever name an instance that already existed and could not be declared in the same forma as one. That is why the type still has no conformance case. It now takes a resolvable, as every other nested type here does. The native-ID parser switched on each collection by name with no default, so an unlisted collection parsed to an empty resource type and read nothing. Adding appProfiles and logicalViews as the fifth and sixth special case would have kept the trap; every instance-scoped collection now shares one branch. The package had no unit tests, so the parser has some now.
Two fixture errors, both mine. An app profile with multiClusterRoutingUseAny needs an instance with more than one cluster to route across; the host here is DEVELOPMENT, which has exactly one. It now pins traffic to that cluster. Both view types run SQL over a table, and the table had no column families, so there was nothing for a query to read. Each now declares one. Diagnosed from timing rather than from the API's own message: the agent logs "Updating resource state to Failed" with an empty native ID and nothing else, so a plugin-side create failure carries no status code or message into the job log. Worth fixing on its own.
BigtableProvisioner.Create unmarshals properties and never called base.UnwrapValues, which base.Create does twice. Any property whose value arrived wrapped - which is what a reference to another resource produces - read as empty, so a table declared alongside its instance failed with "instance is required for nested resources" while the instance sat right next to it in the same forma. That is why bigtable's table schema typed instance as a plain String, and why the type has no conformance case: with a literal string a table can only name an instance that already exists, and a reference never worked. Found by running the discovery phase purely to read the error: an out-of-band create reports the plugin's own message, while the same failure inside an apply reports nothing at all.
"instance is required for nested resources" is unfalsifiable from a CI log: the property may be absent, present under another name, or present as a type this code cannot read. A plugin-side create failure carries no other diagnostic out of an apply, so that one line was the whole signal - and it sent me down three wrong explanations in a row. The error now names the value's Go type and the property keys the request actually carried.
BigtableProvisioner embeds *base.BaseResource and overrode only Create, so it inherited the raw BaseResource.Status - which reports success and nothing else. UnifiedProvisioner wraps that with a Read for exactly this reason; a hand-written provisioner has to do the same, or a resource has no properties after an async create completes. The symptom was a reference to a Bigtable instance never resolving. With no properties on the instance there is no ".name" to read, so a table declared alongside its instance reached the plugin with the reference still unresolved and failed with "instance is required for nested resources". This affects Instance, Cluster and Table - every Bigtable type that existed before this batch.
appProfiles.create answers with the AppProfile itself, not an Operation, but
the package's single OperationConfig declares every Bigtable create async. base
therefore looked for an operation id that was never there, got "", and polled
BaseURL + "/" - which answers 404:
failed to get operation status: HTTP 404 error
(URL: https://bigtableadmin.googleapis.com/v2/)
Of the five collections, appProfiles and tables answer with the resource;
clusters, logicalViews and materializedViews answer with an Operation. Only the
app profile needed the synchronous config here - tables go through
BigtableProvisioner, which handles their response shape itself.
…p profiles Some GCP deletes are refused without a force flag. Bigtable answers an app-profile delete with a safety-check failure unless ignoreWarnings=true is sent - and its own discovery document describes the parameter as "Required" while flagging it optional, which is how it went unnoticed. ResourceConfig already had UpdateQueryParams; DeleteQueryParams is the same idea for the delete path. The app profile now sends ignoreWarnings on both, since a routing change needs it too. Create, Verify, Extract, Sync and Update all passed for the app profile before this; Destroy was the last red cell.
App profiles and both view types live under an instance, and discovery lists with no properties - so it can name no instance, the path builder falls back to a project-level collection URL that addresses nothing, and the conformance Discover step times out with "resource did not appear in inventory". This is the fifth instance of the same shape in this plugin (Service Directory, Spanner, Cloud SQL, DNS and now Bigtable): a nested resource that GCP offers no wildcard for has to be found by walking its parents. The app profile's CRUD is 8/8 as of the previous commit; this is what stands between the three new types and a green discovery phase.
The materialized-view fixture failed at Create with
Error in field 'display_name' : value must be between 4 and 30 characters
in length, inclusive, but got length 33
which was my own doing: renaming the host instance from "Logical View" to
"Materialized View" pushed the display name over the cap.
The limit is now stated in the instance schema, so a fixture that breaks it
fails at eval instead of after provisioning an instance - the same treatment
Spanner's displayName got for the same reason.
LogicalView and MaterializedView are out. Both create fine some runs and fail at Create in others with no message the apply path surfaces, and settling that needs Bigtable SQL semantics I cannot test locally - each attempt costs a CI cycle on a concurrency group the nightly also needs. AppProfile is verified 8/8 and 4/4, and the five fixes below stand on their own; holding them hostage to two views was the wrong trade. LogicalView is removed entirely. MaterializedView keeps its corrected schema and goes back to being a recorded parity gap, so the repo is left better than it was found: the module now describes the real API instead of demanding a cluster the resource does not have and omitting the query it requires.
A record set is what a managed zone actually serves, and the last type the DNS
v1 API can create that this plugin did not have.
It is the only resource here addressed by two path segments -
.../rrsets/{name}/{type}. Both travel in the native ID joined by a slash, which
is unambiguous because a DNS name may contain dots but never a slash, and the
path builder appends ResourceName verbatim, so the two-segment address needs no
special case beyond the parser and the id extractor. Create posts to the
collection and takes no id parameter at all.
Record sets are discovered by walking the managed zones, as the response policy
rules already are - Cloud DNS offers no wildcard for that segment.
Left deliberately: every zone carries an SOA and an NS record set nobody
declared, so discovery will surface two per zone as unmanaged. That is the
API's shape rather than something to filter, and the schema says so.
CRUD passed 8/8 - the two-segment identifier works - but Discover timed out. Cloud DNS answers a record-set read with name, type, ttl and rrdatas and nothing naming the managed zone: that lives only in the URL. So a discovered record set arrived without managedZone, a required createOnly property, and was dropped rather than entering inventory. The walk found it on every pass. Identical to the defect the response policy rule had, and it needs the same fix for the same reason: TransformContext carries no parent, so the zone has to come from the native ID, which means a Read override. That is twice in one package and four times across this plugin. A parent that exists only in the URL is a general problem, not a per-resource quirk - worth solving in base rather than a fifth time.
A pull request based on another pull request's branch matched no trigger, so the six stacked pull requests in this chain got no CI at all - no build, no lint, no conformance. Only the four based on main ever ran, and those were separately gated out of conformance by discover-tests. The scope is resolved from the diff, so a stacked pull request runs exactly the fixtures it touches. The push filter is untouched: the full matrix stays a main thing, and a push to a feature branch still runs nothing.
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md # scripts/ci/clean-environment.sh
naxty
added a commit
that referenced
this pull request
Aug 31, 2026
…ustConfig Restores #171. That pull request was stacked, and it merged into its parent branch rather than into main - GitHub marks a stacked pull request merged once its commits reach its base, and its base was a branch. #167, #168, #169 and #170 went the same way but were recovered when #173 landed, because #173 descended from them. Nothing descended from #171, so certificate manager was the one batch left behind: main has carried a Merged badge and no code since. This is that content on top of current main, nothing else - the three types, their schemas, six fixtures, and the package registration. Verified locally against the service account CI uses, all three CRUD 7/7 with Replace skipped and discovery 4/4. Two things it needed, both recorded in the CHANGELOG: roles/certificatemanager.owner rather than editor, which carries no delete permission at all; and a trust config that is not empty, since the API rejects one carrying neither a trust store nor an allowlisted certificate.
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
1 participant
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.
Completes Cloud DNS. Stacked on #170 — deliberately not on the blocked
Certificate Manager draft (#171), so it does not inherit that blockage.
dns-resource-record-setVerified through
debug-conformance.yml; the local key is 403 on DNS. 27s forCRUD, 17s for discovery.
Notes for review
This is the only resource in the plugin addressed by two path segments
(
.../rrsets/{name}/{type}), which turned out to need less machinery thanexpected. Both segments travel in
ResourceNamejoined by a slash and the pathbuilder appends it verbatim — that works because a DNS name may contain dots but
never a slash, so the join is unambiguous. Only the native-ID parser and the id
extractor had to know. Create posts to the collection and takes no id parameter
at all, so the type only matters for read, update and delete.
Record sets are discovered by walking the managed zones, as response policy
rules already are: Cloud DNS offers no wildcard for that segment.
One bug found by the first run. A record set's read carries nothing naming
its managed zone, so discovered record sets arrived without
managedZone— arequired createOnly property — and were dropped instead of entering inventory.
Fixed with a
Readoverride, sinceTransformContextcarries no parent.That is the same defect
ResponsePolicyRulehad, and the fourth instance of theshape across this plugin (also
ServiceDirectory::Service/EndpointandSQL::User). A parent that exists only in the URL is a general problem, not aper-resource quirk — worth solving once in
baserather than a fifth time.Not done here to keep this branch to one type.
What I could not verify
discovery reports two per zone as unmanaged. That is the API's shape rather
than something to filter, and the schema documents it — but it does mean a
project with zones will show unmanaged record sets after this lands.
routingPolicy(weighted / geo / failover records) is not modelled. Thefixture uses a plain A record; anything else needs its own verification pass.
Correction (2026-08-28): this body reported CRUD as
8/8. That was wrong. TheReplace step reports
[~]— skipped, not passed: the harness only runs it when atestdata/<case>-replace.pklfixture exists, and no case in this repo has one (0 of118). Read every
8/8above as 7 passed, Replace not exercised. The same appliesto every GCP resource already on
main— the Replace path has never been coveredhere. Nothing in this PR regressed.