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

Re-guard AR behind an experiment #3935

Merged
merged 2 commits into from Dec 7, 2021
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -2,3 +2,4 @@
- Fixes issue where providing the `--project` flag during `init` would not be recognized with a default project already set. (#3870)
- Fixes issue with setting memory limits for some functions (#3924)
- New HTTPS functions only allow secure traffic. (#3923)
- No longer default-enable AR and don't send builds to AR unless an experiment is enabled (#3935)
1 change: 1 addition & 0 deletions src/deploy/functions/args.ts
Expand Up @@ -21,6 +21,7 @@ export interface Context {
functionsSourceV1?: string;
functionsSourceV2?: string;
runtimeConfigEnabled?: boolean;
artifactRegistryEnabled?: boolean;
firebaseConfig?: FirebaseConfig;

// Filled in the "deploy" phase.
Expand Down
11 changes: 11 additions & 0 deletions src/deploy/functions/containerCleaner.ts
Expand Up @@ -170,6 +170,17 @@ export class ArtifactRegistryCleaner {
}
}

// Temporary class to turn off AR cleaning if AR isn't enabled yet
export class NoopArtifactRegistryCleaner extends ArtifactRegistryCleaner {
cleanupFunction(): Promise<void> {
return Promise.resolve();
}

cleanupFunctionCache(): Promise<void> {
return Promise.resolve();
}
}

export class ContainerRegistryCleaner {
readonly helpers: Record<string, DockerHelper> = {};

Expand Down
20 changes: 19 additions & 1 deletion src/deploy/functions/prepare.ts
Expand Up @@ -20,6 +20,7 @@ import * as utils from "../../utils";
import { logger } from "../../logger";
import { ensureTriggerRegions } from "./triggerRegionHelper";
import { ensureServiceAgentRoles } from "./checkIam";
import e from "express";

function hasUserConfig(config: Record<string, unknown>): boolean {
// "firebase" key is always going to exist in runtime config.
Expand All @@ -31,6 +32,22 @@ function hasDotenv(opts: functionsEnv.UserEnvsOpts): boolean {
return previews.dotenv && functionsEnv.hasUserEnvs(opts);
}

// We previously force-enabled AR. We want to wait on this to see if we can give
// an upgrade warning in the future. If it already is enabled though we want to
// remember this and still use the cleaner if necessary.
async function maybeEnableAR(projectId: string): Promise<boolean> {
if (previews.artifactregistry) {
return ensureApiEnabled.check(
projectId,
"artifactregistry.googleapis.com",
"functions",
/* silent= */ true
);
}
await ensureApiEnabled.ensure(projectId, "artifactregistry.googleapis.com", "functions");
return true;
}

export async function prepare(
context: args.Context,
options: Options,
Expand Down Expand Up @@ -58,9 +75,10 @@ export async function prepare(
/* silent=*/ true
),
ensureCloudBuildEnabled(projectId),
ensureApiEnabled.ensure(projectId, "artifactregistry.googleapis.com", "functions"),
maybeEnableAR(projectId),
]);
context.runtimeConfigEnabled = checkAPIsEnabled[1];
context.artifactRegistryEnabled = checkAPIsEnabled[3];

// Get the Firebase Config, and set it on each function in the deployment.
const firebaseConfig = await functionsConfig.getFirebaseConfig(options);
Expand Down
12 changes: 10 additions & 2 deletions src/gcp/cloudfunctions.ts
Expand Up @@ -205,8 +205,12 @@ export async function createFunction(
const endpoint = `/${API_VERSION}/${apiPath}`;

try {
const headers: Record<string, string> = {};
if (previews.artifactregistry) {
headers["X-Firebase-Artifact-Registry"] = "optin";
}
const res = await api.request("POST", endpoint, {
headers: { "X-Firebase-Artifact-Registry": "optin" },
headers,
auth: true,
data: cloudFunction,
origin: api.functionsOrigin,
Expand Down Expand Up @@ -370,8 +374,12 @@ export async function updateFunction(
// Failure policy is always an explicit policy and is only signified by the presence or absence of
// a protobuf.Empty value, so we have to manually add it in the missing case.
try {
const headers: Record<string, string> = {};
if (previews.artifactregistry) {
headers["X-Firebase-Artifact-Registry"] = "optin";
}
const res = await api.request("PATCH", endpoint, {
headers: { "X-Firebase-Artifact-Registry": "optin" },
headers,
qs: {
updateMask: fieldMasks.join(","),
},
Expand Down
4 changes: 3 additions & 1 deletion src/previews.ts
Expand Up @@ -10,6 +10,7 @@ interface PreviewFlags {
golang: boolean;
deletegcfartifacts: boolean;
dotenv: boolean;
artifactregistry: boolean;
}

export const previews: PreviewFlags = {
Expand All @@ -22,8 +23,9 @@ export const previews: PreviewFlags = {
golang: false,
deletegcfartifacts: false,
dotenv: false,
artifactregistry: false,

...configstore.get("previews"),
...(configstore.get("previews") as Partial<PreviewFlags>),
};

if (process.env.FIREBASE_CLI_PREVIEWS) {
Expand Down