Add GroupByEncryptedKey transform #36213

damccorm · 2025-09-19T16:17:31Z

Add a transform which allows users to use a secret manager to ensure that any GBK data is encrypted before being sent to a runner. This allows users to avoid unencrypted data at rest when using GBK.

Part of #36214

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

…mccorm/gbek

damccorm · 2025-09-19T19:53:08Z

There are some failing tests, but they all seem unrelated and the core gbek tests are succeeding.

github-actions · 2025-09-19T23:06:35Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

damccorm · 2025-09-22T15:37:28Z

assign set of reviewers

github-actions · 2025-09-22T15:38:44Z

Assigning reviewers:

R: @claudevdm for label python.

Note: If you would like to opt out of this review, comment assign to next reviewer.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

claudevdm · 2025-09-22T19:06:12Z

sdks/python/apache_beam/transforms/util_test.py


 try:
  import dill
+  from google.cloud import secretmanager


Should we move this out of the dill try/except block? This import will always fail if dill is not installed.

Also related, in which test suite do you expect the tests that depend on secretmanager to run? I see they are skipped in PreCommit Python Coverage apache_beam/transforms/util_test.py:354: GCP dependencies are not installed

Thanks, updated. It should run in I think, or at least in the postcommit. I'm not 100% sure where all we have GCP deps installed. I'll verify it runs correctly this time, though, thanks for flagging it.

claudevdm · 2025-09-22T19:15:05Z

sdks/python/apache_beam/transforms/util.py

+        raise ValueError(
+            'Input elements to the transform %s with stateful DoFn must be '
+            'key-value pairs.' % self)
+      key_coder = coder.key_coder()


Should we assert that the key coder is deterministic here/ call self.key_coder.as_deterministic_coder here? For the encrypted key to be deterministic, we assume that the actual key coder is deterministic?

Wondering if its possible to create a new coder that wraps the original coder along with doing the encryption?

I think defining this as a coder could be messy because then you would need to access the secret at construction time and include that as part of the serialized graph definition. This would then not provide sufficient security guarantees since the graph itself would have all information needed to decrypt the value.

Potentially you could include the work of downloading the secret in the coder definition, but I don't think we gain much from this today (and errors might be messy to debug). It also seems nice that we have the actual transform definition which helps make it obvious what is happening from the graph.

Yeah, this is a good call. Interestingly, we do check this for the direct runner GBK implementation, but not more broadly as far as I can tell. But we should definitely be verifying here.

Updated

Got it. I think this will still circumvent update compat checks because we are pulling the original coders out of the graph?

Yeah, it will, though at least the pipeline should cleanly fail in most cases when it tries to perform the encoding. For now, I will add a note to the doc string that this should be used with caution to avoid this issue.

In the future, there are maybe ways we could address this:

Instead of baking this into a transform, we could create a custom coder at the SDK level which handles this (reading in the encoded bytes and then applying the encoding on top of that in bundle_processor.py)

We could create a custom type and associated coder per GBEK instance which handles all encoding pieces.

Neither of these is trivial, so for now I will leave this with the doc note

Something like the coder.encode could process a tuple (secret_version_name, unencrypted_key) and, use a output a (secret_version_name, encrypted_key)?

I thought about this as well - it is pretty expensive though since we're serializing the secret name every time.

This is what I was trying to get around with We could create a custom type and associated coder per GBEK instance which handles all encoding pieces. since the coder definition could contain the secret name.

claudevdm · 2025-09-22T19:16:42Z

sdks/python/setup.py

              # GCP Packages required by ML functionality
              'google-cloud-dlp>=3.0.0,<4',
              'google-cloud-language>=2.0,<3',
+              'google-cloud-secret-manager>=2.0,<3',


I know Ashok had some issues with google-cloud-secret-manager dependency when using it as an additional requirement (because of all the google cloud library namespaces), but that might be resolved if the dependency is installed on the containers

I haven't had any issues here - was it just that finding a version that matched requirements was hard?

I think it was that requirements installed it in a different directory (site packages vs dist packages) than all the other gcp deps. This caused "scecretmanager not defined" issue because it was looking in the path of all the other gcp packages (they were in dist packages when secretmanager was installed in site packages or vice versa)

damccorm · 2025-09-25T19:46:24Z

google.api_core.exceptions.PermissionDenied: 403 Permission 'secretmanager.versions.access' denied for resource 'projects/apache-beam-testing/secrets/gbek_secret_tests/versions/latest' (or it may not exist).

Looks like I need to give broader access to the service account, I'll update back once this is passing.

damccorm · 2025-09-25T19:46:41Z

waiting on author

damccorm · 2025-09-26T14:21:52Z

google.api_core.exceptions.PermissionDenied: 403 Permission 'secretmanager.versions.access' denied for resource 'projects/apache-beam-testing/secrets/gbek_secret_tests/versions/latest' (or it may not exist).
Looks like I need to give broader access to the service account, I'll update back once this is passing.

Fixed - tests are now passing

Add GroupByEncryptedKey transform

8e69e93

github-actions bot added the python label Sep 19, 2025

damccorm added 4 commits September 19, 2025 12:28

Missing requirement

4c17662

Merge branch 'master' of https://github.com/apache/beam into users/da…

b2956ad

…mccorm/gbek

lint

0331905

Import order

cefc7c4

damccorm marked this pull request as ready for review September 19, 2025 19:52

damccorm mentioned this pull request Sep 19, 2025

Java GroupByEncryptedKey #36217

Merged

3 tasks

github-actions bot added the Next Action: Reviewers label Sep 22, 2025

keep type checking

fab145a

claudevdm requested changes Sep 22, 2025

View reviewed changes

damccorm added 3 commits September 22, 2025 15:38

feedback

ee14851

comment disclaimer

d43b5a8

doc note

50443d1

github-actions bot added Next Action: Author and removed Next Action: Reviewers labels Sep 25, 2025

Avoid secret naming conflicts

d5080f4

github-actions bot added Next Action: Reviewers and removed Next Action: Author labels Sep 26, 2025

claudevdm approved these changes Sep 26, 2025

View reviewed changes

damccorm merged commit 7381121 into master Sep 26, 2025
113 of 121 checks passed

damccorm deleted the users/damccorm/gbek branch September 26, 2025 16:33

damccorm mentioned this pull request Sep 29, 2025

[Feature Request]: Add GroupByEncryptedKey transform #36214

Closed

25 tasks

Add GroupByEncryptedKey transform #36213

Add GroupByEncryptedKey transform #36213

Uh oh!

Conversation

damccorm commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

GitHub Actions Tests Status (on master branch)

Uh oh!

damccorm commented Sep 19, 2025

Uh oh!

github-actions bot commented Sep 19, 2025

Uh oh!

damccorm commented Sep 22, 2025

Uh oh!

github-actions bot commented Sep 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damccorm commented Sep 25, 2025

Uh oh!

damccorm commented Sep 25, 2025

Uh oh!

damccorm commented Sep 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

damccorm commented Sep 19, 2025 •

edited

Loading