From 4015a99bb37ca8ca1b64a48ebbd247d84579027c Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 20 Feb 2025 18:56:39 +0100 Subject: [PATCH 1/2] fix: correct source mapping in varfiles Ensures that template error messages in YAML varfiles will be correct --- core/src/config/base.ts | 70 +++++++++++++------ .../src/config/template-contexts/variables.ts | 36 +++++----- 2 files changed, 65 insertions(+), 41 deletions(-) diff --git a/core/src/config/base.ts b/core/src/config/base.ts index 8930c9b8c7..553fd17067 100644 --- a/core/src/config/base.ts +++ b/core/src/config/base.ts @@ -8,7 +8,6 @@ import dotenv from "dotenv" import { sep, resolve, relative, basename, dirname, join } from "path" -import { load } from "js-yaml" import { lint } from "yaml-lint" import { omit, isPlainObject } from "lodash-es" import type { BuildDependencyConfig, ModuleConfig } from "./module.js" @@ -16,6 +15,7 @@ import { coreModuleSpecSchema, baseModuleSchemaKeys } from "./module.js" import { ConfigurationError, FilesystemError, isErrnoException, ParameterError } from "../exceptions.js" import { DEFAULT_BUILD_TIMEOUT_SEC, defaultGardenApiVersion, GardenApiVersion } from "../constants.js" import type { ProjectConfig } from "../config/project.js" +import type { ConfigSource } from "./validation.js" import { validateWithPath } from "./validation.js" import { defaultDotIgnoreFile, listDirectory } from "../util/fs.js" import { isConfigFilename } from "../util/fs.js" @@ -588,15 +588,8 @@ export async function findProjectConfig({ return } -const _readFile = profileAsync(async function _readFile(path: string) { - return await readFile(path) -}) - -function _loadYaml(data: Buffer) { - return load(data.toString()) as PrimitiveMap -} - -const loadVarfileCache = new LRUCache>({ +type LoadedVarfile = { data: PrimitiveMap; source: ConfigSource } +const loadVarfileCache = new LRUCache>({ max: 10000, ttl: 30000, ttlAutopurge: true, @@ -628,7 +621,7 @@ export const loadVarfile = profileAsync(async function loadVarfile({ } const resolvedPath = resolve(configRoot, pathOrDefault) - let promise: Promise | undefined = loadVarfileCache.get(resolvedPath) + let promise: Promise | undefined = loadVarfileCache.get(resolvedPath) if (!promise) { promise = loadVarfileInner() loadVarfileCache.set(resolvedPath, promise) @@ -636,38 +629,64 @@ export const loadVarfile = profileAsync(async function loadVarfile({ return await promise - async function loadVarfileInner(): Promise { + async function loadVarfileInner(): Promise { try { - const data = await _readFile(resolvedPath) - log?.silly(() => `Loaded ${data.length} bytes from varfile ${resolvedPath}`) + const fileContents = await readFile(resolvedPath) + log?.silly(() => `Loaded ${fileContents.length} bytes from varfile ${resolvedPath}`) const relPath = relative(configRoot, resolvedPath) const filename = basename(resolvedPath.toLowerCase()) if (filename.endsWith(".json")) { // JSON parser throws a JSON syntax error on completely empty input file, // and returns an empty object for an empty JSON. - const parsed = JSON.parse(data.toString()) + const parsed = JSON.parse(fileContents.toString()) if (!isPlainObject(parsed)) { throw new ConfigurationError({ message: `Configured variable file ${relPath} must be a valid plain JSON object. Got: ${typeof parsed}`, }) } - return parsed as PrimitiveMap + return { + data: parsed as PrimitiveMap, + // source mapping to JSON has not been implemented at this point + source: { path: [] }, + } } else if (filename.endsWith(".yml") || filename.endsWith(".yaml")) { + const loaded = await loadAndValidateYaml({ + content: fileContents.toString("utf-8"), + filename: resolvedPath, + version: "1.2", + sourceDescription: `varfile at ${relPath}`, + }) + if (loaded.length !== 1) { + throw new ConfigurationError({ + message: `Configured variable file ${relPath} must be a single YAML document. Got multiple (${loaded.length}) YAML documents`, + }) + } + const yamlDoc = loaded[0] // YAML parser returns `undefined` for empty files, we interpret that as an empty object. - const parsed = _loadYaml(data) || {} - if (!isPlainObject(parsed)) { + const data = yamlDoc.toJS() || {} + if (!isPlainObject(data)) { throw new ConfigurationError({ - message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof parsed}`, + message: `Configured variable file ${relPath} must be a single plain YAML mapping. Got: ${typeof data}`, }) } - return parsed as PrimitiveMap + return { + data, + source: { + path: [], + yamlDoc, + }, + } } else { // Note: For backwards-compatibility we fall back on using .env as a default format, // and don't specifically validate the extension for that. // The dotenv parser returns an empty object for invalid or empty input file. - const parsed = dotenv.parse(data) - return parsed as PrimitiveMap + const parsed = dotenv.parse(fileContents) + return { + data: parsed as PrimitiveMap, + // source mapping to dotenv files has not been implemented at this point + source: { path: [] }, + } } } catch (error) { if (error instanceof ConfigurationError) { @@ -687,7 +706,12 @@ export const loadVarfile = profileAsync(async function loadVarfile({ }) } else { // The default var file did not exist. In that case we return empty object. - return {} + return { + data: {}, + source: { + path: [], + }, + } } } diff --git a/core/src/config/template-contexts/variables.ts b/core/src/config/template-contexts/variables.ts index bf86c4e24b..521d5a2e16 100644 --- a/core/src/config/template-contexts/variables.ts +++ b/core/src/config/template-contexts/variables.ts @@ -131,8 +131,8 @@ export class VariablesContext extends LayeredContext { defaultPath: defaultProjectVarfilePath, }) const projectVarfileVars = parseTemplateCollection({ - value: rawProjectVarfileVars, - source: { path: ["varfile"], yamlDoc: projectConfig.internal.yamlDoc }, + value: rawProjectVarfileVars.data, + source: rawProjectVarfileVars.source, }) return new this(`project ${projectConfig.name}`, { @@ -155,8 +155,8 @@ export class VariablesContext extends LayeredContext { defaultPath: defaultEnvVarfilePath(environment), }) const envVarfileVars = parseTemplateCollection({ - value: rawEnvVarfileVars, - source: { path: ["varfile"], yamlDoc: projectConfig.internal.yamlDoc }, + value: rawEnvVarfileVars.data, + source: rawEnvVarfileVars.source, }) return new this(`environment ${environmentConfig.name}`, { @@ -172,7 +172,7 @@ export class VariablesContext extends LayeredContext { * garden.variableOverrides > module varfile > config.variables */ public static async forModule(garden: Garden, config: ModuleConfig, context: ModuleConfigContext) { - let varfileVars: DeepPrimitiveMap = {} + let varfileVars: ParsedTemplate = {} if (config.varfile) { const varfilePath = deepEvaluate(config.varfile, { context, @@ -183,12 +183,16 @@ export class VariablesContext extends LayeredContext { message: `Expected varfile template expression in module configuration ${config.name} to resolve to string, actually got ${typeof varfilePath}`, }) } - varfileVars = await loadVarfile({ + const rawVarfileVars = await loadVarfile({ configRoot: config.path, path: varfilePath, defaultPath: undefined, log: garden.log, }) + varfileVars = parseTemplateCollection({ + value: rawVarfileVars.data, + source: rawVarfileVars.source, + }) } return new this(describeConfig(config), { @@ -205,21 +209,13 @@ export class VariablesContext extends LayeredContext { group?: GroupConfig ) { const effectiveConfigFileLocation = getEffectiveConfigFileLocation(config) - const rawActionVarfileVars = await loadVarfiles(garden, effectiveConfigFileLocation, config.varfiles || []) - const actionVarfileVars = parseTemplateCollection({ - value: rawActionVarfileVars, - source: { path: ["varfiles"], yamlDoc: config.internal.yamlDoc }, - }) + const actionVarfileVars = await loadVarfiles(garden, effectiveConfigFileLocation, config.varfiles || []) const actionVariables = [config.variables, ...actionVarfileVars] let groupVarfileVars: ParsedTemplate[] = [] let groupVariables: ParsedTemplate[] = [] if (group) { - const rawGroupVarfileVars = await loadVarfiles(garden, group.path, group.varfiles || []) - groupVarfileVars = parseTemplateCollection({ - value: rawGroupVarfileVars, - source: { path: [] }, - }) + groupVarfileVars = await loadVarfiles(garden, group.path, group.varfiles || []) groupVariables = [group.variables, ...groupVarfileVars] } @@ -294,15 +290,19 @@ async function loadVarfiles(garden: Garden, configRoot: string, varfiles: Varfil ) const varsByFile = await Promise.all( - (resolvedVarFiles || []).map((varfile) => { + (resolvedVarFiles || []).map(async (varfile) => { const { path, optional } = getVarfileData(varfile) - return loadVarfile({ + const loaded = await loadVarfile({ configRoot, path, defaultPath: undefined, optional, log: garden.log, }) + return parseTemplateCollection({ + value: loaded.data, + source: loaded.source, + }) }) ) From ebdbac5e15e35c87a042a062d607ef95475f182e Mon Sep 17 00:00:00 2001 From: Steffen Neubauer Date: Thu, 20 Feb 2025 19:20:19 +0100 Subject: [PATCH 2/2] fix: allow empty documents --- core/src/config/base.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/core/src/config/base.ts b/core/src/config/base.ts index 553fd17067..146edc3e4d 100644 --- a/core/src/config/base.ts +++ b/core/src/config/base.ts @@ -657,7 +657,16 @@ export const loadVarfile = profileAsync(async function loadVarfile({ version: "1.2", sourceDescription: `varfile at ${relPath}`, }) - if (loaded.length !== 1) { + if (loaded.length === 0) { + // We treat empty documents as an empty object mapping + return { + data: {}, + source: { + path: [], + }, + } + } + if (loaded.length > 1) { throw new ConfigurationError({ message: `Configured variable file ${relPath} must be a single YAML document. Got multiple (${loaded.length}) YAML documents`, })