Skip to content

Commit

Permalink
fix(template): establish backwards bug-compatibility for kubernetes m…
Browse files Browse the repository at this point in the history
…anifest files (#6713)

* fix(template): establish backwards bug-compatibility for kubernetes manifest files

Template strings in Kubernetes manifest files are partially resolved for legacy reasons. The partial resolve behaviour has been changed to improve sanity in #6685 but for kubernetes manifest files, we need to have a version of the old behaviour.

The expected legacy behaviour looks like this:
- If template strings reference variables that do not exist there is no error
- If a template string contains multiple template expressions, each expression can be partially resolved.
- Template expressions are evaluated before parsing yaml, which means that valid yaml with template expressions can be resolved to invalid yaml (e.g. if variable values contain special characters)

This legacy behaviour can lead to quite some surprises and UX problems, for example #5266

I would suggest that we remove this functionality in 0.14.

* test: fix running tests on macos

this commit fixes the "cannot find dockerfile" error that plagued us on
macos

* test: add integ tests for legacyAllowPartial behaviour in kubernetes
manifests
  • Loading branch information
stefreak authored Dec 12, 2024
1 parent a5783bd commit 424b392
Show file tree
Hide file tree
Showing 11 changed files with 464 additions and 137 deletions.
4 changes: 4 additions & 0 deletions core/src/config/template-contexts/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export type ContextKey = ContextKeySegment[]
export interface ContextResolveOpts {
// Allow templates to be partially resolved (used to defer runtime template resolution, for example)
allowPartial?: boolean
// This is kept for backwards compatibility of rendering kubernetes manifests
// TODO(0.14): Do not allow the use of template strings in kubernetes manifest files
// TODO(0.14): Remove legacyAllowPartial
legacyAllowPartial?: boolean
// Allow partial resolution for values that originate from a special context that always returns CONTEXT_RESOLVE_KEY_AVAILABLE_LATER.
// This is used for module resolution and can be removed whenever we remove support for modules.
allowPartialContext?: boolean
Expand Down
12 changes: 11 additions & 1 deletion core/src/plugins/kubernetes/kubernetes-type/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,17 @@ async function readFileManifests(
const absPath = resolve(manifestPath, path)
log.debug(`Reading manifest for ${action.longDescription()} from path ${absPath}`)
const str = (await readFile(absPath)).toString()
const resolved = ctx.resolveTemplateStrings(str, { allowPartial: true, unescape: true })

// TODO(0.14): Do not resolve template strings in unparsed YAML and remove legacyAllowPartial
// First of all, evaluating template strings can result in invalid YAML that fails to parse, because the result of the
// template expressions will be interpreted by the YAML parser later.
// Then also, the use of `legacyAllowPartial: true` is quite unfortunate here, because users will not notice
// if they reference variables that do not exist.
const resolved = ctx.resolveTemplateStrings(str, {
legacyAllowPartial: true,
unescape: true,
})

const manifests = await parseKubernetesManifests(
resolved,
`${basename(absPath)} in directory ${dirname(absPath)} (specified in ${action.longDescription()})`,
Expand Down
Loading

0 comments on commit 424b392

Please sign in to comment.