Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "could not assert Secret Manager permissions" Cloud Build error #6904

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/init/features/apphosting/repo.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
const APPHOSTING_CONN_PATTERN = /.+\/apphosting-github-conn-.+$/;
const APPHOSTING_OAUTH_CONN_NAME = "apphosting-github-oauth";
const CONNECTION_NAME_REGEX =
/^projects\/(?<projectId>[^\/]+)\/locations\/(?<location>[^\/]+)\/connections\/(?<id>[^\/]+)$/;

Check warning on line 24 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unnecessary escape character: \/

Check warning on line 24 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unnecessary escape character: \/

Check warning on line 24 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unnecessary escape character: \/

/**
* Exported for unit testing.
Expand Down Expand Up @@ -96,7 +96,6 @@
const existingConns = await listAppHostingConnections(projectId);

if (existingConns.length === 0) {
await ensureSecretManagerAdminGrant(projectId);
existingConns.push(
await createFullyInstalledConnection(projectId, location, generateConnectionId(), oauthConn),
);
Expand All @@ -122,7 +121,7 @@
} while (repoRemoteUri === ADD_CONN_CHOICE);

// Ensure that the selected connection exists in the same region as the backend
const { id: connectionId } = parseConnectionName(connection.name)!;

Check warning on line 124 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
await getOrCreateConnection(projectId, location, connectionId, {
authorizerCredential: connection.githubConfig?.authorizerCredential,
appInstallationId: connection.githubConfig?.appInstallationId,
Expand Down Expand Up @@ -170,11 +169,24 @@
* Gets or creates the sentinel GitHub connection resource that contains our Firebase-wide GitHub Oauth token.
* This Oauth token can be used to create other connections without reprompting the user to grant access.
*/
async function getOrCreateOauthConnection(
export async function getOrCreateOauthConnection(
projectId: string,
location: string,
): Promise<gcb.Connection> {
let conn = await getOrCreateConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix! I wonder if we can get rid of getOrCreateConnection now by inlining it in the other callsites (which I believe should just be linkGitHubRepository())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya we can but I do like that it helps with readability.

let conn: gcb.Connection;
try {
conn = await gcb.getConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
} catch (err: unknown) {
if ((err as any).status === 404) {

Check warning on line 180 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value

Check warning on line 180 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
// Cloud build P4SA requires the secret manager admin role.
// This is required when creating an initial connection which is the Oauth connection in our case.
await ensureSecretManagerAdminGrant(projectId);
conn = await createConnection(projectId, location, APPHOSTING_OAUTH_CONN_NAME);
} else {
throw err;
}
}

while (conn.installationState.stage === "PENDING_USER_OAUTH") {
utils.logBullet("You must authorize the Cloud Build GitHub app.");
utils.logBullet("Sign in to GitHub and authorize Cloud Build GitHub app:");
Expand All @@ -188,7 +200,7 @@
message: "Press Enter once you have authorized the app",
});
cleanup();
const { projectId, location, id } = parseConnectionName(conn.name)!;

Check warning on line 203 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Forbidden non-null assertion
conn = await gcb.getConnection(projectId, location, id);
}
return conn;
Expand All @@ -203,7 +215,7 @@
type: "autocomplete",
name: "remoteUri",
message: "Which of the following repositories would you like to deploy?",
source: (_: any, input = ""): Promise<(inquirer.DistinctChoice | inquirer.Separator)[]> => {

Check warning on line 218 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
return new Promise((resolve) =>
resolve([
new inquirer.Separator(),
Expand Down Expand Up @@ -296,7 +308,7 @@
try {
conn = await gcb.getConnection(projectId, location, connectionId);
} catch (err: unknown) {
if ((err as any).status === 404) {

Check warning on line 311 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .status on an `any` value

Check warning on line 311 in src/init/features/apphosting/repo.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
conn = await createConnection(projectId, location, connectionId, githubConfig);
} else {
throw err;
Expand Down
30 changes: 30 additions & 0 deletions src/test/init/apphosting/repo.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@ import * as sinon from "sinon";
import { expect } from "chai";

import * as gcb from "../../../gcp/cloudbuild";
import * as rm from "../../../gcp/resourceManager";
import * as prompt from "../../../prompt";
import * as poller from "../../../operation-poller";
import * as repo from "../../../init/features/apphosting/repo";
import * as utils from "../../../utils";
import * as srcUtils from "../../../../src/getProjectNumber";
import { FirebaseError } from "../../../error";

const projectId = "projectId";
Expand Down Expand Up @@ -53,8 +55,11 @@ describe("composer", () => {
let getConnectionStub: sinon.SinonStub;
let getRepositoryStub: sinon.SinonStub;
let createConnectionStub: sinon.SinonStub;
let serviceAccountHasRolesStub: sinon.SinonStub;
let createRepositoryStub: sinon.SinonStub;
let fetchLinkableRepositoriesStub: sinon.SinonStub;
let getProjectNumberStub: sinon.SinonStub;
let openInBrowserPopupStub: sinon.SinonStub;

beforeEach(() => {
promptOnceStub = sandbox.stub(prompt, "promptOnce").throws("Unexpected promptOnce call");
Expand All @@ -70,13 +75,20 @@ describe("composer", () => {
createConnectionStub = sandbox
.stub(gcb, "createConnection")
.throws("Unexpected createConnection call");
serviceAccountHasRolesStub = sandbox.stub(rm, "serviceAccountHasRoles").resolves(true);
createRepositoryStub = sandbox
.stub(gcb, "createRepository")
.throws("Unexpected createRepository call");
fetchLinkableRepositoriesStub = sandbox
.stub(gcb, "fetchLinkableRepositories")
.throws("Unexpected fetchLinkableRepositories call");
sandbox.stub(utils, "openInBrowser").resolves();
openInBrowserPopupStub = sandbox
.stub(utils, "openInBrowserPopup")
.throws("Unexpected openInBrowserPopup call");
getProjectNumberStub = sandbox
.stub(srcUtils, "getProjectNumber")
.throws("Unexpected getProjectNumber call");
});

afterEach(() => {
Expand Down Expand Up @@ -139,6 +151,24 @@ describe("composer", () => {
expect(createConnectionStub).to.be.calledWith(projectId, location, connectionId);
});

it("checks if secret manager admin role is granted for cloud build P4SA when creating an oauth connection", async () => {
getConnectionStub.onFirstCall().rejects(new FirebaseError("error", { status: 404 }));
getConnectionStub.onSecondCall().resolves(completeConn);
createConnectionStub.resolves(op);
pollOperationStub.resolves(pendingConn);
promptOnceStub.resolves("any key");
getProjectNumberStub.onFirstCall().resolves(projectId);
openInBrowserPopupStub.resolves({ url: "", cleanup: sandbox.stub() });

await repo.getOrCreateOauthConnection(projectId, location);
expect(serviceAccountHasRolesStub).to.be.calledWith(
projectId,
`service-${projectId}@gcp-sa-cloudbuild.iam.gserviceaccount.com`,
["roles/secretmanager.admin"],
true,
);
});

it("creates repository if it doesn't exist", async () => {
getConnectionStub.resolves(completeConn);
fetchLinkableRepositoriesStub.resolves(repos);
Expand Down
Loading