From 2a59e27b36f09d1b5b838c6ad237fd662da55f14 Mon Sep 17 00:00:00 2001
From: Javier Viola <363911+pepoviola@users.noreply.github.com>
Date: Sun, 3 Mar 2024 12:24:39 -0300
Subject: [PATCH] Validate spec for provider (#179)
Add small validation, mostly to ensure that the `image` is set in k8s.
fix #143
---
crates/configuration/src/parachain.rs | 2 +-
crates/orchestrator/src/errors.rs | 2 +
crates/orchestrator/src/lib.rs | 117 ++++++++++++++++++++-
crates/provider/src/kubernetes/provider.rs | 6 ++
crates/provider/src/lib.rs | 2 +
crates/provider/src/native/provider.rs | 6 ++
6 files changed, 129 insertions(+), 6 deletions(-)
diff --git a/crates/configuration/src/parachain.rs b/crates/configuration/src/parachain.rs
index 8d07b7f..4e027e6 100644
--- a/crates/configuration/src/parachain.rs
+++ b/crates/configuration/src/parachain.rs
@@ -687,7 +687,7 @@ impl<C: Context> ParachainConfigBuilder<WithId, C> {
/// Add a new collator using a nested [`NodeConfigBuilder`].
pub fn with_collator(
self,
- f: fn(NodeConfigBuilder<node::Initial>) -> NodeConfigBuilder<node::Buildable>,
+ f: impl FnOnce(NodeConfigBuilder<node::Initial>) -> NodeConfigBuilder<node::Buildable>,
) -> ParachainConfigBuilder<WithAtLeastOneCollator, C> {
match f(NodeConfigBuilder::new(
self.default_chain_context(),
diff --git a/crates/orchestrator/src/errors.rs b/crates/orchestrator/src/errors.rs
index d8cd058..8a65c09 100644
--- a/crates/orchestrator/src/errors.rs
+++ b/crates/orchestrator/src/errors.rs
@@ -10,6 +10,8 @@ pub enum OrchestratorError {
// TODO: improve invalid config reporting
#[error("Invalid network configuration: {0}")]
InvalidConfig(String),
+ #[error("Invalid network config to use provider {0}: {1}")]
+ InvalidConfigForProvider(String, String),
#[error("Invalid configuration for node: {0}, field: {1}")]
InvalidNodeConfig(String, String),
#[error("Invariant not fulfilled {0}")]
diff --git a/crates/orchestrator/src/lib.rs b/crates/orchestrator/src/lib.rs
index a4d8dc0..9da760f 100644
--- a/crates/orchestrator/src/lib.rs
+++ b/crates/orchestrator/src/lib.rs
@@ -18,7 +18,10 @@ use configuration::{NetworkConfig, RegistrationStrategy};
use errors::OrchestratorError;
use network::{parachain::Parachain, relaychain::Relaychain, Network};
use network_spec::{parachain::ParachainSpec, NetworkSpec};
-use provider::{types::TransferedFile, DynProvider};
+use provider::{
+ types::{ProviderCapabilities, TransferedFile},
+ DynProvider,
+};
use support::fs::{FileSystem, FileSystemError};
use tokio::time::timeout;
use tracing::{debug, info};
@@ -70,16 +73,21 @@ where
// main driver for spawn the network
debug!("Network spec to spawn, {:#?}", network_spec);
+ // TODO: move to Provider trait
+ validate_spec_with_provider_capabilities(&network_spec, self.provider.capabilities())
+ .map_err(|err| {
+ OrchestratorError::InvalidConfigForProvider(
+ self.provider.name().into(),
+ err.to_string(),
+ )
+ })?;
+
// create namespace
let ns = self.provider.create_namespace().await?;
info!("🧰 ns: {}", ns.name());
info!("🧰 base_dir: {:?}", ns.base_dir());
- // TODO: noop for native
- // Static setup
- // ns.static_setup().await?;
-
let base_dir = ns.base_dir().to_string_lossy();
let scoped_fs = ScopedFilesystem::new(&self.filesystem, &base_dir);
// Create chain-spec for relaychain
@@ -357,6 +365,41 @@ where
}
}
+// Validate that the config fulfill all the requirements of the provider
+fn validate_spec_with_provider_capabilities(
+ network_spec: &NetworkSpec,
+ capabilities: &ProviderCapabilities,
+) -> Result<(), anyhow::Error> {
+ if !capabilities.requires_image {
+ return Ok(());
+ }
+
+ // Relaychain
+ if network_spec.relaychain.default_image.is_none() {
+ // we should check if each node have an image
+ let nodes = &network_spec.relaychain.nodes;
+ if nodes.iter().any(|node| node.image.is_none()) {
+ return Err(anyhow::anyhow!(
+ "missing image for node, and not default is set at relaychain"
+ ));
+ }
+ };
+
+ // Paras
+ for para in &network_spec.parachains {
+ if para.default_image.is_none() {
+ let nodes = ¶.collators;
+ if nodes.iter().any(|node| node.image.is_none()) {
+ return Err(anyhow::anyhow!(
+ "missing image for node, and not default is set at parachain {}",
+ para.id
+ ));
+ }
+ }
+ }
+
+ Ok(())
+}
// TODO: get the fs from `DynNamespace` will make this not needed
// but the FileSystem trait isn't object-safe so we can't pass around
// as `dyn FileSystem`. We can refactor or using some `erase` techniques
@@ -444,3 +487,67 @@ pub enum ZombieRole {
// re-export
pub use network::{AddCollatorOptions, AddNodeOptions};
pub use shared::types::PjsResult;
+
+#[cfg(test)]
+mod tests {
+ use configuration::NetworkConfigBuilder;
+
+ use super::*;
+
+ fn generate(with_image: bool) -> Result<NetworkConfig, Vec<anyhow::Error>> {
+ NetworkConfigBuilder::new()
+ .with_relaychain(|r| {
+ let mut relay = r
+ .with_chain("rococo-local")
+ .with_default_command("polkadot");
+ if with_image {
+ relay = relay.with_default_image("docker.io/parity/polkadot")
+ }
+
+ relay
+ .with_node(|node| node.with_name("alice"))
+ .with_node(|node| node.with_name("bob"))
+ })
+ .with_parachain(|p| {
+ p.with_id(2000).cumulus_based(true).with_collator(|n| {
+ let node = n.with_name("collator").with_command("polkadot-parachain");
+ if with_image {
+ node.with_image("docker.io/paritypr/test-parachain")
+ } else {
+ node
+ }
+ })
+ })
+ .build()
+ }
+
+ #[tokio::test]
+ async fn valid_config_with_image() {
+ let network_config = generate(true).unwrap();
+ let spec = NetworkSpec::from_config(&network_config).await.unwrap();
+ let caps = ProviderCapabilities {
+ requires_image: true,
+ has_resources: false,
+ prefix_with_full_path: false,
+ use_default_ports_in_cmd: false,
+ };
+
+ let valid = validate_spec_with_provider_capabilities(&spec, &caps);
+ assert!(valid.is_ok())
+ }
+
+ #[tokio::test]
+ async fn invalid_config() {
+ let network_config = generate(false).unwrap();
+ let spec = NetworkSpec::from_config(&network_config).await.unwrap();
+ let caps = ProviderCapabilities {
+ requires_image: true,
+ has_resources: false,
+ prefix_with_full_path: false,
+ use_default_ports_in_cmd: false,
+ };
+
+ let valid = validate_spec_with_provider_capabilities(&spec, &caps);
+ assert!(valid.is_err())
+ }
+}
diff --git a/crates/provider/src/kubernetes/provider.rs b/crates/provider/src/kubernetes/provider.rs
index fa73a92..29cba80 100644
--- a/crates/provider/src/kubernetes/provider.rs
+++ b/crates/provider/src/kubernetes/provider.rs
@@ -13,6 +13,8 @@ use crate::{
types::ProviderCapabilities, DynNamespace, Provider, ProviderError, ProviderNamespace,
};
+const PROVIDER_NAME: &str = "k8s";
+
pub struct KubernetesProvider<FS>
where
FS: FileSystem + Send + Sync + Clone,
@@ -58,6 +60,10 @@ impl<FS> Provider for KubernetesProvider<FS>
where
FS: FileSystem + Send + Sync + Clone + 'static,
{
+ fn name(&self) -> &str {
+ PROVIDER_NAME
+ }
+
fn capabilities(&self) -> &ProviderCapabilities {
&self.capabilities
}
diff --git a/crates/provider/src/lib.rs b/crates/provider/src/lib.rs
index 5e6ccf9..f7139c3 100644
--- a/crates/provider/src/lib.rs
+++ b/crates/provider/src/lib.rs
@@ -104,6 +104,8 @@ pub enum ProviderError {
#[async_trait]
pub trait Provider {
+ fn name(&self) -> &str;
+
fn capabilities(&self) -> &ProviderCapabilities;
async fn namespaces(&self) -> HashMap<String, DynNamespace>;
diff --git a/crates/provider/src/native/provider.rs b/crates/provider/src/native/provider.rs
index 5124c70..799c379 100644
--- a/crates/provider/src/native/provider.rs
+++ b/crates/provider/src/native/provider.rs
@@ -13,6 +13,8 @@ use crate::{
types::ProviderCapabilities, DynNamespace, Provider, ProviderError, ProviderNamespace,
};
+const PROVIDER_NAME: &str = "native";
+
pub struct NativeProvider<FS>
where
FS: FileSystem + Send + Sync + Clone,
@@ -58,6 +60,10 @@ impl<FS> Provider for NativeProvider<FS>
where
FS: FileSystem + Send + Sync + Clone + 'static,
{
+ fn name(&self) -> &str {
+ PROVIDER_NAME
+ }
+
fn capabilities(&self) -> &ProviderCapabilities {
&self.capabilities
}
--
GitLab