Skip to content

feat: implement deployment planning and fabricator execution (PR 5)#10736

Open
inlined wants to merge 1 commit into
requires-role-4-promptsfrom
requires-role-5-orchestration
Open

feat: implement deployment planning and fabricator execution (PR 5)#10736
inlined wants to merge 1 commit into
requires-role-4-promptsfrom
requires-role-5-orchestration

Conversation

@inlined

@inlined inlined commented Jun 29, 2026

Copy link
Copy Markdown
Member

Description

This is PR 5 in the requiresRole PR chain.
It implements the orchestration logic:

  1. planner.ts to calculate changesets (roles to add/remove, service account creation/deletion) and check for filtered deployments.
  2. fabricator.ts to execute grantNewRoles (pre-upsert) and removeOldRoles (post-delete).
  3. release/index.ts to hook up the planner and fabricator.

Scenarios Tested

  • Run unit tests: npx mocha src/deploy/functions/release/planner.spec.ts src/deploy/functions/release/fabricator.spec.ts
  • Run npm test
  • Run npm run build & npm run lint

Sample Commands

N/A

@inlined inlined requested review from Berlioz, ajperel and wandamora June 29, 2026 15:55

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces declarative security support and automated rolling IAM role grants/revocations for 2nd gen Cloud Functions by restructuring the deployment planning and execution phases. The feedback recommends several key improvements: executing role removals sequentially to prevent concurrent IAM policy conflicts, adding defensive checks to ensure the managed service account is present before modifying roles, wrapping service account deletion in a try-catch block for robust error handling, and fixing a logic bug in the planner where an empty required roles array prevents service account deletion.

Comment on lines +251 to +254
const roleRemovalPromises = Object.entries(plan).map(([codebase, codebasePlan]) =>
this.removeOldRoles(codebasePlan, codebase),
);
await Promise.all(roleRemovalPromises);

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.

high

Running removeOldRoles concurrently using Promise.all can lead to concurrent IAM policy update conflicts (409 Conflict / etag mismatch) on GCP, especially when deploying multiple codebases. To prevent this, we should execute these updates sequentially using a for...of loop, similar to how grantNewRoles is executed.

Suggested change
const roleRemovalPromises = Object.entries(plan).map(([codebase, codebasePlan]) =>
this.removeOldRoles(codebasePlan, codebase),
);
await Promise.all(roleRemovalPromises);
for (const [codebase, codebasePlan] of Object.entries(plan)) {
await this.removeOldRoles(codebasePlan, codebase);
}

Comment on lines +119 to +126
if (plan.rolesToAdd?.length) {
utils.logLabeledBullet("functions", `Granting IAM roles to ${plan.managedServiceAccount}...`);
try {
await resourcemanager.addServiceAccountRoles(
this.projectId,
plan.managedServiceAccount!,
plan.rolesToAdd,
);

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.

medium

If plan.rolesToAdd is populated but plan.managedServiceAccount is undefined, calling resourcemanager.addServiceAccountRoles will fail at runtime. We should add a defensive check to throw a FirebaseError with a non-zero exit code if the managed service account is missing, as per the repository style guide (lines 30-31).

    if (plan.rolesToAdd?.length) {
      if (!plan.managedServiceAccount) {
        throw new FirebaseError(
          "Failed to grant IAM roles: managed service account is missing.",
          { exit: 1 }
        );
      }
      utils.logLabeledBullet("functions", "Granting IAM roles to " + plan.managedServiceAccount + "...");
      try {
        await resourcemanager.addServiceAccountRoles(
          this.projectId,
          plan.managedServiceAccount,
          plan.rolesToAdd,
        );
References
  1. Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)

Comment on lines +156 to +168
if (!plan.rolesToRemove?.length) {
return;
}
utils.logLabeledBullet(
"functions",
`Revoking unneeded IAM roles from ${plan.managedServiceAccount} for codebase ${codebase}...`,
);
try {
await resourcemanager.removeServiceAccountRoles(
this.projectId,
plan.managedServiceAccount!,
plan.rolesToRemove,
);

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.

medium

Similar to the role addition phase, if plan.rolesToRemove is populated but plan.managedServiceAccount is undefined, calling resourcemanager.removeServiceAccountRoles will fail at runtime. We should add a defensive check to throw a FirebaseError with a non-zero exit code if the managed service account is missing, as per the repository style guide (lines 30-31).

    if (!plan.rolesToRemove?.length) {
      return;
    }
    if (!plan.managedServiceAccount) {
      throw new FirebaseError(
        "Failed to revoke IAM roles: managed service account is missing.",
        { exit: 1 }
      );
    }
    utils.logLabeledBullet(
      "functions",
      "Revoking unneeded IAM roles from " + plan.managedServiceAccount + " for codebase " + codebase + "...",
    );
    try {
      await resourcemanager.removeServiceAccountRoles(
        this.projectId,
        plan.managedServiceAccount,
        plan.rolesToRemove,
      );
References
  1. Throw FirebaseError for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)

Comment on lines +148 to +155
if (plan.serviceAccountToDelete) {
utils.logLabeledBullet(
"functions",
`Deleting managed service account ${plan.serviceAccountToDelete}...`,
);
await iam.deleteServiceAccount(this.projectId, plan.serviceAccountToDelete);
return;
}

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.

medium

For consistency and better user experience, we should wrap iam.deleteServiceAccount in a try-catch block and throw a helpful FirebaseError if the deletion fails, similar to how creation is handled in grantNewRoles.

Suggested change
if (plan.serviceAccountToDelete) {
utils.logLabeledBullet(
"functions",
`Deleting managed service account ${plan.serviceAccountToDelete}...`,
);
await iam.deleteServiceAccount(this.projectId, plan.serviceAccountToDelete);
return;
}
if (plan.serviceAccountToDelete) {
utils.logLabeledBullet(
"functions",
"Deleting managed service account " + plan.serviceAccountToDelete + "...",
);
try {
await iam.deleteServiceAccount(this.projectId, plan.serviceAccountToDelete);
} catch (e) {
throw new FirebaseError(
"Failed to delete managed service account " + plan.serviceAccountToDelete + ". Please ask an IAM administrator to delete it manually.",
{ original: e as Error },
);
}
return;
}

Comment thread src/deploy/functions/release/planner.ts Outdated
Comment on lines +169 to +177
if (requiredRoles) {
rolesToAdd = requiredRoles.filter((r) => !roles.includes(r));
rolesToRemove = roles.filter((r) => !requiredRoles.includes(r));
if (!existingManagedSA && managedSA) {
serviceAccountToCreate = managedSA;
}
} else if (existingManagedSA && !isFiltered) {
serviceAccountToDelete = existingManagedSA;
}

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.

medium

If requiredRoles is defined but empty (e.g., []), checking if (requiredRoles) will evaluate to true. This prevents the planner from falling into the else if block to delete the service account when declarative security is no longer needed. We should check if (requiredRoles && requiredRoles.length > 0) instead to ensure the service account is correctly deleted when there are no required roles.

Suggested change
if (requiredRoles) {
rolesToAdd = requiredRoles.filter((r) => !roles.includes(r));
rolesToRemove = roles.filter((r) => !requiredRoles.includes(r));
if (!existingManagedSA && managedSA) {
serviceAccountToCreate = managedSA;
}
} else if (existingManagedSA && !isFiltered) {
serviceAccountToDelete = existingManagedSA;
}
if (requiredRoles && requiredRoles.length > 0) {
rolesToAdd = requiredRoles.filter((r) => !roles.includes(r));
rolesToRemove = roles.filter((r) => !requiredRoles.includes(r));
if (!existingManagedSA && managedSA) {
serviceAccountToCreate = managedSA;
}
} else if (existingManagedSA && !isFiltered) {
serviceAccountToDelete = existingManagedSA;
}

@inlined inlined force-pushed the requires-role-4-prompts branch from 31fc515 to 7d9cc2f Compare June 29, 2026 16:50
@inlined inlined force-pushed the requires-role-5-orchestration branch from 1dc7096 to ad4f818 Compare June 29, 2026 16:50
@inlined inlined force-pushed the requires-role-4-prompts branch from 7d9cc2f to ae9eaaa Compare June 29, 2026 17:33
### Description
This is PR 5 in the requiresRole PR chain.
It implements the orchestration logic:
1. planner.ts to calculate changesets (roles to add/remove, service account creation/deletion) and check for filtered deployments.
2. fabricator.ts to execute grantNewRoles (pre-upsert) and removeOldRoles (post-delete).
3. release/index.ts to hook up the planner and fabricator.

### Scenarios Tested
- Run unit tests: npx mocha src/deploy/functions/release/planner.spec.ts src/deploy/functions/release/fabricator.spec.ts
- Run npm test
- Run npm run build & npm run lint

### Sample Commands
N/A
@inlined inlined force-pushed the requires-role-5-orchestration branch from ad4f818 to f440407 Compare June 29, 2026 17:34
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