Remove tap status being explicitly set in config (#838)

This is required as part of linkerd/linkerd2#5326 because the tap env variables
will no longer always be set.

There is a dependency on identity being enabled for tap to work. The
status of tap is determined by the `ENV_TAP_SVC_NAME` env variable being set
or not set.

- If identity is disabled, tap is disabled, but a warning is issued if
  `ENV_TAP_SVC_NAME` is set.
- If identity is enabled, the status of tap is determined by
  `ENV_TAP_SVC_NAME`.

Signed-off-by: Kevin Leimkuhler <[email protected]>

Author: kleimkuhler

linkerd/app/integration/src/proxy.rs

@@ -244,18 +244,9 @@ async fn run(proxy: Proxy, mut env: TestEnv, random_ports: bool) -> Listening {
         Some(identity.addr)
     } else {
         env.put(app::env::ENV_IDENTITY_DISABLED, "test".to_owned());
-        env.put(app::env::ENV_TAP_DISABLED, "test".to_owned());
         None
     };
 
-    // If identity is enabled but the test is not concerned with tap, ensure
-    // there is a tap service name set
-    if !env.contains_key(app::env::ENV_TAP_DISABLED)
-        && !env.contains_key(app::env::ENV_TAP_SVC_NAME)
-    {
-        env.put(app::env::ENV_TAP_SVC_NAME, "test-identity".to_owned())
-    }
-
     if let Some(ports) = proxy.inbound_disable_ports_protocol_detection {
         let ports = ports
             .into_iter()

linkerd/app/integration/src/tests/tap.rs

@@ -3,11 +3,14 @@ use crate::*;
 use std::time::SystemTime;
 
 #[tokio::test]
-async fn tap_enabled_when_identity_enabled() {
+async fn tap_enabled_when_tap_svc_name_set() {
     let _trace = trace_init();
 
     let identity = "foo.ns1.serviceaccount.identity.linkerd.cluster.local";
-    let identity_env = identity::Identity::new("foo-ns1", identity.to_string());
+    let mut identity_env = identity::Identity::new("foo-ns1", identity.to_string());
+    identity_env
+        .env
+        .put("LINKERD2_PROXY_TAP_SVC_NAME", "test-identity".to_string());
 
     let proxy = proxy::new()
         .identity(identity_env.service().run().await)
@@ -26,13 +29,14 @@ async fn tap_disabled_when_identity_disabled() {
 }
 
 #[tokio::test]
-async fn can_disable_tap() {
+async fn tap_disabled_when_tap_svc_name_not_set() {
     let _trace = trace_init();
-    let mut env = TestEnv::default();
-    env.put(app::env::ENV_TAP_DISABLED, "true".to_owned());
-
-    let proxy = proxy::new().run_with_test_env(env).await;
-
+    let identity = "foo.ns1.serviceaccount.identity.linkerd.cluster.local";
+    let identity_env = identity::Identity::new("foo-ns1", identity.to_string());
+    let proxy = proxy::new()
+        .identity(identity_env.service().run().await)
+        .run_with_test_env(identity_env.env)
+        .await;
     assert!(proxy.tap.is_none())
 }
 

linkerd/app/src/env.rs

@@ -148,7 +148,6 @@ pub const ENV_DESTINATION_CONTEXT: &str = "LINKERD2_PROXY_DESTINATION_CONTEXT";
 pub const ENV_DESTINATION_PROFILE_INITIAL_TIMEOUT: &str =
     "LINKERD2_PROXY_DESTINATION_PROFILE_INITIAL_TIMEOUT";
 
-pub const ENV_TAP_DISABLED: &str = "LINKERD2_PROXY_TAP_DISABLED";
 pub const ENV_TAP_SVC_NAME: &str = "LINKERD2_PROXY_TAP_SVC_NAME";
 const ENV_RESOLV_CONF: &str = "LINKERD2_PROXY_RESOLV_CONF";
 
@@ -702,52 +701,34 @@ impl Env {
 
 // ===== Parsing =====
 
-/// There is a dependency on identity being enabled for tap to properly work.
-/// Depending on the setting of both, a user could end up in an improperly
-/// configured proxy environment.
+/// There is a dependency on identity being enabled for tap to work. The
+/// status of tap is determined by the ENV_TAP_SVC_NAME env variable being set
+/// or not set.
 ///
-/// ```plain
-///     E = Enabled; D = Disabled
-///     +----------+-----+--------------+
-///     | Identity | Tap | Result       |
-///     +----------+-----+--------------+
-///     | E        | E   | Ok(Some(..)) |
-///     +----------+-----+--------------+
-///     | E        | D   | Ok(None)     |
-///     +----------+-----+--------------+
-///     | D        | E   | Err(..)      |
-///     +----------+-----+--------------+
-///     | D        | D   | Ok(None)     |
-///     +----------+-----+--------------+
-/// ```
+/// - If identity is disabled, tap is disabled, but a warning is issued if
+///   ENV_TAP_SVC_NAME is set.
+/// - If identity is enabled, the status of tap is determined by
+///   ENV_TAP_SVC_NAME.
 fn parse_tap_config(
     strings: &dyn Strings,
     id_disabled: bool,
 ) -> Result<Option<(SocketAddr, IndexSet<identity::Name>)>, EnvError> {
-    let tap_disabled = strings
-        .get(ENV_TAP_DISABLED)?
-        .map(|d| !d.is_empty())
-        .unwrap_or(false);
-    match (id_disabled, tap_disabled) {
-        (_, true) => Ok(None),
-        (true, false) => {
-            error!("{} must be set if identity is disabled", ENV_TAP_DISABLED);
-            Err(EnvError::InvalidEnvVar)
+    let tap_identity = parse(strings, ENV_TAP_SVC_NAME, parse_identity)?;
+    if id_disabled {
+        if tap_identity.is_some() {
+            warn!(
+                "{} should not be set if identity is disabled; continuing with tap disabled",
+                ENV_TAP_SVC_NAME
+            );
         }
-        (false, false) => {
-            let addr = parse(strings, ENV_CONTROL_LISTEN_ADDR, parse_socket_addr)?
-                .unwrap_or_else(|| parse_socket_addr(DEFAULT_CONTROL_LISTEN_ADDR).unwrap());
-            let peer_identity = parse(strings, ENV_TAP_SVC_NAME, parse_identity);
-
-            match peer_identity? {
-                Some(peer_identity) => Ok(Some((addr, vec![peer_identity].into_iter().collect()))),
-                None => {
-                    error!("{} must be set or tap must be disabled", ENV_TAP_SVC_NAME);
-                    Err(EnvError::InvalidEnvVar)
-                }
-            }
+    } else {
+        let addr = parse(strings, ENV_CONTROL_LISTEN_ADDR, parse_socket_addr)?
+            .unwrap_or_else(|| parse_socket_addr(DEFAULT_CONTROL_LISTEN_ADDR).unwrap());
+        if let Some(id) = tap_identity {
+            return Ok(Some((addr, vec![id].into_iter().collect())));
         }
-    }
+    };
+    Ok(None)
 }
 
 fn parse_bool(s: &str) -> Result<bool, ParseError> {