Skip to content

Add the ability to disable autostart #366

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

Merged
merged 3 commits into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,12 @@

## Unreleased

### Changed

- Disable autostarting workspaces by default on macOS to prevent an issue where
it wakes periodically and keeps the workspace on. This can be toggled via the
settings.

## 2.9.3 - 2024-02-10

### Fixed
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
package com.coder.gateway

import com.coder.gateway.services.CoderSettingsService
import com.coder.gateway.util.canCreateDirectory
import com.coder.gateway.services.CoderSettingsState
import com.coder.gateway.util.canCreateDirectory
import com.intellij.openapi.components.service
import com.intellij.openapi.options.BoundConfigurable
import com.intellij.openapi.ui.DialogPanel
Expand Down Expand Up @@ -102,6 +102,13 @@ class CoderSettingsConfigurable : BoundConfigurable("Coder") {
CoderGatewayBundle.message("gateway.connector.settings.tls-alt-name.comment")
)
}.layout(RowLayout.PARENT_GRID)
row(CoderGatewayBundle.message("gateway.connector.settings.disable-autostart.heading")) {
checkBox(CoderGatewayBundle.message("gateway.connector.settings.disable-autostart.title"))
.bindSelected(state::disableAutostart)
.comment(
CoderGatewayBundle.message("gateway.connector.settings.disable-autostart.comment")
)
}.layout(RowLayout.PARENT_GRID)
}
}

Expand Down
35 changes: 22 additions & 13 deletions src/main/kotlin/com/coder/gateway/cli/CoderCLIManager.kt
Original file line number Diff line number Diff line change
Expand Up @@ -199,9 +199,11 @@ class CoderCLIManager(

/**
* Configure SSH to use this binary.
*
* This can take a version for testing purposes only.
*/
fun configSsh(workspaceNames: List<String>) {
writeSSHConfig(modifySSHConfig(readSSHConfig(), workspaceNames))
fun configSsh(workspaceNames: List<String>, version: SemVer? = tryVersion()) {
writeSSHConfig(modifySSHConfig(readSSHConfig(), workspaceNames, version))
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: refactor to configSsh(options: configSSHOptions) where options consists of

  workspaceNames: List<String>
  disableAutostart: boolean

This allows you to then simplify the logic in line 235 to

if (opts.disable-autostart) "--disable-autostart" else null

and move the version checking logic outside of modifySSHConfig()

Copy link
Member Author

@code-asher code-asher Feb 23, 2024

Choose a reason for hiding this comment

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

I was thinking of something like this! But then it felt weird because sometimes we would use the settings (for header command) and sometimes not (for disable autostart). Maybe a supports or features object that has keys for what features are supported then I have settings.disableAutostart && supports.disableAutostart.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, that would definitely make things more confusing. Not a blocker for me in any case.

}

/**
Expand All @@ -220,7 +222,7 @@ class CoderCLIManager(
* this deployment and return the modified config or null if it does not
* need to be modified.
*/
private fun modifySSHConfig(contents: String?, workspaceNames: List<String>): String? {
private fun modifySSHConfig(contents: String?, workspaceNames: List<String>, version: SemVer?): String? {
val host = deploymentURL.safeHost()
val startBlock = "# --- START CODER JETBRAINS $host"
val endBlock = "# --- END CODER JETBRAINS $host"
Expand All @@ -230,15 +232,17 @@ class CoderCLIManager(
"--global-config", escape(coderConfigPath.toString()),
if (settings.headerCommand.isNotBlank()) "--header-command" else null,
if (settings.headerCommand.isNotBlank()) escapeSubcommand(settings.headerCommand) else null,
"ssh", "--stdio")
"ssh", "--stdio",
// Autostart on SSH was added in 2.5.0.
if (settings.disableAutostart && version != null && version >= SemVer(2, 5, 0)) "--disable-autostart" else null)
val blockContent = workspaceNames.joinToString(
System.lineSeparator(),
startBlock + System.lineSeparator(),
System.lineSeparator() + endBlock,
transform = {
"""
Host ${getHostName(deploymentURL, it)}
ProxyCommand ${proxyArgs.joinToString(" ")} ${it}
ProxyCommand ${proxyArgs.joinToString(" ")} $it
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
Expand Down Expand Up @@ -333,31 +337,36 @@ class CoderCLIManager(
}

/**
* Returns true if the CLI has the same major/minor/patch version as the
* provided version, false if it does not match, or null if the CLI version
* could not be determined because the binary could not be executed or the
* version could not be parsed.
* Like version(), but logs errors instead of throwing them.
*/
fun matchesVersion(rawBuildVersion: String): Boolean? {
val cliVersion = try {
private fun tryVersion(): SemVer? {
return try {
version()
} catch (e: Exception) {
when (e) {
is JsonSyntaxException,
is InvalidVersionException -> {
logger.info("Got invalid version from $localBinaryPath: ${e.message}")
return null
}
else -> {
// An error here most likely means the CLI does not exist or
// it executed successfully but output no version which
// suggests it is not the right binary.
logger.info("Unable to determine $localBinaryPath version: ${e.message}")
return null
}
}
null
}
}

/**
* Returns true if the CLI has the same major/minor/patch version as the
* provided version, false if it does not match, or null if the CLI version
* could not be determined because the binary could not be executed or the
* version could not be parsed.
*/
fun matchesVersion(rawBuildVersion: String): Boolean? {
val cliVersion = tryVersion() ?: return null
val buildVersion = try {
SemVer.parse(rawBuildVersion)
} catch (e: InvalidVersionException) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package com.coder.gateway.services

import com.coder.gateway.util.OS
import com.coder.gateway.util.getOS
import com.intellij.openapi.components.PersistentStateComponent
import com.intellij.openapi.components.RoamingType
import com.intellij.openapi.components.Service
Expand Down Expand Up @@ -56,6 +58,10 @@ class CoderSettingsState(
// connections. This is useful when the hostname used to connect to the
// Coder service does not match the hostname in the TLS certificate.
var tlsAlternateHostname: String = "",
// Whether to add --disable-autostart to the proxy command. This works
// around issues on macOS where it periodically wakes and Gateway
// reconnects, keeping the workspace constantly up.
var disableAutostart: Boolean = getOS() == OS.MAC,
) : PersistentStateComponent<CoderSettingsState> {
override fun getState(): CoderSettingsState {
return this
Expand Down
3 changes: 3 additions & 0 deletions src/main/kotlin/com/coder/gateway/settings/CoderSettings.kt
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ open class CoderSettings(
val headerCommand: String
get() = state.headerCommand

val disableAutostart: Boolean
get() = state.disableAutostart

/**
* Where the specified deployment should put its data.
*/
Expand Down
6 changes: 6 additions & 0 deletions src/main/resources/messages/CoderGatewayBundle.properties
Original file line number Diff line number Diff line change
Expand Up @@ -111,3 +111,9 @@ gateway.connector.settings.tls-alt-name.comment=Optionally set this to \
an alternate hostname used for verifying TLS connections. This is useful \
when the hostname used to connect to the Coder service does not match the \
hostname in the TLS certificate.
gateway.connector.settings.disable-autostart.heading=Autostart:
gateway.connector.settings.disable-autostart.title=Disable autostart
gateway.connector.settings.disable-autostart.comment=Checking this box will \
cause the plugin to configure the CLI with --disable-autostart. You must go \
through the IDE selection again for the plugin to reconfigure the CLI with \
this setting.
9 changes: 9 additions & 0 deletions src/test/fixtures/outputs/disable-autostart.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# --- START CODER JETBRAINS test.coder.invalid
Host coder-jetbrains--foo--test.coder.invalid
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config ssh --stdio --disable-autostart foo
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
LogLevel ERROR
SetEnv CODER_SSH_SESSION_TYPE=JetBrains
# --- END CODER JETBRAINS test.coder.invalid
9 changes: 9 additions & 0 deletions src/test/fixtures/outputs/no-disable-autostart.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# --- START CODER JETBRAINS test.coder.invalid
Host coder-jetbrains--foo--test.coder.invalid
ProxyCommand /tmp/coder-gateway/test.coder.invalid/coder-linux-amd64 --global-config /tmp/coder-gateway/test.coder.invalid/config ssh --stdio foo
ConnectTimeout 0
StrictHostKeyChecking no
UserKnownHostsFile /dev/null
LogLevel ERROR
SetEnv CODER_SSH_SESSION_TYPE=JetBrains
# --- END CODER JETBRAINS test.coder.invalid
22 changes: 18 additions & 4 deletions src/test/kotlin/com/coder/gateway/cli/CoderCLIManagerTest.kt
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,15 @@ internal class CoderCLIManagerTest {
srv2.stop(0)
}

data class SSHTest(val workspaces: List<String>, val input: String?, val output: String, val remove: String, val headerCommand: String?)
data class SSHTest(
val workspaces: List<String>,
val input: String?,
val output: String,
val remove: String,
val headerCommand: String?,
val disableAutostart: Boolean = false,
val version: SemVer? = null,
)

@Test
fun testConfigureSSH() {
Expand All @@ -256,13 +264,19 @@ internal class CoderCLIManagerTest {
SSHTest(listOf("header"), null, "header-command-windows", "blank", """"C:\Program Files\My Header Command\HeaderCommand.exe" --url="%CODER_URL%" --test="foo bar"""")
} else {
SSHTest(listOf("header"), null, "header-command", "blank", "my-header-command --url=\"\$CODER_URL\" --test=\"foo bar\" --literal='\$CODER_URL'")
}
},
SSHTest(listOf("foo"), null, "disable-autostart", "blank", null, true, SemVer(2, 5, 0)),
SSHTest(listOf("foo"), null, "disable-autostart", "blank", null, true, SemVer(3, 5, 0)),
SSHTest(listOf("foo"), null, "no-disable-autostart", "blank", null, true, SemVer(2, 4, 9)),
SSHTest(listOf("foo"), null, "no-disable-autostart", "blank", null, true, SemVer(1, 0, 1)),
SSHTest(listOf("foo"), null, "no-disable-autostart", "blank", null, true),
)

val newlineRe = "\r?\n".toRegex()

tests.forEach {
val settings = CoderSettings(CoderSettingsState(
disableAutostart = it.disableAutostart,
dataDirectory = tmpdir.resolve("configure-ssh").toString(),
headerCommand = it.headerCommand ?: ""),
sshConfigPath = tmpdir.resolve(it.input + "_to_" + it.output + ".conf"))
Expand All @@ -285,12 +299,12 @@ internal class CoderCLIManagerTest {
.replace("/tmp/coder-gateway/test.coder.invalid/coder-linux-amd64", escape(ccm.localBinaryPath.toString()))

// Add workspaces.
ccm.configSsh(it.workspaces)
ccm.configSsh(it.workspaces, it.version)

assertEquals(expectedConf, settings.sshConfigPath.toFile().readText())

// Remove configuration.
ccm.configSsh(emptyList())
ccm.configSsh(emptyList(), it.version)

// Remove is the configuration we expect after removing.
assertEquals(
Expand Down
25 changes: 13 additions & 12 deletions src/test/kotlin/com/coder/gateway/settings/CoderSettingsTest.kt
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package com.coder.gateway.settings

import com.coder.gateway.services.CoderSettingsState
import kotlin.test.Test
import kotlin.test.assertContains
import kotlin.test.assertEquals

import com.coder.gateway.util.OS
import com.coder.gateway.util.getOS
import com.coder.gateway.util.withPath
import java.net.URL
import java.nio.file.Path
import kotlin.test.Test
import kotlin.test.assertContains
import kotlin.test.assertEquals

internal class CoderSettingsTest {
@Test
Expand Down Expand Up @@ -179,14 +178,15 @@ internal class CoderSettingsTest {
// Make sure the remaining settings are being conveyed.
val settings = CoderSettings(
CoderSettingsState(
enableDownloads = false,
enableBinaryDirectoryFallback = true,
headerCommand = "test header",
tlsCertPath = "tls cert path",
tlsKeyPath = "tls key path",
tlsCAPath = "tls ca path",
tlsAlternateHostname = "tls alt hostname",
)
enableDownloads = false,
enableBinaryDirectoryFallback = true,
headerCommand = "test header",
tlsCertPath = "tls cert path",
tlsKeyPath = "tls key path",
tlsCAPath = "tls ca path",
tlsAlternateHostname = "tls alt hostname",
disableAutostart = true,
)
)

assertEquals(false, settings.enableDownloads)
Expand All @@ -196,5 +196,6 @@ internal class CoderSettingsTest {
assertEquals("tls key path", settings.tls.keyPath)
assertEquals("tls ca path", settings.tls.caPath)
assertEquals("tls alt hostname", settings.tls.altHostname)
assertEquals(true, settings.disableAutostart)
}
}