Skip to content

Commit 9ae8650

Browse files
ammarioammario
committed
Merge remote-tracking branch 'origin/main' into agent-metadata
2 parents fdce29f + 3b52d4f commit 9ae8650

File tree

146 files changed

+3284
-1565
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

146 files changed

+3284
-1565
lines changed

‎.github/workflows/ci.yaml

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -254,14 +254,11 @@ jobs:
254254
with:
255255
go-version: "~1.20"
256256

257-
# Sadly the new "set output" syntax (of writing env vars to
258-
# $GITHUB_OUTPUT) does not work on both powershell and bash so we use the
259-
# deprecated syntax here.
260257
- name: Echo Go Cache Paths
261258
id: go-cache-paths
262259
run: |
263-
echo "::set-output name=GOCACHE::$(go env GOCACHE)"
264-
echo "::set-output name=GOMODCACHE::$(go env GOMODCACHE)"
260+
echo "GOCACHE=$(go env GOCACHE)" >> ${{ runner.os == 'Windows' && '$env:' || '$' }}GITHUB_OUTPUT
261+
echo "GOMODCACHE=$(go env GOMODCACHE)" >> ${{ runner.os == 'Windows' && '$env:' || '$' }}GITHUB_OUTPUT
265262
266263
- name: Go Build Cache
267264
uses: actions/cache@v3

‎.vscode/settings.json

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,6 @@
9090
"pqtype",
9191
"prometheusmetrics",
9292
"promhttp",
93-
"promptui",
9493
"protobuf",
9594
"provisionerd",
9695
"provisionerdserver",

‎agent/agent.go

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1275,7 +1275,8 @@ func (a *agent) handleSSHSession(session ssh.Session) (retErr error) {
12751275
go func() {
12761276
for win := range windowSize {
12771277
resizeErr := ptty.Resize(uint16(win.Height), uint16(win.Width))
1278-
if resizeErr != nil {
1278+
// If the pty is closed, then command has exited, no need to log.
1279+
if resizeErr != nil && !errors.Is(resizeErr, pty.ErrClosed) {
12791280
a.logger.Warn(ctx, "failed to resize tty", slog.Error(resizeErr))
12801281
}
12811282
}
@@ -1291,19 +1292,32 @@ func (a *agent) handleSSHSession(session ssh.Session) (retErr error) {
12911292
// output being lost. To avoid this, we wait for the output copy to
12921293
// start before waiting for the command to exit. This ensures that the
12931294
// output copy goroutine will be scheduled before calling close on the
1294-
// pty. There is still a risk of data loss if a command produces a lot
1295-
// of output, see TestAgent_Session_TTY_HugeOutputIsNotLost (skipped).
1295+
// pty. This shouldn't be needed because of `pty.Dup()` below, but it
1296+
// may not be supported on all platforms.
12961297
outputCopyStarted := make(chan struct{})
1297-
ptyOutput := func() io.Reader {
1298+
ptyOutput := func() io.ReadCloser {
12981299
defer close(outputCopyStarted)
1299-
return ptty.Output()
1300+
// Try to dup so we can separate stdin and stdout closure.
1301+
// Once the original pty is closed, the dup will return
1302+
// input/output error once the buffered data has been read.
1303+
stdout, err := ptty.Dup()
1304+
if err == nil {
1305+
return stdout
1306+
}
1307+
// If we can't dup, we shouldn't close
1308+
// the fd since it's tied to stdin.
1309+
return readNopCloser{ptty.Output()}
13001310
}
13011311
wg.Add(1)
13021312
go func() {
13031313
// Ensure data is flushed to session on command exit, if we
13041314
// close the session too soon, we might lose data.
13051315
defer wg.Done()
1306-
_, _ = io.Copy(session, ptyOutput())
1316+
1317+
stdout := ptyOutput()
1318+
defer stdout.Close()
1319+
1320+
_, _ = io.Copy(session, stdout)
13071321
}()
13081322
<-outputCopyStarted
13091323

@@ -1336,6 +1350,11 @@ func (a *agent) handleSSHSession(session ssh.Session) (retErr error) {
13361350
return cmd.Wait()
13371351
}
13381352

1353+
type readNopCloser struct{ io.Reader }
1354+
1355+
// Close implements io.Closer.
1356+
func (readNopCloser) Close() error { return nil }
1357+
13391358
func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, msg codersdk.WorkspaceAgentReconnectingPTYInit, conn net.Conn) (retErr error) {
13401359
defer conn.Close()
13411360

‎agent/agent_test.go

Lines changed: 14 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -351,15 +351,8 @@ func TestAgent_Session_TTY_Hushlogin(t *testing.T) {
351351

352352
func TestAgent_Session_TTY_FastCommandHasOutput(t *testing.T) {
353353
t.Parallel()
354-
if runtime.GOOS == "windows" {
355-
// This might be our implementation, or ConPTY itself.
356-
// It's difficult to find extensive tests for it, so
357-
// it seems like it could be either.
358-
t.Skip("ConPTY appears to be inconsistent on Windows.")
359-
}
360-
361354
// This test is here to prevent regressions where quickly executing
362-
// commands (with TTY) don't flush their output to the SSH session.
355+
// commands (with TTY) don't sync their output to the SSH session.
363356
//
364357
// See: https://github.com/coder/coder/issues/6656
365358
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
@@ -405,20 +398,13 @@ func TestAgent_Session_TTY_FastCommandHasOutput(t *testing.T) {
405398

406399
func TestAgent_Session_TTY_HugeOutputIsNotLost(t *testing.T) {
407400
t.Parallel()
408-
if runtime.GOOS == "windows" {
409-
// This might be our implementation, or ConPTY itself.
410-
// It's difficult to find extensive tests for it, so
411-
// it seems like it could be either.
412-
t.Skip("ConPTY appears to be inconsistent on Windows.")
413-
}
414-
t.Skip("This test proves we have a bug where parts of large output on a PTY can be lost after the command exits, skipped to avoid test failures.")
415-
416-
// This test is here to prevent prove we have a bug where quickly executing
417-
// commands (with TTY) don't flush their output to the SSH session. This is
418-
// due to the pty being closed before all the output has been copied, but
419-
// protecting against this requires a non-trivial rewrite of the output
420-
// processing (or figuring out a way to put the pty in a mode where this
421-
// does not happen).
401+
402+
// This test is here to prevent regressions where a command (with or
403+
// without) a large amount of output would not be fully copied to the
404+
// SSH session. On unix systems, this was fixed by duplicating the file
405+
// descriptor of the PTY master and using it for copying the output.
406+
//
407+
// See: https://github.com/coder/coder/issues/6656
422408
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
423409
defer cancel()
424410
//nolint:dogsled
@@ -688,13 +674,14 @@ func TestAgent_UnixRemoteForwarding(t *testing.T) {
688674
err = cmd.Start()
689675
require.NoError(t, err)
690676

677+
// It's possible that the socket is created but the server is not ready to
678+
// accept connections yet. We need to retry until we can connect.
679+
var conn net.Conn
691680
require.Eventually(t, func() bool {
692-
_, err := os.Stat(remoteSocketPath)
681+
var err error
682+
conn, err = net.Dial("unix", remoteSocketPath)
693683
return err == nil
694-
}, testutil.WaitLong, testutil.IntervalFast)
695-
696-
conn, err := net.Dial("unix", remoteSocketPath)
697-
require.NoError(t, err)
684+
}, testutil.WaitShort, testutil.IntervalFast)
698685
defer conn.Close()
699686
_, err = conn.Write([]byte("test"))
700687
require.NoError(t, err)

‎cli/clibase/values.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -190,19 +190,6 @@ func (d *Duration) String() string {
190190
return time.Duration(*d).String()
191191
}
192192

193-
func (d *Duration) MarshalJSON() ([]byte, error) {
194-
return json.Marshal(d.String())
195-
}
196-
197-
func (d *Duration) UnmarshalJSON(b []byte) error {
198-
var s string
199-
err := json.Unmarshal(b, &s)
200-
if err != nil {
201-
return err
202-
}
203-
return d.Set(s)
204-
}
205-
206193
func (Duration) Type() string {
207194
return "duration"
208195
}

‎cli/cliui/log.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ func (m cliMessage) String() string {
3535
// Warn writes a log to the writer provided.
3636
func Warn(wtr io.Writer, header string, lines ...string) {
3737
_, _ = fmt.Fprint(wtr, cliMessage{
38-
Style: Styles.Warn,
38+
Style: Styles.Warn.Copy(),
3939
Prefix: "WARN: ",
4040
Header: header,
4141
Lines: lines,
@@ -63,7 +63,7 @@ func Infof(wtr io.Writer, fmtStr string, args ...interface{}) {
6363
// Error writes a log to the writer provided.
6464
func Error(wtr io.Writer, header string, lines ...string) {
6565
_, _ = fmt.Fprint(wtr, cliMessage{
66-
Style: Styles.Error,
66+
Style: Styles.Error.Copy(),
6767
Prefix: "ERROR: ",
6868
Header: header,
6969
Lines: lines,

‎cli/server.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,8 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
726726
EmailDomain: cfg.OIDC.EmailDomain,
727727
AllowSignups: cfg.OIDC.AllowSignups.Value(),
728728
UsernameField: cfg.OIDC.UsernameField.String(),
729+
EmailField: cfg.OIDC.EmailField.String(),
730+
AuthURLParams: cfg.OIDC.AuthURLParams.Value,
729731
GroupField: cfg.OIDC.GroupField.String(),
730732
GroupMapping: cfg.OIDC.GroupMapping.Value,
731733
SignInText: cfg.OIDC.SignInText.String(),
@@ -1390,6 +1392,7 @@ func generateSelfSignedCertificate() (*tls.Certificate, error) {
13901392
func configureTLS(tlsMinVersion, tlsClientAuth string, tlsCertFiles, tlsKeyFiles []string, tlsClientCAFile string) (*tls.Config, error) {
13911393
tlsConfig := &tls.Config{
13921394
MinVersion: tls.VersionTLS12,
1395+
NextProtos: []string{"h2", "http/1.1"},
13931396
}
13941397
switch tlsMinVersion {
13951398
case "tls10":
@@ -1679,6 +1682,7 @@ func configureHTTPClient(ctx context.Context, clientCertFile, clientKeyFile stri
16791682

16801683
tlsClientConfig := &tls.Config{ //nolint:gosec
16811684
Certificates: certificates,
1685+
NextProtos: []string{"h2", "http/1.1"},
16821686
}
16831687
err = configureCAPool(tlsClientCAFile, tlsClientConfig)
16841688
if err != nil {

‎cli/server_test.go

Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ import (
3737
"github.com/coder/coder/coderd/database/postgres"
3838
"github.com/coder/coder/coderd/telemetry"
3939
"github.com/coder/coder/codersdk"
40+
"github.com/coder/coder/cryptorand"
4041
"github.com/coder/coder/pty/ptytest"
4142
"github.com/coder/coder/testutil"
4243
)
@@ -1016,6 +1017,166 @@ func TestServer(t *testing.T) {
10161017
require.True(t, strings.HasPrefix(fakeURL.String(), fakeRedirect), fakeURL.String())
10171018
})
10181019

1020+
t.Run("OIDC", func(t *testing.T) {
1021+
t.Parallel()
1022+
1023+
t.Run("Defaults", func(t *testing.T) {
1024+
t.Parallel()
1025+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
1026+
defer cancel()
1027+
1028+
// Startup a fake server that just responds to .well-known/openid-configuration
1029+
// This is just needed to get Coder to start up.
1030+
oidcServer := httptest.NewServer(nil)
1031+
fakeWellKnownHandler := func(w http.ResponseWriter, r *http.Request) {
1032+
w.Header().Set("Content-Type", "application/json")
1033+
payload := fmt.Sprintf("{\"issuer\": %q}", oidcServer.URL)
1034+
_, _ = w.Write([]byte(payload))
1035+
}
1036+
oidcServer.Config.Handler = http.HandlerFunc(fakeWellKnownHandler)
1037+
t.Cleanup(oidcServer.Close)
1038+
1039+
inv, cfg := clitest.New(t,
1040+
"server",
1041+
"--in-memory",
1042+
"--http-address", ":0",
1043+
"--access-url", "http://example.com",
1044+
"--oidc-client-id", "fake",
1045+
"--oidc-client-secret", "fake",
1046+
"--oidc-issuer-url", oidcServer.URL,
1047+
// Leaving the rest of the flags as defaults.
1048+
)
1049+
1050+
// Ensure that the server starts up without error.
1051+
clitest.Start(t, inv)
1052+
accessURL := waitAccessURL(t, cfg)
1053+
client := codersdk.New(accessURL)
1054+
1055+
randPassword, err := cryptorand.String(24)
1056+
require.NoError(t, err)
1057+
1058+
_, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{
1059+
Email: "admin@coder.com",
1060+
Password: randPassword,
1061+
Username: "admin",
1062+
Trial: true,
1063+
})
1064+
require.NoError(t, err)
1065+
1066+
loginResp, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
1067+
Email: "admin@coder.com",
1068+
Password: randPassword,
1069+
})
1070+
require.NoError(t, err)
1071+
client.SetSessionToken(loginResp.SessionToken)
1072+
1073+
deploymentConfig, err := client.DeploymentConfig(ctx)
1074+
require.NoError(t, err)
1075+
1076+
// Ensure that the OIDC provider is configured correctly.
1077+
require.Equal(t, "fake", deploymentConfig.Values.OIDC.ClientID.Value())
1078+
// The client secret is not returned from the API.
1079+
require.Empty(t, deploymentConfig.Values.OIDC.ClientSecret.Value())
1080+
require.Equal(t, oidcServer.URL, deploymentConfig.Values.OIDC.IssuerURL.Value())
1081+
// These are the default values returned from the API. See codersdk/deployment.go for the default values.
1082+
require.True(t, deploymentConfig.Values.OIDC.AllowSignups.Value())
1083+
require.Empty(t, deploymentConfig.Values.OIDC.EmailDomain.Value())
1084+
require.Equal(t, []string{"openid", "profile", "email"}, deploymentConfig.Values.OIDC.Scopes.Value())
1085+
require.False(t, deploymentConfig.Values.OIDC.IgnoreEmailVerified.Value())
1086+
require.Equal(t, "preferred_username", deploymentConfig.Values.OIDC.UsernameField.Value())
1087+
require.Equal(t, "email", deploymentConfig.Values.OIDC.EmailField.Value())
1088+
require.Equal(t, map[string]string{"access_type": "offline"}, deploymentConfig.Values.OIDC.AuthURLParams.Value)
1089+
require.Empty(t, deploymentConfig.Values.OIDC.GroupField.Value())
1090+
require.Empty(t, deploymentConfig.Values.OIDC.GroupMapping.Value)
1091+
require.Equal(t, "OpenID Connect", deploymentConfig.Values.OIDC.SignInText.Value())
1092+
require.Empty(t, deploymentConfig.Values.OIDC.IconURL.Value())
1093+
})
1094+
1095+
t.Run("Overrides", func(t *testing.T) {
1096+
t.Parallel()
1097+
1098+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitMedium)
1099+
defer cancel()
1100+
1101+
// Startup a fake server that just responds to .well-known/openid-configuration
1102+
// This is just needed to get Coder to start up.
1103+
oidcServer := httptest.NewServer(nil)
1104+
fakeWellKnownHandler := func(w http.ResponseWriter, r *http.Request) {
1105+
w.Header().Set("Content-Type", "application/json")
1106+
payload := fmt.Sprintf("{\"issuer\": %q}", oidcServer.URL)
1107+
_, _ = w.Write([]byte(payload))
1108+
}
1109+
oidcServer.Config.Handler = http.HandlerFunc(fakeWellKnownHandler)
1110+
t.Cleanup(oidcServer.Close)
1111+
1112+
inv, cfg := clitest.New(t,
1113+
"server",
1114+
"--in-memory",
1115+
"--http-address", ":0",
1116+
"--access-url", "http://example.com",
1117+
"--oidc-client-id", "fake",
1118+
"--oidc-client-secret", "fake",
1119+
"--oidc-issuer-url", oidcServer.URL,
1120+
// The following values have defaults that we want to override.
1121+
"--oidc-allow-signups=false",
1122+
"--oidc-email-domain", "example.com",
1123+
"--oidc-scopes", "360noscope",
1124+
"--oidc-ignore-email-verified",
1125+
"--oidc-username-field", "not_preferred_username",
1126+
"--oidc-email-field", "not_email",
1127+
"--oidc-auth-url-params", `{"prompt":"consent"}`,
1128+
"--oidc-group-field", "serious_business_unit",
1129+
"--oidc-group-mapping", `{"serious_business_unit": "serious_business_unit"}`,
1130+
"--oidc-sign-in-text", "Sign In With Coder",
1131+
"--oidc-icon-url", "https://example.com/icon.png",
1132+
)
1133+
1134+
// Ensure that the server starts up without error.
1135+
clitest.Start(t, inv)
1136+
accessURL := waitAccessURL(t, cfg)
1137+
client := codersdk.New(accessURL)
1138+
1139+
randPassword, err := cryptorand.String(24)
1140+
require.NoError(t, err)
1141+
1142+
_, err = client.CreateFirstUser(ctx, codersdk.CreateFirstUserRequest{
1143+
Email: "admin@coder.com",
1144+
Password: randPassword,
1145+
Username: "admin",
1146+
Trial: true,
1147+
})
1148+
require.NoError(t, err)
1149+
1150+
loginResp, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{
1151+
Email: "admin@coder.com",
1152+
Password: randPassword,
1153+
})
1154+
require.NoError(t, err)
1155+
client.SetSessionToken(loginResp.SessionToken)
1156+
1157+
deploymentConfig, err := client.DeploymentConfig(ctx)
1158+
require.NoError(t, err)
1159+
1160+
// Ensure that the OIDC provider is configured correctly.
1161+
require.Equal(t, "fake", deploymentConfig.Values.OIDC.ClientID.Value())
1162+
// The client secret is not returned from the API.
1163+
require.Empty(t, deploymentConfig.Values.OIDC.ClientSecret.Value())
1164+
require.Equal(t, oidcServer.URL, deploymentConfig.Values.OIDC.IssuerURL.Value())
1165+
// These are values that we want to make sure were overridden.
1166+
require.False(t, deploymentConfig.Values.OIDC.AllowSignups.Value())
1167+
require.Equal(t, []string{"example.com"}, deploymentConfig.Values.OIDC.EmailDomain.Value())
1168+
require.Equal(t, []string{"360noscope"}, deploymentConfig.Values.OIDC.Scopes.Value())
1169+
require.True(t, deploymentConfig.Values.OIDC.IgnoreEmailVerified.Value())
1170+
require.Equal(t, "not_preferred_username", deploymentConfig.Values.OIDC.UsernameField.Value())
1171+
require.Equal(t, "not_email", deploymentConfig.Values.OIDC.EmailField.Value())
1172+
require.Equal(t, map[string]string{"access_type": "offline", "prompt": "consent"}, deploymentConfig.Values.OIDC.AuthURLParams.Value)
1173+
require.Equal(t, "serious_business_unit", deploymentConfig.Values.OIDC.GroupField.Value())
1174+
require.Equal(t, map[string]string{"serious_business_unit": "serious_business_unit"}, deploymentConfig.Values.OIDC.GroupMapping.Value)
1175+
require.Equal(t, "Sign In With Coder", deploymentConfig.Values.OIDC.SignInText.Value())
1176+
require.Equal(t, "https://example.com/icon.png", deploymentConfig.Values.OIDC.IconURL.Value().String())
1177+
})
1178+
})
1179+
10191180
t.Run("RateLimit", func(t *testing.T) {
10201181
t.Parallel()
10211182

0 commit comments

Comments
 (0)