Skip to content

Add GCP permission for secrets encryption - #1944

Merged
Andy Hay (Andy-Hay) merged 8 commits into
mainfrom
add-gcp-permission-for-secrets-encryption
Sep 16, 2026
Merged

Andy Hay (Andy-Hay) merged 8 commits into
mainfrom
add-gcp-permission-for-secrets-encryption

Conversation

@Andy-Hay

Copy link
Copy Markdown
Contributor

What changed?

Add additional optional permission for deploying GCP pipeline with GKE Application-Layer Secrets Enryption

Why?

The feature cannot be enabled without the permission, so customers need to be aware that they have to add it.

AI reviews

Claude will automatically review this PR against the docs style guide.

If you have questions or want it to look again at something specific, tag @claude in a comment.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Documentation style review

Small, focused diff. One grammar issue worth fixing, plus two minor points.

1. Missing article (grammar)

The following role is also required if GKE Application-Layer Secrets Encryption feature is enabled:

The sentence needs an article before the feature name — "if GKE ... feature is enabled" is ungrammatical. The style guide also asks for active voice ("Use active voice: the Collector receives events" — style guide, Writing style; also CLAUDE.md, Writing style) and for addressing the reader as "you".

Suggested fix:

The following role is also required if you enable the GKE Application-layer Secrets Encryption feature:

2. Third-party product name capitalization

GKE Application-Layer Secrets Encryption

The style guide says to "Match third-party official styling" (General technical terms). Google writes this as Application-layer Secrets Encryption, with a lowercase "l" in "layer". Use that spelling.

3. Minor: link host consistency

The new cloudkms.admin link resolves fine (HTTP 200), so this is non-blocking. It points at the docs.cloud.google.com host, while every other link in this list uses cloud.google.com except one pre-existing datastore.owner entry. Consider using cloud.google.com for consistency with the surrounding bullets.

Checked and fine

  • Frontmatter: untouched by this PR.
  • Heading structure, list introduction (sentence ending in a colon), and backtick usage for the role name all follow the conventions of the surrounding list.
  • The external link added in this PR returns HTTP 200.

@Andy-Hay

Copy link
Copy Markdown
Contributor Author

Claude (@claude) can you re-review please ?

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Documentation style review (re-review)

The grammar and capitalization points from the previous round are fixed — the sentence now reads naturally, uses "you", and matches Google's "Application-layer" styling. Two small things remain.

1. Filler words in the conditional

docs/get-started/private-managed-cloud/setup-guide-gcp/index.md:41

The following role is also required if you want to enable the GKE Application-layer Secrets Encryption feature:

"want to" does not add meaning here — the role is required when the feature is enabled. The style guide asks you to remove filler words (Writing style), and the parallel sentence three lines above is already terse ("The following roles are also required if using RDB Loader within GCP"). Suggested:

The following role is also required if you enable the GKE Application-layer Secrets Encryption feature:

2. Link host consistency (carried over, non-blocking)

docs/get-started/private-managed-cloud/setup-guide-gcp/index.md:43

The cloudkms.admin link points at docs.cloud.google.com, while every bullet in this section uses cloud.google.com apart from the pre-existing datastore.owner entry. Both hosts serve the same content, so this is cosmetic, but cloud.google.com would keep the list uniform.

One question

Does enabling GKE Application-layer Secrets Encryption on Private Managed Cloud count as new user-facing functionality? CLAUDE.md asks for a release note alongside new features or new support, and to ask rather than guess. If this permission documents newly available support, it likely needs a release note in release-notes/; if the feature was already available and this just fills a gap in the prerequisites, no release note is needed.

Checked and fine

  • Frontmatter: untouched by this PR.
  • Heading structure, sentence-ending colon before the list, and backticked role name all match the surrounding entries.
  • Role name capitalization (Roles/...) is consistent with the rest of the list.

@Andy-Hay

Copy link
Copy Markdown
Contributor Author

Claude (@claude) followed your advice and adjusted sentence, please re-review

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Docs style review

Small, focused change — the placement alongside the existing conditional role lists reads well. A few suggestions:

1. Capitalization of the feature name (line 41)

"GKE Application-layer Secrets Encryption feature" mixes title and sentence case. Google’s own docs call this "Application-layer secrets encryption", and the style guide favors sentence case for anything that is not a proper noun:

-The following role is also required if you enable the GKE Application-layer Secrets Encryption feature:
+The following role is also required if you enable GKE application-layer secrets encryption:

Dropping "feature" also tightens the sentence.

2. Use the same Google docs domain as the rest of the page (line 43)

The link points at docs.cloud.google.com, which appears only twice in the whole docs site; the other 100+ Google links (including every other role in this list) use cloud.google.com. Both resolve, but consistency helps:

-- [`Roles/cloudkms.admin`](https://docs.cloud.google.com/iam/docs/roles-permissions/cloudkms#cloudkms.admin)
+- [`Roles/cloudkms.admin`](https://cloud.google.com/iam/docs/roles-permissions/cloudkms#cloudkms.admin)

(The same applies to Roles/datastore.owner on line 34, if you want to fix it while you are here.)

3. Link out to what the feature is

The two lists above this one both give the reader somewhere to go — the RDB Loader list links to the loader docs. "Application-layer secrets encryption" is not mentioned anywhere else in the docs, so a reader who has not heard of it cannot tell whether this role applies to them. Consider linking the feature name to Google's GKE documentation, and, if enabling it is something a customer requests from Snowplow rather than toggles themselves, saying so in a half-sentence.

4. Release note?

If this permission requirement is new for customers (rather than documenting something that has always been true), CLAUDE.md asks for an accompanying release note. If it is just filling a documentation gap, no action needed — flagging it rather than guessing.

@Andy-Hay

Copy link
Copy Markdown
Contributor Author

Claude (@claude) can you review again

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Docs style review

Thanks — the link now uses cloud.google.com, matching the rest of the list. One thing got dropped in the rewording, plus two open items from earlier.

1. Missing article (line 41)

The following role is also required if you enable GKE application-layer secrets encryption feature:

Lowercasing the feature name is right, but "enable GKE application-layer secrets encryption feature" needs an article before "feature". Either add "the", or drop "feature" entirely — the shorter version reads better and matches the terser sentence three lines above:

-The following role is also required if you enable GKE application-layer secrets encryption feature:
+The following role is also required if you enable GKE application-layer secrets encryption:

2. Still no link for the feature itself (line 41)

Both lists above this one give the reader somewhere to go — the RDB Loader list links to the loader docs. Application-layer secrets encryption is not mentioned anywhere else on the docs site, so a reader who has not met the term cannot tell whether this role applies to them. Linking the feature name to Google's GKE documentation would close that gap, and if enabling it is something a customer requests from Snowplow rather than toggles themselves, a half-sentence saying so would help too.

3. Release note — still an open question

Flagging rather than guessing, per CLAUDE.md: if this permission requirement is new for customers, it needs a release note in release-notes/. If it has always been true and this just fills a documentation gap, no action needed.

@Andy-Hay

Copy link
Copy Markdown
Contributor Author

Claude (@claude) please re-review

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Reviewed against the docs style guide. The addition reads well and matches the surrounding structure — the switch to "if you enable" is a nice improvement over the "if using" phrasing in the paragraph above it. Three minor points:

1. Use cloud.google.com, not docs.cloud.google.com

The first link points at docs.cloud.google.com. Across the docs repo that host appears twice versus 106 uses of cloud.google.com, and every other link in this page except one (datastore.owner) uses the shorter form. Suggest:

The following role is also required if you enable [GKE application-layer secrets encryption](https://cloud.google.com/kubernetes-engine/docs/how-to/encrypting-secrets):

2. Expand GKE on first use

"GKE" does not appear anywhere else on this page, so this is its first and only use. Consider spelling it out:

The following role is also required if you enable [application-layer secrets encryption](https://cloud.google.com/kubernetes-engine/docs/how-to/encrypting-secrets) on Google Kubernetes Engine (GKE):

3. Does this need a release note?

CLAUDE.md asks for a release note when a docs change documents new functionality or a behavior change, and to ask rather than guess when it is borderline. A new permission customers must grant before a feature can be enabled feels user-facing, but if application-layer secrets encryption already shipped and this is just documenting a previously undocumented prerequisite, it is editorial and no note is needed. Could you confirm which it is?

🤖 Generated with Claude Code

@Andy-Hay

Copy link
Copy Markdown
Contributor Author

Claude (@claude) please re-review. This isn't a release note feature.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Docs style review

Small, well-placed addition — it follows the existing pattern of the page (intro sentence, then a role list), uses "you", and the link text matches the target page name. A few suggestions:

1. Link domain consistency (docs/get-started/private-managed-cloud/setup-guide-gcp/index.md:41)

The new link points at docs.cloud.google.com, but almost every other Google link on this page uses cloud.google.com. Only one other line (Roles/datastore.owner) uses the docs. variant. Suggest matching the majority convention:

-The following role is also required if you enable [application-layer secrets encryption](https://docs.cloud.google.com/kubernetes-engine/docs/how-to/encrypting-secrets) on Google Kubernetes Engine (GKE):
+The following role is also required if you enable [application-layer secrets encryption](https://cloud.google.com/kubernetes-engine/docs/how-to/encrypting-secrets) on Google Kubernetes Engine:

2. Unused abbreviation (same line)

(GKE) is defined but never used again on the page, so the expansion is not earning its place. Dropping it is covered in the diff above.

3. Clarify who enables the feature, and when (same line)

For a Private Managed Cloud setup guide, "if you enable" is slightly ambiguous, since Snowplow installs and maintains the cluster. Consider making it explicit whether this is something the reader turns on themselves, or something they request as part of pipeline setup, for example:

The following role is also required if your pipeline is deployed with application-layer secrets encryption enabled on Google Kubernetes Engine:

4. Optional, pre-existing: the role identifiers on this page are written as Roles/..., but GCP's actual identifiers are lowercase (roles/cloudkms.admin). This is not introduced by the PR, and the new line is consistent with its neighbors — just flagging in case you want to normalize the list while you are here.

5. Release note?

CLAUDE.md asks for a release note when a change documents new behavior or a new requirement, rather than pure editorial work. This adds a permission customers must grant for a deployment option, which feels borderline — worth confirming whether you want a release note in this PR or a follow-up.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Sep 15, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
documentation 80cbef2 Commit Preview URL

Branch Preview URL
Sep 16 2026, 03:57 PM

@stdfalse Egor Abramov (stdfalse) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Andy-Hay
Andy Hay (Andy-Hay) merged commit 318fa3e into main Sep 16, 2026
8 checks passed
@Andy-Hay
Andy Hay (Andy-Hay) deleted the add-gcp-permission-for-secrets-encryption branch September 16, 2026 17:16
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.

2 participants