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.

Sign up for GitHub

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

Jump to bottom

feat(oci): manifest/config updates to support containerd #1882

Merged
merged 5 commits into from
Oct 31, 2023
Merged

feat(oci): manifest/config updates to support containerd #1882

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
604f038
feat(oci): manifest/config updates to support containerd
vdice Oct 11, 2023
50053da
remove artifactType from OCI manifest
vdice Oct 17, 2023
6b1db26
create default config for OCI manifest
vdice Oct 19, 2023
dd07144
bump oci-distribution rev to get Os::Wasip1; update OCI config with a…
vdice Oct 20, 2023
464214b
keep WASM_LAYER_MEDIA_TYPE the same until canonical value finalized
vdice Oct 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/oci/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ dkregistry = { git = "https://github.com/camallo/dkregistry-rs", rev = "37acecb4
docker_credential = "1.0"
dirs = "4.0"
futures-util = "0.3"
oci-distribution = { git = "https://github.com/fermyon/oci-distribution", rev = "05022618d78feef9b99f20b5da8fd6def6bb80d2" }
oci-distribution = { git = "https://github.com/fermyon/oci-distribution", rev = "63cbb0925775e0c9c870195cad1d50ac8707a264" }
reqwest = "0.11"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
Expand Down
67 changes: 48 additions & 19 deletions crates/oci/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,9 @@ use anyhow::{bail, Context, Result};
use docker_credential::DockerCredential;
use futures_util::future;
use futures_util::stream::{self, StreamExt, TryStreamExt};
use oci_distribution::token_cache::RegistryTokenType;
use oci_distribution::RegistryOperation;
use oci_distribution::{
client::{Config, ImageLayer},
manifest::OciImageManifest,
secrets::RegistryAuth,
Reference,
client::ImageLayer, config::ConfigFile, manifest::OciImageManifest, secrets::RegistryAuth,
token_cache::RegistryTokenType, Reference, RegistryOperation,
};
use reqwest::Url;
use spin_common::sha256;
Expand All @@ -25,15 +21,15 @@ use walkdir::WalkDir;

use crate::auth::AuthConfig;

// TODO: the media types for application, wasm module, data and archive layer are not final.
// TODO: the media types for application, data and archive layer are not final
/// Media type for a layer representing a locked Spin application configuration
pub const SPIN_APPLICATION_MEDIA_TYPE: &str = "application/vnd.fermyon.spin.application.v1+config";
// Note: we hope to use a canonical value defined upstream for this media type
const WASM_LAYER_MEDIA_TYPE: &str = "application/vnd.wasm.content.layer.v1+wasm";
/// Media type for a layer representing a generic data file used by a Spin application
pub const DATA_MEDIATYPE: &str = "application/vnd.wasm.content.layer.v1+data";
/// Media type for a layer representing a compressed archive of one or more files used by a Spin application
pub const ARCHIVE_MEDIATYPE: &str = "application/vnd.wasm.content.bundle.v1.tar+gzip";
// Note: this will be updated with a canonical value once defined upstream
const WASM_LAYER_MEDIA_TYPE: &str = "application/vnd.wasm.content.layer.v1+wasm";

const CONFIG_FILE: &str = "config.json";
const LATEST_TAG: &str = "latest";
Expand Down Expand Up @@ -164,12 +160,27 @@ impl Client {
locked.components = components;
locked.metadata.remove("origin");

let oci_config = Config {
data: serde_json::to_vec(&locked)?,
media_type: SPIN_APPLICATION_MEDIA_TYPE.to_string(),
annotations: None,
// Push layer for locked spin application config
let locked_config_layer = ImageLayer::new(
serde_json::to_vec(&locked).context("could not serialize locked config")?,
SPIN_APPLICATION_MEDIA_TYPE.to_string(),
None,
);
layers.push(locked_config_layer);

// Construct empty/default OCI config file. Data may be parsed according to
// the expected config structure per the image spec, so we want to ensure it conforms.
// (See https://github.com/opencontainers/image-spec/blob/main/config.md)
// TODO: Explore adding data applicable to the Spin app being published.
let oci_config_file = ConfigFile {
vdice marked this conversation as resolved.
Show resolved Hide resolved
architecture: oci_distribution::config::Architecture::Wasm,
os: oci_distribution::config::Os::Wasip1,
..Default::default()
Copy link
Contributor

@jsturtevant jsturtevant Oct 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

One minor but user experience improvement would be to provide a default entry point https://github.com/containerd/runwasi/blob/2fb77889d5d7b0e590a574cfbc44089b54036811/crates/oci-tar-builder/src/bin.rs#L61C39-L61C39

if this isn't set then when running with ctr need to provide one:

which is the shim.wasm in the command sudo ./ctr run --net-host --rm --runtime=io.containerd.spin.v1 docker.io/ jsturtevant/spin-wasm-shim:latest-2.0 testwasm shim.wasm

otherwise end up with

time="2023-10-30T22:55:30.029486688Z" level=error msg="on non-Windows, at least one process arg entry is required"

time="2023-10-30T22:55:30.029555687Z" level=error msg="failed to initialize container process: missing args in the process spec"

Copy link
Member

@radu-matei radu-matei Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jsturtevant -- what exactly would be the meaning for the "default entrypoint" for a Spin app, which can have multiple components (which means no one wasm component)?

The OCI loader from Spin loads the locked application file and knows which layers are the wasm components, and so it doesn't need any default entrypoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

This value is passed as arg0 in wasmtime shim. Maybe it isn't used spin shim?

Copy link
Member

@radu-matei radu-matei Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

My assumption is that it wouldn't be used in the Spin shim, as the information about components is part of the locked app manifest -- if that's not the case, it's something we can explore in detail as a follow-up for this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I don't think it is blocking. I was using the applications name in runwasi. I am afk, but can look what happens in the docker container scenario later

Copy link
Contributor

Choose a reason for hiding this comment

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

Spin currently ignores this value, runwasi parses it and sends to the shim via RuntimeContext but it is ignored by spin's shim: https://github.com/deislabs/containerd-wasm-shims/blob/6ca8df8247d94cf1dc6ce5e529b00ee728aee4ec/containerd-shim-spin-v1/src/engine.rs#L123

The reason this is working when packages as a container is because / is the command is set on the yaml: https://github.com/deislabs/containerd-wasm-shims/blob/6ca8df8247d94cf1dc6ce5e529b00ee728aee4ec/README.md?plain=1#L95

In the case of a shim such as wasmtime/wasmedge it makes sense to have pass the entrypoint info as arg0 and we even do some additional parsing to allow for entering other functions in the wasm module https://github.com/containerd/runwasi/blob/642cafacde77d25762c8c0c0ca78ff1010caff16/crates/containerd-shim-wasm/src/container/context.rs#L17-L25

So in this case we can continue to use / in the command for the pod yaml or you could set this value in the config (maybe image name?) and users wouldn't need to set the command to / in the yaml. In the future maybe it can be useful otherwise we can revisit this in the next iteration of the config support in containerd.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for all of the great context @jsturtevant. Much appreciated. Tracking in #2000.

};
let oci_config =
oci_distribution::client::Config::oci_v1_from_config_file(oci_config_file, None)?;
let manifest = OciImageManifest::build(&layers, &oci_config, None);

let response = self
.oci
.push(&reference, &layers, oci_config, &auth, Some(manifest))
Expand Down Expand Up @@ -275,16 +286,16 @@ impl Client {
let m = self.manifest_path(&reference.to_string()).await?;
fs::write(&m, &manifest_json).await?;

// Older published Spin apps feature the locked app config *as* the OCI manifest config layer,
// while newer versions publish the locked app config as a generic layer alongside others.
// Assume that these bytes may represent the locked app config and write it as such.
let mut cfg_bytes = Vec::new();
self.oci
.pull_blob(&reference, &manifest.config.digest, &mut cfg_bytes)
.await?;
let cfg = std::str::from_utf8(&cfg_bytes)?;
tracing::debug!("Pulled config: {}", cfg);

// Write the config object in `<cache_root>/registry/oci/manifests/repository:<tag_or_latest>/config.json`
let c = self.lockfile_path(&reference.to_string()).await?;
fs::write(&c, &cfg).await?;
self.write_locked_app_config(&reference.to_string(), &cfg_bytes)
.await
.context("unable to write locked app config to cache")?;

// If a layer is a Wasm module, write it in the Wasm directory.
// Otherwise, write it in the data directory (after unpacking if archive layer)
Expand All @@ -307,6 +318,11 @@ impl Client {
.pull_blob(&reference, &layer.digest, &mut bytes)
.await?;
match layer.media_type.as_str() {
SPIN_APPLICATION_MEDIA_TYPE => {
this.write_locked_app_config(&reference.to_string(), &bytes)
.await
.with_context(|| "unable to write locked app config to cache")?;
}
WASM_LAYER_MEDIA_TYPE => {
this.cache.write_wasm(&bytes, &layer.digest).await?;
}
Expand Down Expand Up @@ -373,6 +389,19 @@ impl Client {
Ok(p.join(CONFIG_FILE))
}

/// Write the config object in `<cache_root>/registry/oci/manifests/repository:<tag_or_latest>/config.json`
async fn write_locked_app_config(
&self,
reference: impl AsRef<str>,
bytes: impl AsRef<[u8]>,
) -> Result<()> {
let cfg = std::str::from_utf8(bytes.as_ref())?;
tracing::debug!("Pulled config: {}", cfg);

let c = self.lockfile_path(reference).await?;
fs::write(&c, &cfg).await.map_err(anyhow::Error::from)
}

/// Create a new wasm layer based on a file.
async fn wasm_layer(file: &Path) -> Result<ImageLayer> {
tracing::log::trace!("Reading wasm module from {:?}", file);
Expand Down
Loading