chore: IAM Validation added for fail-safe. #3072

pratickchokhani · 2025-12-08T07:29:58Z

New IAM Validation Utilities: Introduced a set of new utility classes (IAMCheckResult, IAMPermissionsChecker, IAMRequirementsCreator, IAMResourceRequirements) to programmatically check IAM permissions for Google Cloud Platform resources.
Cloud Resource Manager API Integration: Added the google-api-services-cloudresourcemanager dependency to the pom.xml to enable interaction with the Cloud Resource Manager API for permission checks.
Spanner Permission Validation: Integrated a validateRequiredPermissions method into the Import and Export pipeline template to perform pre-execution IAM checks for Spanner resources, ensuring necessary permissions are present for the pipeline to run successfully.

gemini-code-assist · 2025-12-08T07:30:13Z

Summary of Changes

Hello @pratickchokhani, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the robustness and fail-safety of the SourceDbToSpanner Dataflow template by implementing a proactive IAM permission validation mechanism. Before pipeline execution, the system will now verify that the necessary Spanner permissions are granted, preventing potential runtime failures due to insufficient access rights. This involves adding new common utility classes for handling IAM checks and integrating this validation into the template's startup process.

Highlights

New IAM Validation Utilities: Introduced a set of new utility classes (IAMCheckResult, IAMPermissionsChecker, IAMRequirementsCreator, IAMResourceRequirements) to programmatically check IAM permissions for Google Cloud Platform resources.
Cloud Resource Manager API Integration: Added the google-api-services-cloudresourcemanager dependency to the pom.xml to enable interaction with the Cloud Resource Manager API for permission checks.
Spanner Permission Validation: Integrated a validateRequiredPermissions method into the SourceDbToSpanner template to perform pre-execution IAM checks for Spanner resources, ensuring necessary permissions are present for the pipeline to run successfully.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

codecov · 2025-12-08T08:36:55Z

Codecov Report

❌ Patch coverage is 34.69388% with 64 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.44%. Comparing base (4e8bd77) to head (92d29c7).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...ud/teleport/spanner/iam/IAMPermissionsChecker.java	54.54%	20 Missing ⚠️
.../google/cloud/teleport/spanner/ExportPipeline.java	0.00%	17 Missing ⚠️
.../google/cloud/teleport/spanner/ImportPipeline.java	0.00%	17 Missing ⚠️
...d/teleport/spanner/iam/IAMRequirementsCreator.java	0.00%	4 Missing ⚠️
...gle/cloud/teleport/spanner/iam/IAMCheckResult.java	66.66%	3 Missing ⚠️
.../teleport/spanner/iam/IAMResourceRequirements.java	57.14%	2 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (34.69%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3072      +/-   ##
============================================
+ Coverage     50.41%   50.44%   +0.03%     
- Complexity     5022     5051      +29     
============================================
  Files           970      977       +7     
  Lines         59609    59782     +173     
  Branches       6507     6512       +5     
============================================
+ Hits          30050    30156     +106     
- Misses        27436    27497      +61     
- Partials       2123     2129       +6

Components	Coverage Δ
spanner-templates	`70.57% <34.69%> (-0.08%)`	⬇️
spanner-import-export	`68.58% <34.69%> (-0.42%)`	⬇️
spanner-live-forward-migration	`80.00% <ø> (-0.03%)`	⬇️
spanner-live-reverse-replication	`77.77% <ø> (+0.31%)`	⬆️
spanner-bulk-migration	`88.23% <ø> (-0.02%)`	⬇️

Files with missing lines	Coverage Δ
...gle/cloud/teleport/spanner/iam/IAMCheckResult.java	`66.66% <66.66%> (ø)`
.../teleport/spanner/iam/IAMResourceRequirements.java	`57.14% <57.14%> (ø)`
...d/teleport/spanner/iam/IAMRequirementsCreator.java	`0.00% <0.00%> (ø)`
.../google/cloud/teleport/spanner/ExportPipeline.java	`0.00% <0.00%> (ø)`
.../google/cloud/teleport/spanner/ImportPipeline.java	`0.00% <0.00%> (ø)`
...ud/teleport/spanner/iam/IAMPermissionsChecker.java	`54.54% <54.54%> (ø)`

... and 17 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

rohitwali · 2025-12-12T16:07:12Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+
+  private CloudResourceManager createCloudResourceManagerService()
+      throws IOException, GeneralSecurityException {
+    if (resourceManagerForTesting != null) {


do you want to inject it into the class?

We can do that. But that will only help in testing purpose.

My thinking was that if IAMpermissionsChecker is initialized by the user at class level and is never destroyed, then the resource manager will also never be destroyed.

Hence, initialized later.

we are mixing testing code in main code. injecting would ensure decoupling and avoids doing stuff like if test code. just a bit cleaner to read IMO

Have moved this to constructur.

rohitwali · 2025-12-12T16:10:34Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+  private IAMCheckResult check(
+      CloudResourceManager resourceManager, List<String> requiredPermissions) {
+    HashSet<String> grantedPermissions =
+        new HashSet<>(checkPermission(resourceManager, projectIdResource, requiredPermissions));


are we only checking at project level? should we fallback to more granular level as well?
e.g., if user/role has required permissions at say, spanner instance or database level?

The testIam API is not to specific on how to validate permissions at instance or database level.

Few of my experiments failed on that part. We can do a followup on figuring out if granular permission is required.

The testIam API is not to specific on how to validate permissions at instance or database level.

Could you say more about this? The TestIamPermissionsRequest takes resource as an argument, which can be instance or database. are you saying there is no way to specify them?

my concern here is that this may cause permission failures even though user may have required permissions and hence may be confusing to them.

rohitwali · 2025-12-12T16:11:37Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+    return new IAMCheckResult(projectIdResource, missingPermissions);
+  }
+
+  private List<String> checkPermission(


what is the difference between this and previous "check" method? their signature looks similar

This require resource name. This was made from the perspective when we are checking permission at resource level.

Currently, there isn't much difference.

It seems like a helper method. In which case, it might make sense to call the other method from here (or vice versa) to not duplicate the flows.

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

rohitwali · 2025-12-12T16:15:35Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+   */
+  public IAMCheckResult check(List<IAMResourceRequirements> requirements) {
+    try {
+      CloudResourceManager resourceManager = createCloudResourceManagerService();


should this be injected at class level?

This class is designed for the check method to be called only once. This reduces IAM validation call.

Caller is suppose to aggregate all the permissions and call the Check method.

Maybe add this in the docs to make it clear that it is an expensive call it should not be called frequently.

Though, just double checking if there is any harm in making it a class level variable as suggested ?

Have moved this to constructor.

rohitwali · 2025-12-12T16:17:23Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMRequirementsCreator.java

+import java.util.List;
+
+public class IAMRequirementsCreator {
+  private static final List<String> SPANNER_PERMISSIONS =


curious: why do we have a single list? shouldnt the list be use case specific?
for example, I see same list for both import and export. shouldnt their permission list be different?

should this be a list that caller (user of the module) passes into this module?

The goal of this class is to have separate lists. For the export pipeline, we can have a separate list within this class.

The thought process is that, we do not need too many distinct lists and there will be duplicates.

For export, I can create a instances list and databases create or drop. But rest should be there.

In that case call this the DEFAULT_SPANNER_PERMISSIONS or so. Again with comments to cover the thought process.

rohitwali · 2025-12-12T16:20:23Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMResourceRequirements.java

+import java.util.List;
+
+/** Represents the IAM permissions required on a specific GCP resource. */
+public class IAMResourceRequirements {


this class seems like a holder for an arraylist.
how are we thinking of growing/extending this class, if in future?

This will grow when we will be checking permissions per resource.

+1 to Rohit's comment here. Please remove if not needed or add docs on where it might be needed.

Have added the comment.

rohitwali · 2025-12-12T16:26:44Z

v1/src/main/java/com/google/cloud/teleport/spanner/ExportPipeline.java

    }
  }
+
+  private static void validateRequiredPermissions(ExportPipelineOptions options) {


i see this code is duplicated across pipelines. would it make sense to push it down as an API in the IAM checker module?

It needs to be per-Template. For Import and export the code is same. But this will change when we shift to other templates.

could you pl help be understand better with an example, on how it might deviate? and may be leave a comment for future code readers

bharadwaj-aditya · 2025-12-13T12:41:36Z

v1/src/main/java/com/google/cloud/teleport/spanner/ExportPipeline.java

+        new IAMPermissionsChecker(gcpOptions.getProject(), gcpOptions);
+    IAMCheckResult missingPermission =
+        iamPermissionsChecker.check(Collections.singletonList(spannerRequirements));
+    if (missingPermission.isSuccess()) {


the naming is a little confusing. Does missingPermission.isSuccess mean the permission is there or the check failed or does it mean the check succeeded ?

Try to avoid double negatives where possible.

Based on the code reading - i would suggest renaming this to permissionCheck or something rather than missing permissions.

bharadwaj-aditya · 2025-12-13T12:45:01Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMCheckResult.java

+    return new ArrayList<>(missingPermissions);
+  }
+
+  public boolean isSuccess() {


nit: rename method to permissionsAvailable or something clearer.
isSuccess could be misunderstood as the check API is successful or so.

bharadwaj-aditya · 2025-12-13T12:46:49Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+   */
+  public IAMCheckResult check(List<IAMResourceRequirements> requirements) {
+    try {
+      CloudResourceManager resourceManager = createCloudResourceManagerService();


Maybe add this in the docs to make it clear that it is an expensive call it should not be called frequently.

Though, just double checking if there is any harm in making it a class level variable as suggested ?

bharadwaj-aditya · 2025-12-13T12:47:17Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+              .collect(Collectors.toList());
+
+      return new IAMCheckResult(projectIdResource, missingPermissions);
+    } catch (IOException | GeneralSecurityException e) {


Add an info logger here with the error message. It is possible that the parent hides this in another error.

bharadwaj-aditya · 2025-12-13T12:49:39Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMPermissionsChecker.java

+    return new IAMCheckResult(projectIdResource, missingPermissions);
+  }
+
+  private List<String> checkPermission(


It seems like a helper method. In which case, it might make sense to call the other method from here (or vice versa) to not duplicate the flows.

bharadwaj-aditya · 2025-12-13T12:51:29Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMRequirementsCreator.java

+import java.util.List;
+
+public class IAMRequirementsCreator {
+  private static final List<String> SPANNER_PERMISSIONS =


In that case call this the DEFAULT_SPANNER_PERMISSIONS or so. Again with comments to cover the thought process.

bharadwaj-aditya · 2025-12-13T12:52:14Z

v1/src/main/java/com/google/cloud/teleport/spanner/iam/IAMResourceRequirements.java

+import java.util.List;
+
+/** Represents the IAM permissions required on a specific GCP resource. */
+public class IAMResourceRequirements {


+1 to Rohit's comment here. Please remove if not needed or add docs on where it might be needed.

manitgupta

Only the IAMPermissiomsChecker seems to have a UT, do we need UTs for the other classes that have been added?
Did you get a resolution on the placement of this lib? There was a discussion on v1 v/s v2 and how that would work out.

pratickchokhani requested a review from a team as a code owner December 8, 2025 07:29

pratickchokhani requested review from bharadwaj-aditya and rohitwali December 8, 2025 07:29

pull-request-size bot added the size/L label Dec 8, 2025

pratickchokhani force-pushed the iam_check branch from 597b5c8 to 8974d7c Compare December 8, 2025 08:27

pratickchokhani force-pushed the iam_check branch from 8974d7c to fab36a4 Compare December 8, 2025 08:48

pratickchokhani marked this pull request as draft December 8, 2025 08:48

pratickchokhani force-pushed the iam_check branch 9 times, most recently from 026faff to a832363 Compare December 11, 2025 10:01

pratickchokhani added the bug-fix label Dec 11, 2025

pratickchokhani force-pushed the iam_check branch 7 times, most recently from 9de79c9 to 5ab4b26 Compare December 12, 2025 09:23

pratickchokhani marked this pull request as ready for review December 12, 2025 10:49

rohitwali reviewed Dec 12, 2025

View reviewed changes

pratickchokhani force-pushed the iam_check branch from 5ab4b26 to 441067b Compare December 13, 2025 05:09

chore: IAM Validation added for fail-safe.

3bc834f

pratickchokhani force-pushed the iam_check branch from 441067b to 3bc834f Compare December 13, 2025 05:45

bharadwaj-aditya reviewed Dec 13, 2025

View reviewed changes

manitgupta reviewed Dec 15, 2025

View reviewed changes

pratickchokhani force-pushed the iam_check branch 2 times, most recently from 5f0a0cb to 42e6235 Compare December 15, 2025 05:35

chore: Fixed constructor argument.

92d29c7

pratickchokhani force-pushed the iam_check branch from 42e6235 to 92d29c7 Compare December 15, 2025 05:49

chore: IAM Validation added for fail-safe. #3072

Are you sure you want to change the base?

chore: IAM Validation added for fail-safe. #3072

Uh oh!

Conversation

pratickchokhani commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot commented Dec 8, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

codecov bot commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rohitwali Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rohitwali Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

pratickchokhani commented Dec 8, 2025 •

edited

Loading

codecov bot commented Dec 8, 2025 •

edited

Loading

rohitwali Dec 13, 2025 •

edited

Loading

rohitwali Dec 13, 2025 •

edited

Loading