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.

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

Prevent config rendering on every process #3011

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions changelog.d/2936.fixed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Stopped mirrord entering a crash loop when trying to load into some processes like VSCode's `watchdog.js` when the user config contained a call to `get_env()`, which occurred due to missing env - the config is now only rendered once and set into an env var.
2 changes: 2 additions & 0 deletions mirrord/cli/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ pub(crate) async fn container_command(
(CONTAINER_EXECUTION_KIND as u32).to_string(),
);

// LayerConfig must be created after setting relevant env vars
let (mut config, mut analytics) = create_config_and_analytics(&mut progress, watch)?;

let (_internal_proxy_tls_guards, _external_proxy_tls_guards) =
Expand Down Expand Up @@ -427,6 +428,7 @@ pub(crate) async fn container_ext_command(
env.insert("MIRRORD_IMPERSONATED_TARGET".into(), target.to_string());
}

// LayerConfig must be created after setting relevant env vars
let (mut config, mut analytics) = create_config_and_analytics(&mut progress, watch)?;

let (_internal_proxy_tls_guards, _external_proxy_tls_guards) =
Expand Down
7 changes: 3 additions & 4 deletions mirrord/cli/src/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -289,10 +289,9 @@ impl MirrordExecution {
})?;

// Provide details for layer to connect to agent via internal proxy
env_vars.insert(
MIRRORD_CONNECT_TCP_ENV.to_string(),
format!("127.0.0.1:{}", address.port()),
);
let mut config = config.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Instead of clone to mut here, why not take this as &mut in the function args?

Copy link
Member

Choose a reason for hiding this comment

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

Im a bit worried for container feature behaviour where we split up the env variables for sidecar and main container relying on MIRRORD_CONNECT_TCP_ENV as env value

config.connect_tcp = Some(format!("127.0.0.1:{}", address.port()));
config.update_env()?;

// Fixes <https://github.com/metalbear-co/mirrord/issues/1745>
// by disabling the fork safety check in the Objective-C runtime.
Expand Down
2 changes: 2 additions & 0 deletions mirrord/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,7 @@ async fn exec(args: &ExecArgs, watch: drain::Watch) -> CliResult<()> {
std::env::set_var(name, value);
}

// LayerConfig must be created after setting relevant env vars
let (config, mut context) = LayerConfig::from_env_with_warnings()?;

let mut analytics = AnalyticsReporter::only_error(config.telemetry, Default::default(), watch);
Expand Down Expand Up @@ -472,6 +473,7 @@ async fn port_forward(args: &PortForwardArgs, watch: drain::Watch) -> CliResult<
std::env::set_var("MIRRORD_CONFIG_FILE", config_file);
}

// LayerConfig must be created after setting relevant env vars
let (config, mut context) = LayerConfig::from_env_with_warnings()?;

let mut analytics = AnalyticsReporter::new(config.telemetry, ExecutionKind::PortForward, watch);
Expand Down
7 changes: 4 additions & 3 deletions mirrord/config/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ edition.workspace = true
workspace = true

[dependencies]
mirrord-config-derive = { path = "./derive"}
mirrord-analytics = { path = "../analytics"}
mirrord-config-derive = { path = "./derive" }
mirrord-analytics = { path = "../analytics" }
gememma marked this conversation as resolved.
Show resolved Hide resolved

serde.workspace = true
serde_json.workspace = true
Expand All @@ -27,13 +27,14 @@ tracing.workspace = true
serde_yaml.workspace = true
toml = "0.8"
schemars.workspace = true
bimap = "0.6"
bimap = { version = "0.6", features = ["serde"] }
nom = "7.1"
ipnet.workspace = true
bitflags = "2"
k8s-openapi = { workspace = true, features = ["schemars", "earliest"] }
tera = "1"
fancy-regex.workspace = true
base64.workspace = true

[dev-dependencies]
rstest.workspace = true
6 changes: 3 additions & 3 deletions mirrord/config/src/agent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl fmt::Display for LinuxCapability {
/// }
/// }
/// ```
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "AgentFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq"))]
pub struct AgentConfig {
Expand Down Expand Up @@ -354,7 +354,7 @@ pub struct AgentConfig {
/// Create an agent that returns an error after accepting the first client. For testing
/// purposes. Only supported with job agents (not with ephemeral agents).
#[cfg(all(debug_assertions, not(test)))] // not(test) so that it's not included in the schema json.
#[serde(skip_serializing)]
#[serde(skip_serializing, skip_deserializing)]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[serde(skip_serializing, skip_deserializing)]
#[serde(skip)]

#[config(env = "MIRRORD_AGENT_TEST_ERROR", default = false, unstable)]
pub test_error: bool,
}
Expand Down Expand Up @@ -477,7 +477,7 @@ impl AgentFileConfig {
}
}

#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
#[config(derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct AgentDnsConfig {
Expand Down
6 changes: 6 additions & 0 deletions mirrord/config/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,12 @@ pub enum ConfigError {
value: String,
fail: Box<fancy_regex::Error>,
},

#[error("mirrord-config: decoding resolved config from env var failed with `{0}`")]
EnvVarDecodeError(String),

#[error("mirrord-config: encoding resolved config failed with `{0}`")]
EnvVarEncodeError(String),
}

impl From<tera::Error> for ConfigError {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/container.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::path::PathBuf;

use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use crate::config::source::MirrordConfigSource;

Expand All @@ -12,7 +12,7 @@ static DEFAULT_CLI_IMAGE: &str = concat!(
);

/// Unstable: `mirrord container` command specific config.
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "ContainerFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq"))]
pub struct ContainerConfig {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/experimental.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use mirrord_analytics::CollectAnalytics;
use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use crate::config::source::MirrordConfigSource;

/// mirrord Experimental features.
/// This shouldn't be used unless someone from MetalBear/mirrord tells you to.
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "ExperimentalFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct ExperimentalConfig {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/external_proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::path::PathBuf;

use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use crate::config::source::MirrordConfigSource;

Expand All @@ -23,7 +23,7 @@ pub static MIRRORD_EXTERNAL_TLS_KEY_ENV: &str = "MIRRORD_EXTERNAL_TLS_KEY";
/// }
/// }
/// ```
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "ExternalProxyFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq"))]
pub struct ExternalProxyConfig {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/feature.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use mirrord_analytics::CollectAnalytics;
use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use self::{copy_target::CopyTargetConfig, env::EnvConfig, fs::FsConfig, network::NetworkConfig};
use crate::{config::source::MirrordConfigSource, feature::split_queues::SplitQueuesConfig};
Expand Down Expand Up @@ -64,7 +64,7 @@ pub mod split_queues;
/// }
/// }
/// ```
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "FeatureFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct FeatureConfig {
Expand Down
2 changes: 1 addition & 1 deletion mirrord/config/src/feature/copy_target.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ impl FromMirrordConfig for CopyTargetConfig {
/// }
/// }
/// ```
#[derive(Clone, Debug, Serialize)]
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq)]
pub struct CopyTargetConfig {
pub enabled: bool,

Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/feature/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{collections::HashMap, path::PathBuf};
use mirrord_analytics::CollectAnalytics;
use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use crate::{
config::{from_env::FromEnv, source::MirrordConfigSource, ConfigContext, Result},
Expand Down Expand Up @@ -47,7 +47,7 @@ pub const MIRRORD_OVERRIDE_ENV_FILE_ENV: &str = "MIRRORD_OVERRIDE_ENV_VARS_FILE"
/// }
/// }
/// ```
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "EnvFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct EnvConfig {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/feature/fs/advanced.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::collections::HashMap;
use mirrord_analytics::{AnalyticValue, CollectAnalytics};
use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use super::{FsModeConfig, FsUserConfig};
use crate::{
Expand Down Expand Up @@ -80,7 +80,7 @@ use crate::{
/// }
/// }
/// ```
#[derive(MirrordConfig, Default, Clone, PartialEq, Eq, Debug, Serialize)]
#[derive(MirrordConfig, Default, Clone, PartialEq, Eq, Debug, Serialize, Deserialize)]
#[config(
map_to = "AdvancedFsUserConfig",
derive = "PartialEq,Eq,JsonSchema",
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/feature/network.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use dns::{DnsConfig, DnsFileConfig};
use mirrord_analytics::CollectAnalytics;
use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use self::{incoming::*, outgoing::*};
use crate::{
Expand Down Expand Up @@ -54,7 +54,7 @@ pub mod outgoing;
/// }
/// }
/// ```
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
#[config(map_to = "NetworkFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct NetworkConfig {
Expand Down
2 changes: 1 addition & 1 deletion mirrord/config/src/feature/network/dns.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub enum DnsFilterConfig {
/// `read_only: ["/etc/resolv.conf"]`.
/// - DNS filter currently works only with frameworks that use `getaddrinfo`/`gethostbyname`
/// functions.
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
#[config(map_to = "DnsFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct DnsConfig {
Expand Down
19 changes: 2 additions & 17 deletions mirrord/config/src/feature/network/incoming.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::{collections::HashSet, fmt, str::FromStr};
use bimap::BiMap;
use mirrord_analytics::{AnalyticValue, Analytics, CollectAnalytics};
use schemars::JsonSchema;
use serde::{de, ser, ser::SerializeSeq as _, Deserialize, Serialize};
use serde::{de, Deserialize, Serialize};
use thiserror::Error;

use crate::{
Expand Down Expand Up @@ -306,19 +306,6 @@ pub struct IncomingAdvancedFileConfig {
pub ports: Option<Vec<u16>>,
}

fn serialize_bi_map<S>(map: &BiMap<u16, u16>, serializer: S) -> Result<S::Ok, S::Error>
meowjesty marked this conversation as resolved.
Show resolved Hide resolved
where
S: ser::Serializer,
{
let mut seq = serializer.serialize_seq(Some(map.len()))?;

for (key, value) in map {
seq.serialize_element(&[key, value])?;
}

seq.end()
}

/// Controls the incoming TCP traffic feature.
///
/// See the incoming [reference](https://mirrord.dev/docs/reference/traffic/#incoming) for more
Expand Down Expand Up @@ -387,7 +374,7 @@ where
/// }
/// }
/// ```
#[derive(Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
pub struct IncomingConfig {
/// #### feature.network.incoming.port_mapping {#feature-network-incoming-port_mapping}
///
Expand All @@ -396,7 +383,6 @@ pub struct IncomingConfig {
/// This is useful when you want to mirror/steal a port to a different port on the remote
/// machine. For example, your local process listens on port `9333` and the container listens
/// on port `80`. You'd use `[[9333, 80]]`
#[serde(serialize_with = "serialize_bi_map")]
pub port_mapping: BiMap<u16, u16>,

/// #### feature.network.incoming.ignore_localhost {#feature-network-incoming-ignore_localhost}
Expand Down Expand Up @@ -434,7 +420,6 @@ pub struct IncomingConfig {
/// you probably can't listen on `80` without sudo, so you can use `[[80, 4480]]`
/// then access it on `4480` while getting traffic from remote `80`.
/// The value of `port_mapping` doesn't affect this.
#[serde(serialize_with = "serialize_bi_map")]
pub listen_ports: BiMap<u16, u16>,

/// #### feature.network.incoming.on_concurrent_steal {#feature-network-incoming-on_concurrent_steal}
Expand Down
2 changes: 1 addition & 1 deletion mirrord/config/src/feature/network/incoming/http_filter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ use crate::{
/// { "header": "^x-debug-session: 121212$" }
/// ]
///}
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
#[config(map_to = "HttpFilterFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct HttpFilterConfig {
Expand Down
2 changes: 1 addition & 1 deletion mirrord/config/src/feature/network/outgoing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ pub enum OutgoingFilterConfig {
/// }
/// }
/// ```
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Default, PartialEq, Eq, Clone, Debug, Serialize, Deserialize)]
#[config(map_to = "OutgoingFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq, Eq"))]
pub struct OutgoingConfig {
Expand Down
4 changes: 2 additions & 2 deletions mirrord/config/src/internal_proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{net::SocketAddr, path::PathBuf};

use mirrord_config_derive::MirrordConfig;
use schemars::JsonSchema;
use serde::Serialize;
use serde::{Deserialize, Serialize};

use crate::config::source::MirrordConfigSource;

Expand All @@ -26,7 +26,7 @@ pub static MIRRORD_INTPROXY_CLIENT_TLS_KEY_ENV: &str = "MIRRORD_INTPROXY_CLIENT_
/// }
/// }
/// ```
#[derive(MirrordConfig, Clone, Debug, Serialize)]
#[derive(MirrordConfig, Clone, Debug, Serialize, Deserialize, PartialEq)]
#[config(map_to = "InternalProxyFileConfig", derive = "JsonSchema")]
#[cfg_attr(test, config(derive = "PartialEq"))]
pub struct InternalProxyConfig {
Expand Down
Loading
Loading