From 6d5d9aa60e01b39dbbeee497cd8e47af30feab32 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Tue, 21 Jun 2022 12:57:04 +0100 Subject: [PATCH 1/2] Make the Buildpack API version check more robust Previously, if a `buildpack.toml` either: 1. Didn't match the specific custom buildpack type (used when custom fields are specified under `[[metadata]]`) 2. Didn't match the CNB spec ...then a generic "failed to deserialise buildpack.toml" message was shown, rather than any error about the buildpack API version not matching. In cases where the chosen API version is actually the cause of the error (for example if a future API version makes breaking changes to the schema), then this means the underlying cause isn't mentioned. Now, the check uses a minimal `BuildpackDescriptorApiOnly` type when deserialising `buildpack.toml`, to ensure the API version can still be determined in those cases. The `BuildpackDescriptorApiOnly` type was used instead of a raw `toml::value::Table` since otherwise there are a myriad of `Option`s and related to handle, from both the potential lack of `api` field and it not having a value of type string etc. --- libcnb-data/CHANGELOG.md | 1 + libcnb-data/src/buildpack/mod.rs | 15 +++++++++++++ libcnb/CHANGELOG.md | 1 + libcnb/src/exit_code.rs | 2 +- libcnb/src/runtime.rs | 38 ++++++++++++++++++-------------- 5 files changed, 40 insertions(+), 17 deletions(-) diff --git a/libcnb-data/CHANGELOG.md b/libcnb-data/CHANGELOG.md index 8c2991fd..8de91c8d 100644 --- a/libcnb-data/CHANGELOG.md +++ b/libcnb-data/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- Add `BuildpackDescriptorApiOnly` for representing only the API version of a `buildpack.toml` ([#421](https://github.com/heroku/libcnb.rs/pull/421)). - Add `Launch::processes` function to add multiple `Process`es to a `Launch` value at once. ([#418](https://github.com/heroku/libcnb.rs/pull/418)) - `Launch::process`'s argument changed from `Process` to `Into`. ([#418](https://github.com/heroku/libcnb.rs/pull/418)) - Update `fancy-regex` dependency from 0.8.0 to 0.10.0 ([#393](https://github.com/heroku/libcnb.rs/pull/393) and [#394](https://github.com/heroku/libcnb.rs/pull/394)). diff --git a/libcnb-data/src/buildpack/mod.rs b/libcnb-data/src/buildpack/mod.rs index e794c20a..39d3f4d0 100644 --- a/libcnb-data/src/buildpack/mod.rs +++ b/libcnb-data/src/buildpack/mod.rs @@ -145,6 +145,21 @@ pub struct MetaBuildpackDescriptor { pub metadata: BM, } +/// Partial data structure for the Buildpack descriptor (buildpack.toml) of a buildpack. +/// +/// A representation of [buildpack.toml](https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml) +/// that contains only the Buildpack API version from the `api` field. +/// +/// For use when the Buildpack API version must be read from buildpack descriptors that might +/// otherwise be invalid or not match the supported spec version. +/// +/// For all other use-cases, use [`BuildpackDescriptor`], [`SingleBuildpackDescriptor`] or +/// [`MetaBuildpackDescriptor`] instead. +#[derive(Deserialize, Debug)] +pub struct BuildpackDescriptorApiOnly { + pub api: BuildpackApi, +} + #[derive(Deserialize, Debug)] #[serde(deny_unknown_fields)] pub struct Buildpack { diff --git a/libcnb/CHANGELOG.md b/libcnb/CHANGELOG.md index 182dba80..c3cecd0e 100644 --- a/libcnb/CHANGELOG.md +++ b/libcnb/CHANGELOG.md @@ -2,6 +2,7 @@ ## [Unreleased] +- Make the "Buildpack API version mismatch" check still work when `buildpack.toml` doesn't match the spec or custom buildpack type ([#421](https://github.com/heroku/libcnb.rs/pull/421)). - Remove support for custom exit codes from `Buildpack::on_error`. Exit codes are part of the CNB spec and there are cases where some exit codes have special meaning to the CNB lifecycle. This put the burden on the buildpack author to not pick exit codes with special meanings, dependent on the currently executing phase. This makes `Buildpack::on_error` more consistent with the rest of the framework where we don't expose the interface between the buildpack and the CNB lifecycle directly but use abstractions for easier forward-compatibility and to prevent accidental misuse. ([#415](https://github.com/heroku/libcnb.rs/pull/415)). ## [0.7.0] 2022-04-12 diff --git a/libcnb/src/exit_code.rs b/libcnb/src/exit_code.rs index 7c3ab8f7..85aba5af 100644 --- a/libcnb/src/exit_code.rs +++ b/libcnb/src/exit_code.rs @@ -5,7 +5,7 @@ pub(crate) const GENERIC_SUCCESS: i32 = 0; pub(crate) const GENERIC_UNSPECIFIED_ERROR: i32 = 1; -pub(crate) const GENERIC_CNB_API_MISMATCH_ERROR: i32 = 254; +pub(crate) const GENERIC_CNB_API_VERSION_ERROR: i32 = 254; pub(crate) const GENERIC_UNEXPECTED_EXECUTABLE_NAME_ERROR: i32 = 255; pub(crate) const DETECT_DETECTION_PASSED: i32 = 0; diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 8b9cd748..1606a796 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -1,6 +1,6 @@ use crate::build::{BuildContext, InnerBuildResult}; use crate::buildpack::Buildpack; -use crate::data::buildpack::{SingleBuildpackDescriptor, StackId}; +use crate::data::buildpack::{BuildpackDescriptorApiOnly, StackId}; use crate::detect::{DetectContext, InnerDetectResult}; use crate::error::Error; use crate::platform::Platform; @@ -26,22 +26,31 @@ use std::process::exit; /// Don't implement this directly and use the [`buildpack_main`] macro instead! #[doc(hidden)] pub fn libcnb_runtime(buildpack: &B) { - match read_buildpack_descriptor::() { + // Before we do anything else, we must validate that the Buildpack's API version + // matches that supported by libcnb, to improve the UX in cases where the lifecycle + // passes us arguments or env vars we don't expect, due to changes between API versions. + // We use a cut-down buildpack descriptor type, to ensure we can still read the API + // version even if the rest of buildpack.toml doesn't match the spec (or the buildpack's + // chosen custom `metadata` type). + match read_buildpack_descriptor::() { Ok(buildpack_descriptor) => { if buildpack_descriptor.api != LIBCNB_SUPPORTED_BUILDPACK_API { eprintln!("Error: Cloud Native Buildpack API mismatch"); eprintln!( - "This buildpack ({}) uses Cloud Native Buildpacks API version {}.", - &buildpack_descriptor.buildpack.id, &buildpack_descriptor.api, + "This buildpack uses Cloud Native Buildpacks API version {} (specified in buildpack.toml).", + &buildpack_descriptor.api, ); - eprintln!("But the underlying libcnb.rs library requires CNB API {LIBCNB_SUPPORTED_BUILDPACK_API}."); - - exit(exit_code::GENERIC_CNB_API_MISMATCH_ERROR) + eprintln!("However, the underlying libcnb.rs library only supports CNB API {LIBCNB_SUPPORTED_BUILDPACK_API}."); + exit(exit_code::GENERIC_CNB_API_VERSION_ERROR) } } - Err(lib_cnb_error) => { - buildpack.on_error(lib_cnb_error); - exit(exit_code::GENERIC_UNSPECIFIED_ERROR); + Err(libcnb_error) => { + // This case will likely never occur, since Pack/lifecycle validates each buildpack's + // `buildpack.toml` before the buildpack even runs, so the file being missing or the + // `api` key not being set will have already resulted in an error much earlier. + eprintln!("Error: Unable to determine Buildpack API version"); + eprintln!("Cause: {libcnb_error}"); + exit(exit_code::GENERIC_CNB_API_VERSION_ERROR); } } @@ -51,7 +60,6 @@ pub fn libcnb_runtime(buildpack: &B) { // symlinks to their target on some platforms, whereas we need the original filename. let current_exe = args.first(); let current_exe_file_name = current_exe - .as_ref() .map(Path::new) .and_then(Path::file_name) .and_then(OsStr::to_str); @@ -84,7 +92,6 @@ pub fn libcnb_runtime(buildpack: &B) { "Error: Expected the name of this executable to be 'detect' or 'build', but it was '{}'", other.unwrap_or("") ); - eprintln!("The executable name is used to determine the current buildpack phase."); eprintln!("You might want to create 'detect' and 'build' links to this executable and run those instead."); exit(exit_code::GENERIC_UNEXPECTED_EXECUTABLE_NAME_ERROR) @@ -93,8 +100,8 @@ pub fn libcnb_runtime(buildpack: &B) { match result { Ok(code) => exit(code), - Err(lib_cnb_error) => { - buildpack.on_error(lib_cnb_error); + Err(libcnb_error) => { + buildpack.on_error(libcnb_error); exit(exit_code::GENERIC_UNSPECIFIED_ERROR); } } @@ -247,8 +254,7 @@ fn read_buildpack_dir() -> crate::Result { .map(PathBuf::from) } -fn read_buildpack_descriptor( -) -> crate::Result, E> { +fn read_buildpack_descriptor() -> crate::Result { read_buildpack_dir().and_then(|buildpack_dir| { read_toml_file(buildpack_dir.join("buildpack.toml")) .map_err(Error::CannotReadBuildpackDescriptor) From 85655e63f5c98d4eefebff5efb11c312adb54d28 Mon Sep 17 00:00:00 2001 From: Ed Morley <501702+edmorley@users.noreply.github.com> Date: Wed, 22 Jun 2022 12:26:57 +0100 Subject: [PATCH 2/2] Make `BuildpackDescriptorApiOnly` private --- libcnb-data/CHANGELOG.md | 1 - libcnb-data/src/buildpack/mod.rs | 15 --------------- libcnb/src/runtime.rs | 11 ++++++++++- 3 files changed, 10 insertions(+), 17 deletions(-) diff --git a/libcnb-data/CHANGELOG.md b/libcnb-data/CHANGELOG.md index 8de91c8d..8c2991fd 100644 --- a/libcnb-data/CHANGELOG.md +++ b/libcnb-data/CHANGELOG.md @@ -2,7 +2,6 @@ ## [Unreleased] -- Add `BuildpackDescriptorApiOnly` for representing only the API version of a `buildpack.toml` ([#421](https://github.com/heroku/libcnb.rs/pull/421)). - Add `Launch::processes` function to add multiple `Process`es to a `Launch` value at once. ([#418](https://github.com/heroku/libcnb.rs/pull/418)) - `Launch::process`'s argument changed from `Process` to `Into`. ([#418](https://github.com/heroku/libcnb.rs/pull/418)) - Update `fancy-regex` dependency from 0.8.0 to 0.10.0 ([#393](https://github.com/heroku/libcnb.rs/pull/393) and [#394](https://github.com/heroku/libcnb.rs/pull/394)). diff --git a/libcnb-data/src/buildpack/mod.rs b/libcnb-data/src/buildpack/mod.rs index 39d3f4d0..e794c20a 100644 --- a/libcnb-data/src/buildpack/mod.rs +++ b/libcnb-data/src/buildpack/mod.rs @@ -145,21 +145,6 @@ pub struct MetaBuildpackDescriptor { pub metadata: BM, } -/// Partial data structure for the Buildpack descriptor (buildpack.toml) of a buildpack. -/// -/// A representation of [buildpack.toml](https://github.com/buildpacks/spec/blob/main/buildpack.md#buildpacktoml-toml) -/// that contains only the Buildpack API version from the `api` field. -/// -/// For use when the Buildpack API version must be read from buildpack descriptors that might -/// otherwise be invalid or not match the supported spec version. -/// -/// For all other use-cases, use [`BuildpackDescriptor`], [`SingleBuildpackDescriptor`] or -/// [`MetaBuildpackDescriptor`] instead. -#[derive(Deserialize, Debug)] -pub struct BuildpackDescriptorApiOnly { - pub api: BuildpackApi, -} - #[derive(Deserialize, Debug)] #[serde(deny_unknown_fields)] pub struct Buildpack { diff --git a/libcnb/src/runtime.rs b/libcnb/src/runtime.rs index 1606a796..741d6118 100644 --- a/libcnb/src/runtime.rs +++ b/libcnb/src/runtime.rs @@ -1,12 +1,13 @@ use crate::build::{BuildContext, InnerBuildResult}; use crate::buildpack::Buildpack; -use crate::data::buildpack::{BuildpackDescriptorApiOnly, StackId}; +use crate::data::buildpack::{BuildpackApi, StackId}; use crate::detect::{DetectContext, InnerDetectResult}; use crate::error::Error; use crate::platform::Platform; use crate::toml_file::{read_toml_file, write_toml_file}; use crate::{exit_code, LIBCNB_SUPPORTED_BUILDPACK_API}; use serde::de::DeserializeOwned; +use serde::Deserialize; use std::env; use std::ffi::OsStr; use std::fmt::Debug; @@ -196,6 +197,14 @@ pub fn libcnb_runtime_build( } } +// A partial representation of buildpack.toml that contains only the Buildpack API version, +// so that the version can still be read when the buildpack descriptor doesn't match the +// supported spec version. +#[derive(Deserialize)] +struct BuildpackDescriptorApiOnly { + pub api: BuildpackApi, +} + #[doc(hidden)] pub struct DetectArgs { pub platform_dir_path: PathBuf,