feat: Add deployment side config-ssh options by Emyrk · Pull Request #6613 · coder/coder

Emyrk · 2023-03-15T15:25:31Z

What this does

Allows setting deployment wide config-ssh options.

Bugs fixed

User provided config options did not override defaults if they shared the same key.
Invalid config ssh options were allowed.

Example

Ran coderd with env vars to overwrite a default ConnectTimeout and add a new option. Also changed the hostname prefix.

CODER_SSH_HOSTNAME_PREFIX=steven-local.
CODER_SSH_CONFIG_OPTIONS=ConnectTimeout=20,ConnectionAttempts=5

# ------------START-CODER-----------
# This section is managed by coder. DO NOT EDIT.
#
# You should not hand-edit this section unless you are removing it, all
# changes will be lost when running "coder config-ssh".
#
Host steven-local.test
        HostName steven-local.test
        ConnectTimeout 20
        StrictHostKeyChecking=no
        UserKnownHostsFile=/dev/null
        LogLevel ERROR
        ProxyCommand /home/steven/go/bin/coder --global-config /home/steven/.config/coderv2 ssh --stdio test
        ConnectionAttempts 5
Host steven-local.test.main
        HostName steven-local.test.main
        ConnectTimeout 20
        StrictHostKeyChecking=no
        UserKnownHostsFile=/dev/null
        LogLevel ERROR
        ProxyCommand /home/steven/go/bin/coder --global-config /home/steven/.config/coderv2 ssh --stdio test.main
        ConnectionAttempts 5
# ------------END-CODER------------

Also properly overwrites

Emyrk · 2023-03-15T16:58:20Z

+				// If the error is 404, this deployment does not support
+				// this endpoint yet. Do not error, just assume defaults.
+				// TODO: Remove this in 2 months (May 2023). Just return the error
+				// 	and remove this 404 check.
+				var sdkErr *codersdk.Error
+				if !xerrors.As(err, &sdkErr) {
+					// not an SDK error, return the original error.
+					return xerrors.Errorf("fetch coderd config failed: %w", err)
+				}
+
+				if sdkErr.StatusCode() != http.StatusNotFound {
+					// Not a 404, return the original error.
+					return xerrors.Errorf("fetch coderd config failed: %w", err)
+				}


This is just to support new coder cli against an older coderd. Can drop it after some time.

Meh, I don't think we should do this. Our install script supports installing by version, so it's really easy to upgrade/downgrade.

If we don't care, I can drop it. I think it is mostly something developers will hit.

What error will users see when they do this on an old client?

As long as it's clear to the user that they need to upgrade to resolve it, I don't think we need to have backward compatibility.

Let's be sure to label this as a breaking change though

Can I just keep this then @kylecarbs? It makes things backwards compatible without docs needed. I included a drop date to remove the code.

It's just a convince to handle any edge cases, and it's not expensive or complex.

Emyrk · 2023-03-15T16:59:18Z

+					// Override with deployment options
+					for k, v := range coderdConfig.SSHConfigOptions {
+						opt := fmt.Sprintf("%s %s", k, v)
+						err := configOptions.addOptions(opt)
+						if err != nil {
+							return xerrors.Errorf("add coderd config option %q: %w", opt, err)
+						}
+					}
+					// Override with flag options
+					for _, opt := range sshConfigOpts.sshOptions {
+						err := configOptions.addOptions(opt)
+						if err != nil {
+							return xerrors.Errorf("add flag config option %q: %w", opt, err)
+						}
+					}


Default > Deployment > Flags

So the user can always override any values by using -o flag.

kylecarbs

LGTM. Mostly small naming things. Just ping and I'll approve!

Emyrk · 2023-03-15T20:10:21Z

The CODER_ prefix for env vars is included.
See this constant: https://github.com/coder/coder/blob/stevenmasley%2Fssh_config/cli/usage.go#L26-L26

And usage:
https://github.com/coder/coder/blob/stevenmasley%2Fssh_config/cli/server.go#L188-L188

kylecarbs

Prob worth just doing ctrl+f CLISSH and replacing with SSH since this won't only apply to the CLI

mafredri

LGTM!

mafredri · 2023-03-16T17:29:39Z

+Override the default host prefix.
+<br/>
+| | |
+| --- | --- |


Hmm, is this a bug in gen?

Uh,I guess if the default is "", it doesn't put anything?

Emyrk added 2 commits March 14, 2023 18:03

feat: Allow setting deployment wide ssh config settings

79535d0

feat: config-ssh respects deployment ssh config

f0eb123

Also properly overwrites

github-actions Bot assigned Emyrk Mar 15, 2023

Emyrk added 9 commits March 15, 2023 10:32

Catch early parse error

4beeb62

Add to unit test

08f5d3d

fix typo

c8c5189

Add unit test

96ad4bc

Fix output

f9f4a8f

Make gen

ebf9eb9

Simplify if/else

119695b

Fix AutorizeAllEndpoints

b8f3242

Fic swager docs

b082a5a

Emyrk commented Mar 15, 2023

View reviewed changes

Emyrk marked this pull request as ready for review March 15, 2023 17:59

Make gen

4a1e3c2

Emyrk requested review from a team and mafredri and removed request for a team March 15, 2023 18:01

kylecarbs reviewed Mar 15, 2023

View reviewed changes

Comment thread codersdk/deployment.go Outdated

Comment thread codersdk/deployment.go Outdated

Comment thread codersdk/deployment.go Outdated

Comment thread coderd/coderd.go Outdated

Comment thread coderd/coderd.go Outdated

Emyrk added 3 commits March 15, 2023 15:04

The '.' is now configurable

01ea08f

CODER env prefix is automatic

7074f50

Renames

952c591

Emyrk added 2 commits March 15, 2023 15:12

Make gen

dae091a

Fix AutorizeAllEndpoints

a1dd7d4

kylecarbs reviewed Mar 15, 2023

View reviewed changes

Comment thread coderd/clissh.go Outdated

Comment thread codersdk/deployment.go Outdated

Comment thread codersdk/deployment.go Outdated

Emyrk added 2 commits March 15, 2023 15:31

Rename to drop 'CLI'

4f42634

Prefix requires .

c218edd

Emyrk added 2 commits March 15, 2023 15:32

Use constant in test

a752fc8

Linting

d328d97

Emyrk requested a review from kylecarbs March 15, 2023 21:15

Formatting

a4b9620

mafredri reviewed Mar 16, 2023

View reviewed changes

Emyrk added 12 commits March 16, 2023 10:09

Allow the user to override the host prefix

78fbda8

Fix doc messages

d28b850

Make gen

617d987

Fix comment

eb4bb7b

Remove "CLI" part of naming

123ce02

Update golden files

ca41cce

Fix 404 logic

efcbc29

remove 1 error check

3c1c87f

Move buildinfo into deployment.go

4586b11

fixup! Move buildinfo into deployment.go

0f2ef97

make gen

a5aac50

Golden files

a3254cd

mafredri approved these changes Mar 16, 2023

View reviewed changes

Emyrk merged commit fe247c8 into main Mar 16, 2023

Emyrk deleted the stevenmasley/ssh_config branch March 16, 2023 18:03

github-actions Bot locked and limited conversation to collaborators Mar 16, 2023

Conversation

Emyrk commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this does

Bugs fixed

Example

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpmct Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Emyrk commented Mar 15, 2023

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Emyrk commented Mar 15, 2023 •

edited

Loading

bpmct Mar 16, 2023 •

edited

Loading