Skip to content

feat(storage): add ManagedFolder and Folder, and fix three defects in existing Storage code - #174

Merged
naxty merged 10 commits into
mainfrom
naxty/gcpStorageBatch
Aug 31, 2026
Merged

feat(storage): add ManagedFolder and Folder, and fix three defects in existing Storage code#174
naxty merged 10 commits into
mainfrom
naxty/gcpStorageBatch

Conversation

@naxty

@naxty naxty commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Adds the two Storage folder types — and fixes three defects in Storage code that
shipped before this PR.

Branched off main, not stacked on the other batches, so it can be reviewed
and merged independently.

Case Result
storage-managed-folder crud 8/8, discovery 4/4
storage-folder crud 8/8, discovery 4/4
bucket (pre-existing) passes — included deliberately, see below

Verified through debug-conformance.yml; the local key is 403 on Storage.

The fixes

GCP::Storage::Bucket's resolvable named properties that do not exist. It
pointed at "Id", "SelfLink" and "Name"; the resource declares id and
name in lower case and has no selfLink at all. So bkt.res.name resolved to
no value and any resource referencing a bucket reached the plugin with the
reference unresolved
. Latent since it was written, because nothing in the
repository referenced a bucket until these folder types did.

Storage names containing a slash were truncated on a native-ID round trip.
Both folder types are named with a trailing slash that is part of the identity
("reports/" is not "reports"); the parser took a single path segment, so the
slash was dropped and the rebuilt URL addressed something that does not exist.

Neither folder type was discoverable. With no properties, discovery can name
no bucket, so the path builder fell through to its project-scoped branch and
asked /projects/{p}/managedFolders. Now walks the buckets.

Notes for review

This is the only batch that changes shared code green cases depend on, so the
pre-existing bucket case is in the CI run above rather than assumed safe. The
parser change is behaviour-preserving by construction — for an ACL entity, a
default object ACL or an anywhere cache the name is a single segment, so joining
the remainder returns the same string and PathEscape is a no-op — and unit
tests pin that, because the package's own tests are integration-tagged and do
not run without real GCP.

Bucket gains iamConfiguration.uniformBucketLevelAccess and
hierarchicalNamespace: a managed folder requires the first, a folder requires
the second, and neither could otherwise be declared. Both are additive and
optional, with hasProviderDefault on the fields GCS populates itself.

The trailing slash is a type constraint (String(endsWith("/"))), not a
comment, so a fixture that omits it fails at eval rather than after creating a
bucket.

The bucket walker is the sixth copy of this shape in the plugin — Service
Directory, Spanner, Cloud SQL, DNS, Bigtable, Storage. Six packages hand-writing
the same walk because the generic List cannot find a nested resource GCP offers
no wildcard for. It belongs in base; not done here to keep this branch to one
service.

What I could not verify

  • HmacKey and Notification are not included. HMAC keys need a PUT state=INACTIVE before delete and a service-account email a fixture cannot
    know; notifications need a Pub/Sub topic plus an IAM grant for the GCS service
    agent.
  • The commented-out ObjectAccessControl says base "doesn't support resources
    that need TWO parent properties (bucket + object)". The GrandParentType
    support added in feat(servicedirectory): add Namespace, Service and Endpoint #165 does now provide that — but it is not enough to
    close the gap: an object-scoped ACL needs an object to attach to, and there
    is no GCP::Storage::Object type, so no fixture could create one. Objects are
    uploaded through the media endpoint rather than created through this API
    surface. The gap stays recorded.

Correction (2026-08-28): this body reported CRUD as 8/8. That was wrong. The
Replace step reports [~]skipped, not passed: the harness only runs it when a
testdata/<case>-replace.pkl fixture exists, and no case in this repo has one (0 of
118). Read every 8/8 above as 7 passed, Replace not exercised. The same applies
to every GCP resource already on main — the Replace path has never been covered
here. Nothing in this PR regressed.

naxty added 10 commits August 28, 2026 11:33
…espace

Groundwork for managed folders and folders, neither of which is registered yet.
GCS refuses to create a managed folder in a bucket that still has per-object
ACLs, and a bucket is created flat or hierarchical and cannot convert - so both
settings have to be declarable before either type can have a fixture.

Both are additive and optional, with hasProviderDefault on the fields GCS fills
in itself (publicAccessPrevention, lockedTime) so an unset-but-returned value is
not read as drift. The existing bucket fixture still evaluates.
Both are named with a trailing slash that is part of the identity - "reports/"
is a folder, "reports" is not - and the storage native-ID parser took a single
path segment, so the slash was dropped and the rebuilt URL addressed something
that does not exist. The name is now taken whole and escaped when addressed.

That change is deliberately behaviour-preserving for what already worked: for an
ACL entity, a default object ACL or an anywhere cache the name is one segment,
so joining the remainder returns the same string and PathEscape is a no-op.
Unit tests pin both halves, because the pre-existing storage tests are
integration-tagged and do not run without real GCP - the existing bucket case is
included in the CI run for the same reason.

The schema states the trailing slash as a constraint rather than a comment, so a
fixture that omits it fails at eval instead of after creating a bucket.

A managed folder needs uniform bucket-level access and a folder needs a
hierarchical namespace, so Bucket now models both.
Discovery scanned GCP::Storage::ManagedFolder on every pass and never received
a single result. With no properties it can name no bucket, so the path builder
fell through to its project-scoped branch and asked /projects/{p}/managedFolders
- a URL that addresses nothing. GCS has no wildcard for the bucket segment.

The out-of-band create had already proved the rest works: the folder was created
with native ID b/<bucket>/managedFolders/formae-reports/, trailing slash intact.

Sixth copy of this shape in the plugin - Service Directory, Spanner, Cloud SQL,
DNS, Bigtable and now Storage. Six packages hand-writing the same walk because
the generic List cannot find a nested resource GCP offers no wildcard for. It
belongs in base; the comment says so.
BucketResolvable named "Id", "SelfLink" and "Name". The resource declares
id and name in lower case and has no selfLink at all, so bkt.res.name resolved
to no value and any resource referencing a bucket reached the plugin with the
reference unresolved - which is why the folder cases failed at Create in an
apply while succeeding in the out-of-band path, where the harness resolves
references itself.

Nothing in the repository referenced a bucket until these folder types did, so
the defect has been latent since the resolvable was written. selfLink is removed
rather than corrected: there is no such property to point at.
The credential a service account presents to the S3-compatible XML API, and the
last Storage type that needs no prerequisite beyond a service account.

Three things the generic engine cannot express, so create and delete are
hand-written:

  - the key is created by naming its service account in a query parameter, with
    no request body at all;
  - the create answers {metadata, secret} while a get answers the metadata
    directly, so reads would otherwise disagree with creates on every field;
  - GCS refuses to delete a key that is still ACTIVE, so delete reads the key,
    PUTs state=INACTIVE if needed, and only then deletes.

The secret is dropped rather than stored: it is returned once and never again,
so keeping it would both persist a live credential and guarantee drift on the
next read.

testdata/config/vars.pkl now exposes gcpProjectNumber. The default compute
service account's email is built from the project number rather than its id,
and every workflow that runs conformance already sets GCP_PROJECT_NUMBER -
checked, because vars.pkl is imported by every fixture and a missing variable
would break all of them.
CI's service account cannot create one:

    does not have storage.hmacKeys.create access to the Google Cloud project.
    Permission 'storage.hmacKeys.create' denied [reason: forbidden]

The code may well be right - the create, the wrapped response and the
deactivate-before-delete were all written against the documented contract - but
it cannot be conformance-tested by either identity, and shipping an unverified
resource type is exactly what the rest of this work avoided.

Reverted whole rather than left dormant, including the gcpProjectNumber
addition to vars.pkl: with no resource using it, it is dead config in a file
every fixture imports.

This reverts commit 47da6d3.
@naxty
naxty merged commit 1f727a8 into main Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant