cli: fix 'failed to lookup tunnel' with a name given (microsoft#166507)

Fixes microsoft/vscode-remote-tunnels#563

If a new tunnel `--name` was provided but the existing tunnel was deleted,
the CLI would error out with a lookup failed message. Instead it now
recreates it like it normally would.

Author: connor4312

cli/src/tunnels/dev_tunnels.rs

@@ -12,6 +12,7 @@ use crate::util::input::prompt_placeholder;
 use crate::{debug, info, log, spanf, trace, warning};
 use async_trait::async_trait;
 use futures::TryFutureExt;
+use lazy_static::lazy_static;
 use rand::prelude::IteratorRandom;
 use regex::Regex;
 use reqwest::StatusCode;
@@ -121,7 +122,7 @@ impl AccessTokenProvider for LookupAccessTokenProvider {
 
 		match tunnel_lookup {
 			Ok(tunnel) => Ok(get_host_token_from_tunnel(&tunnel)),
-			Err(e) => Err(wrap(e, "failed to lookup tunnel")),
+			Err(e) => Err(wrap(e, "failed to lookup tunnel for host token")),
 		}
 	}
 }
@@ -212,6 +213,14 @@ fn is_valid_name(name: &str) -> Result<(), InvalidTunnelName> {
 	Ok(())
 }
 
+lazy_static! {
+	static ref HOST_TUNNEL_REQUEST_OPTIONS: TunnelRequestOptions = TunnelRequestOptions {
+		include_ports: true,
+		token_scopes: vec!["host".to_string()],
+		..Default::default()
+	};
+}
+
 /// Structure optionally passed into `start_existing_tunnel` to forward an existing tunnel.
 #[derive(Clone, Debug)]
 pub struct ExistingTunnel {
@@ -260,6 +269,7 @@ impl DevTunnels {
 		Ok(())
 	}
 
+	/// Renames the current tunnel to the new name.
 	pub async fn rename_tunnel(&mut self, name: &str) -> Result<(), AnyError> {
 		is_valid_name(name)?;
 
@@ -269,7 +279,7 @@ impl DevTunnels {
 			Some(t) => t,
 			None => {
 				debug!(self.log, "No code server tunnel found, creating new one");
-				let (persisted, _) = self.create_tunnel(name).await?;
+				let (persisted, _) = self.create_tunnel(name, NO_REQUEST_OPTIONS).await?;
 				self.launcher_tunnel.save(Some(persisted))?;
 				return Ok(());
 			}
@@ -282,7 +292,7 @@ impl DevTunnels {
 			self.log.span("dev-tunnel.tag.get"),
 			self.client.get_tunnel(&locator, NO_REQUEST_OPTIONS)
 		)
-		.map_err(|e| wrap(e, "failed to lookup tunnel"))?;
+		.map_err(|e| wrap(e, "failed to lookup original tunnel"))?;
 
 		full_tunnel.tags = vec![name.to_string(), VSCODE_CLI_TUNNEL_TAG.to_string()];
 		spanf!(
@@ -297,6 +307,70 @@ impl DevTunnels {
 		Ok(())
 	}
 
+	/// Updates the name of the existing persisted tunnel to the new name.
+	/// Gracefully creates a new tunnel if the previous one was deleted.
+	async fn update_tunnel_name(
+		&mut self,
+		persisted: PersistedTunnel,
+		name: &str,
+	) -> Result<(Tunnel, PersistedTunnel), AnyError> {
+		self.check_is_name_free(name).await?;
+
+		debug!(self.log, "Tunnel name changed, applying updates...");
+
+		let (mut full_tunnel, mut persisted, is_new) = self
+			.get_or_create_tunnel(persisted, Some(name), NO_REQUEST_OPTIONS)
+			.await?;
+		if is_new {
+			return Ok((full_tunnel, persisted));
+		}
+
+		full_tunnel.tags = vec![name.to_string(), VSCODE_CLI_TUNNEL_TAG.to_string()];
+
+		let new_tunnel = spanf!(
+			self.log,
+			self.log.span("dev-tunnel.tag.update"),
+			self.client.update_tunnel(&full_tunnel, NO_REQUEST_OPTIONS)
+		)
+		.map_err(|e| wrap(e, "failed to rename tunnel"))?;
+
+		persisted.name = name.to_string();
+		self.launcher_tunnel.save(Some(persisted.clone()))?;
+
+		Ok((new_tunnel, persisted))
+	}
+
+	/// Gets the persisted tunnel from the service, or creates a new one.
+	/// If `create_with_new_name` is given, the new tunnel has that name
+	/// instead of the one previously persisted.
+	async fn get_or_create_tunnel(
+		&mut self,
+		persisted: PersistedTunnel,
+		create_with_new_name: Option<&str>,
+		options: &TunnelRequestOptions,
+	) -> Result<(Tunnel, PersistedTunnel, /* is_new */ bool), AnyError> {
+		let tunnel_lookup = spanf!(
+			self.log,
+			self.log.span("dev-tunnel.tag.get"),
+			self.client.get_tunnel(&persisted.locator(), options)
+		);
+
+		match tunnel_lookup {
+			Ok(ft) => Ok((ft, persisted, false)),
+			Err(HttpError::ResponseError(e))
+				if e.status_code == StatusCode::NOT_FOUND
+					|| e.status_code == StatusCode::FORBIDDEN =>
+			{
+				let (persisted, tunnel) = self
+					.create_tunnel(create_with_new_name.unwrap_or(&persisted.name), options)
+					.await?;
+				self.launcher_tunnel.save(Some(persisted.clone()))?;
+				Ok((tunnel, persisted, true))
+			}
+			Err(e) => Err(wrap(e, "failed to lookup tunnel").into()),
+		}
+	}
+
 	/// Starts a new tunnel for the code server on the port. Unlike `start_new_tunnel`,
 	/// this attempts to reuse or create a tunnel of a preferred name or of a generated friendly tunnel name.
 	pub async fn start_new_launcher_tunnel(
@@ -308,64 +382,23 @@ impl DevTunnels {
 			Some(mut persisted) => {
 				if let Some(name) = preferred_name {
 					if persisted.name.ne(&name) {
-						self.check_is_name_free(&name).await?;
-						let mut full_tunnel = spanf!(
-							self.log,
-							self.log.span("dev-tunnel.tag.get"),
-							self.client
-								.get_tunnel(&persisted.locator(), NO_REQUEST_OPTIONS)
-						)
-						.map_err(|e| wrap(e, "failed to lookup tunnel"))?;
-
-						info!(self.log, "Updating name of existing tunnel");
-
-						full_tunnel.tags =
-							vec![name.to_string(), VSCODE_CLI_TUNNEL_TAG.to_string()];
-						if spanf!(
-							self.log,
-							self.log.span("dev-tunnel.tag.update"),
-							self.client.update_tunnel(&full_tunnel, NO_REQUEST_OPTIONS)
-						)
-						.is_ok()
-						{
-							persisted.name = name.to_string();
-							self.launcher_tunnel.save(Some(persisted.clone()))?;
-						}
+						(_, persisted) = self.update_tunnel_name(persisted, &name).await?;
 					}
 				}
 
-				let tunnel_lookup = spanf!(
-					self.log,
-					self.log.span("dev-tunnel.tag.get"),
-					self.client.get_tunnel(
-						&persisted.locator(),
-						&TunnelRequestOptions {
-							include_ports: true,
-							token_scopes: vec!["host".to_string()],
-							..Default::default()
-						}
-					)
-				);
-
-				match tunnel_lookup {
-					Ok(ft) => (ft, persisted),
-					Err(HttpError::ResponseError(e))
-						if e.status_code == StatusCode::NOT_FOUND
-							|| e.status_code == StatusCode::FORBIDDEN =>
-					{
-						let (persisted, tunnel) = self.create_tunnel(&persisted.name).await?;
-						self.launcher_tunnel.save(Some(persisted.clone()))?;
-						(tunnel, persisted)
-					}
-					Err(e) => return Err(AnyError::from(wrap(e, "failed to lookup tunnel"))),
-				}
+				let (tunnel, persisted, _) = self
+					.get_or_create_tunnel(persisted, None, &HOST_TUNNEL_REQUEST_OPTIONS)
+					.await?;
+				(tunnel, persisted)
 			}
 			None => {
 				debug!(self.log, "No code server tunnel found, creating new one");
 				let name = self
 					.get_name_for_tunnel(preferred_name, use_random_name)
 					.await?;
-				let (persisted, full_tunnel) = self.create_tunnel(&name).await?;
+				let (persisted, full_tunnel) = self
+					.create_tunnel(&name, &HOST_TUNNEL_REQUEST_OPTIONS)
+					.await?;
 				self.launcher_tunnel.save(Some(persisted.clone()))?;
 				(full_tunnel, persisted)
 			}
@@ -419,7 +452,11 @@ impl DevTunnels {
 		.await
 	}
 
-	async fn create_tunnel(&mut self, name: &str) -> Result<(PersistedTunnel, Tunnel), AnyError> {
+	async fn create_tunnel(
+		&mut self,
+		name: &str,
+		options: &TunnelRequestOptions,
+	) -> Result<(PersistedTunnel, Tunnel), AnyError> {
 		info!(self.log, "Creating tunnel with the name: {}", name);
 
 		let mut tried_recycle = false;
@@ -433,7 +470,7 @@ impl DevTunnels {
 			let result = spanf!(
 				self.log,
 				self.log.span("dev-tunnel.create"),
-				self.client.create_tunnel(&new_tunnel, NO_REQUEST_OPTIONS)
+				self.client.create_tunnel(&new_tunnel, options)
 			);
 
 			match result {
@@ -446,9 +483,9 @@ impl DevTunnels {
 					}
 
 					return Err(AnyError::from(TunnelCreationFailed(
-                        name.to_string(),
-                        "You've exceeded the 10 machine limit for the port fowarding service. Please remove other machines before trying to add this machine.".to_string(),
-                    )));
+						name.to_string(),
+						"You've exceeded the 10 machine limit for the port fowarding service. Please remove other machines before trying to add this machine.".to_string(),
+					)));
 				}
 				Err(e) => {
 					return Err(AnyError::from(TunnelCreationFailed(

cli/src/util/zipper.rs

@@ -54,12 +54,7 @@ where
 	let mut archive = zip::ZipArchive::new(file)
 		.map_err(|e| wrap(e, format!("failed to open zip archive {}", path.display())))?;
 
-	let skip_segments_no = if should_skip_first_segment(&mut archive) {
-		1
-	} else {
-		0
-	};
-
+	let skip_segments_no = usize::from(should_skip_first_segment(&mut archive));
 	for i in 0..archive.len() {
 		reporter.report_progress(i as u64, archive.len() as u64);
 		let mut file = archive
@@ -83,7 +78,7 @@ where
 		}
 
 		if let Some(p) = outpath.parent() {
-			fs::create_dir_all(&p)
+			fs::create_dir_all(p)
 				.map_err(|e| wrap(e, format!("could not create dir for {}", outpath.display())))?;
 		}
 
@@ -138,7 +133,7 @@ fn apply_permissions(file: &ZipFile, outpath: &Path) -> Result<(), WrappedError>
 	use std::os::unix::fs::PermissionsExt;
 
 	if let Some(mode) = file.unix_mode() {
-		fs::set_permissions(&outpath, fs::Permissions::from_mode(mode)).map_err(|e| {
+		fs::set_permissions(outpath, fs::Permissions::from_mode(mode)).map_err(|e| {
 			wrap(
 				e,
 				format!("error setting permissions on {}", outpath.display()),