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

feat: supply system log to trusted plugins via FD #5391

Merged
merged 12 commits into from
Nov 20, 2023
5 changes: 5 additions & 0 deletions packages/build/src/core/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ const tExecBuild = async function ({
logs,
debug,
systemLog,
systemLogFile,
verbose,
timers: timersA,
sendStatus,
Expand Down Expand Up @@ -264,6 +265,7 @@ export const runAndReportBuild = async function ({
logs,
debug,
systemLog,
systemLogFile,
verbose,
timers,
sendStatus,
Expand Down Expand Up @@ -315,6 +317,7 @@ export const runAndReportBuild = async function ({
logs,
debug,
systemLog,
systemLogFile,
verbose,
timers,
sendStatus,
Expand Down Expand Up @@ -418,6 +421,7 @@ const initAndRunBuild = async function ({
logs,
debug,
systemLog,
systemLogFile,
verbose,
sendStatus,
saveConfig,
Expand Down Expand Up @@ -466,6 +470,7 @@ const initAndRunBuild = async function ({
timers: timersA,
featureFlags,
quiet,
systemLogFile,
})

try {
Expand Down
13 changes: 12 additions & 1 deletion packages/build/src/plugins/child/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
import { logPluginMethodStart, logPluginMethodEnd } from '../../log/messages/ipc.js'

import { cloneNetlifyConfig, getConfigMutations } from './diff.js'
import { getSystemLog } from './systemLog.js'
import { getUtils } from './utils.js'

// Run a specific plugin event handler
Expand All @@ -11,9 +12,19 @@
) {
const method = methods[event]
const runState = {}
const systemLog = getSystemLog()
const utils = getUtils({ event, constants, runState })
const netlifyConfigCopy = cloneNetlifyConfig(netlifyConfig)
const runOptions = { utils, constants, inputs, netlifyConfig: netlifyConfigCopy, packageJson, error, featureFlags }
const runOptions = {
utils,
constants,
inputs,
netlifyConfig: netlifyConfigCopy,
packageJson,
error,
featureFlags,
systemLog,
}

Check warning on line 27 in packages/build/src/plugins/child/run.js

View check run for this annotation

Codecov / codecov/patch

packages/build/src/plugins/child/run.js#L26-L27

Added lines #L26 - L27 were not covered by tests

const envBefore = setEnvChanges(envChanges)

Expand Down
15 changes: 15 additions & 0 deletions packages/build/src/plugins/child/systemLog.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { appendFileSync, openSync } from 'fs'

const systemLogLocation = '/dev/fd/4'

export const getSystemLog = () => {
try {
// throws if system log wasn't hooked up
const fd = openSync(systemLogLocation, 'a')

Check warning on line 8 in packages/build/src/plugins/child/systemLog.js

View check run for this annotation

Codecov / codecov/patch

packages/build/src/plugins/child/systemLog.js#L8

Added line #L8 was not covered by tests
return (message) => {
appendFileSync(fd, `${message}\n`)
}
} catch {
return
}
}
11 changes: 8 additions & 3 deletions packages/build/src/plugins/spawn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
logIncompatiblePlugins,
logLoadingIntegration,
} from '../log/messages/compatibility.js'
import { isTrustedPlugin } from '../steps/plugin.js'
import { measureDuration } from '../time/main.js'

import { getEventFromChild } from './ipc.js'
Expand All @@ -23,7 +24,7 @@ const CHILD_MAIN_FILE = fileURLToPath(new URL('child/main.js', import.meta.url))
// (for both security and safety reasons)
// - logs can be buffered which allows manipulating them for log shipping,
// transforming and parallel plugins
const tStartPlugins = async function ({ pluginsOptions, buildDir, childEnv, logs, debug, quiet }) {
const tStartPlugins = async function ({ pluginsOptions, buildDir, childEnv, logs, debug, quiet, systemLogFile }) {
if (!quiet) {
logRuntime(logs, pluginsOptions)
logLoadingPlugins(logs, pluginsOptions, debug)
Expand All @@ -34,14 +35,16 @@ const tStartPlugins = async function ({ pluginsOptions, buildDir, childEnv, logs
logIncompatiblePlugins(logs, pluginsOptions)

const childProcesses = await Promise.all(
pluginsOptions.map(({ pluginDir, nodePath }) => startPlugin({ pluginDir, nodePath, buildDir, childEnv })),
pluginsOptions.map(({ pluginDir, nodePath, pluginPackageJson }) =>
startPlugin({ pluginDir, nodePath, buildDir, childEnv, systemLogFile, pluginPackageJson }),
),
)
return { childProcesses }
}

export const startPlugins = measureDuration(tStartPlugins, 'start_plugins')

const startPlugin = async function ({ pluginDir, nodePath, buildDir, childEnv }) {
const startPlugin = async function ({ pluginDir, nodePath, buildDir, childEnv, systemLogFile, pluginPackageJson }) {
const childProcess = execaNode(CHILD_MAIN_FILE, [], {
cwd: buildDir,
preferLocal: true,
Expand All @@ -50,6 +53,8 @@ const startPlugin = async function ({ pluginDir, nodePath, buildDir, childEnv })
execPath: nodePath,
env: childEnv,
extendEnv: false,
stdio:
isTrustedPlugin(pluginPackageJson) && systemLogFile ? ['pipe', 'pipe', 'pipe', 'ipc', systemLogFile] : undefined,
})

try {
Expand Down
6 changes: 3 additions & 3 deletions packages/build/src/steps/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { getSuccessStatus } from '../status/success.js'
import { getPluginErrorType } from './error.js'
import { updateNetlifyConfig, listConfigSideFiles } from './update_config.js'

export const isTrustedPlugin = (pluginPackageJson) => pluginPackageJson?.name?.startsWith('@netlify/')

// Fire a plugin step
export const firePluginStep = async function ({
event,
Expand All @@ -33,8 +35,6 @@ export const firePluginStep = async function ({
}) {
const listeners = pipePluginOutput(childProcess, logs)

const isTrustedPlugin = pluginPackageJson?.name?.startsWith('@netlify/')

try {
const configSideFiles = await listConfigSideFiles([headersPath, redirectsPath])
const {
Expand All @@ -48,7 +48,7 @@ export const firePluginStep = async function ({
event,
error,
envChanges,
featureFlags: isTrustedPlugin ? featureFlags : undefined,
featureFlags: isTrustedPlugin(pluginPackageJson) ? featureFlags : undefined,
netlifyConfig,
constants,
},
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export const onBuild = function ({ featureFlags }) {
export const onBuild = function ({ featureFlags, systemLog }) {
systemLog?.("some system-facing logs")
Copy link
Member

Choose a reason for hiding this comment

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

We should add this to the types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 6fa00d8

console.log(JSON.stringify(featureFlags))
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,6 @@
export const onBuild = function ({ featureFlags }) {
export const onBuild = function ({ featureFlags, systemLog }) {
if (systemLog) {
throw new Error("System log shouldn't be acessible from untrusted plugins")
}
console.log("typeof featureflags:", typeof featureFlags)
}
18 changes: 17 additions & 1 deletion packages/build/tests/plugins/tests.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import * as fs from 'fs/promises'
import { platform } from 'process'
import { fileURLToPath } from 'url'

import { Fixture, normalizeOutput, removeDir } from '@netlify/testing'
import test from 'ava'
import { tmpName } from 'tmp-promise'

const FIXTURES_DIR = fileURLToPath(new URL('fixtures', import.meta.url))

Expand Down Expand Up @@ -142,13 +145,24 @@ test('Plugins can have inputs', async (t) => {
t.snapshot(normalizeOutput(output))
})

test('Plugins are passed featureflags', async (t) => {
test('Trusted plugins are passed featureflags and system log', async (t) => {
const systemLogFile = await tmpName()
const output = await new Fixture('./fixtures/feature_flags')
.withFlags({
featureFlags: { test_flag: true },
debug: false,
systemLogFile: await fs.open(systemLogFile, 'a'),
})
.runWithBuild()

if (platform !== 'win32') {
Skn0tt marked this conversation as resolved.
Show resolved Hide resolved
const systemLog = (await fs.readFile(systemLogFile, { encoding: 'utf8' })).split('\n')

const expectedSystemLogs = 'some system-facing logs'
t.false(output.includes(expectedSystemLogs))
t.true(systemLog.includes(expectedSystemLogs))
}

t.true(
output.includes(
JSON.stringify({
Expand All @@ -165,6 +179,8 @@ test('Plugins are passed featureflags', async (t) => {
const outputUntrusted = await new Fixture('./fixtures/feature_flags_untrusted')
.withFlags({
featureFlags: { test_flag: true },
debug: false,
systemLogFile: await fs.open(systemLogFile, 'a'),
})
.runWithBuild()

Expand Down
1 change: 1 addition & 0 deletions packages/build/types/netlify_plugin_options.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,4 +21,5 @@ export interface NetlifyPluginOptions<TInputs extends PluginInputs<StringKeys<TI
constants: NetlifyPluginConstants
utils: NetlifyPluginUtils
featureFlags?: Record<string, unknown>
systemLog?(message: string): void
}
Loading