Skip to content

Commit 5132db6

Browse files
ashmckenzieashmckenzie
committed
Merge branch 'ashmckenzie/log-metadata-refactor' into 'main'
Log metadata refactor See merge request https://gitlab.com/gitlab-org/gitlab-shell/-/merge_requests/832 Merged-by: Ash McKenzie <[email protected]> Approved-by: Alejandro Rodríguez <[email protected]> Approved-by: Igor Drozdov <[email protected]>
2 parents 91fa231 + 9be2752 commit 5132db6

15 files changed

+90
-79
lines changed

‎internal/command/command.go

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,15 @@ type Command interface {
1717
}
1818

1919
type LogMetadata struct {
20-
Username string `json:"username"`
2120
Project string `json:"project,omitempty"`
2221
RootNamespace string `json:"root_namespace,omitempty"`
2322
}
2423

24+
type LogData struct {
25+
Username string `json:"username"`
26+
Meta LogMetadata `json:"meta"`
27+
}
28+
2529
func CheckForVersionFlag(osArgs []string, version, buildTime string) {
2630
// We can't use the flag library because gitlab-shell receives other arguments
2731
// that confuse the parser.
@@ -69,7 +73,7 @@ func Setup(serviceName string, config *config.Config) (context.Context, func())
6973
}
7074
}
7175

72-
func NewLogMetadata(project, username string) LogMetadata {
76+
func NewLogData(project, username string) LogData {
7377
rootNameSpace := ""
7478

7579
if len(project) > 0 {
@@ -82,9 +86,11 @@ func NewLogMetadata(project, username string) LogMetadata {
8286
}
8387
}
8488

85-
return LogMetadata{
86-
Username: username,
87-
Project: project,
88-
RootNamespace: rootNameSpace,
89+
return LogData{
90+
Username: username,
91+
Meta: LogMetadata{
92+
Project: project,
93+
RootNamespace: rootNameSpace,
94+
},
8995
}
9096
}

‎internal/command/command_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ func addAdditionalEnv(envMap map[string]string) func() {
7878
}
7979
}
8080

81-
func TestNewLogMetadata(t *testing.T) {
81+
func TestNewLogData(t *testing.T) {
8282
testCases := []struct {
8383
desc string
8484
project string
@@ -107,10 +107,10 @@ func TestNewLogMetadata(t *testing.T) {
107107

108108
for _, tc := range testCases {
109109
t.Run(tc.desc, func(t *testing.T) {
110-
metadata := NewLogMetadata(tc.project, tc.username)
111-
require.Equal(t, tc.project, metadata.Project)
112-
require.Equal(t, tc.username, metadata.Username)
113-
require.Equal(t, tc.expectedRootNamespace, metadata.RootNamespace)
110+
data := NewLogData(tc.project, tc.username)
111+
require.Equal(t, tc.username, data.Username)
112+
require.Equal(t, tc.project, data.Meta.Project)
113+
require.Equal(t, tc.expectedRootNamespace, data.Meta.RootNamespace)
114114
})
115115
}
116116
}

‎internal/command/discover/discover.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,18 +23,18 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
2323
return ctx, fmt.Errorf("Failed to get username: %v", err)
2424
}
2525

26-
metadata := command.LogMetadata{}
26+
logData := command.LogData{}
2727
if response.IsAnonymous() {
28-
metadata.Username = "Anonymous"
28+
logData.Username = "Anonymous"
2929
fmt.Fprintf(c.ReadWriter.Out, "Welcome to GitLab, Anonymous!\n")
3030
} else {
31-
metadata.Username = response.Username
31+
logData.Username = response.Username
3232
fmt.Fprintf(c.ReadWriter.Out, "Welcome to GitLab, @%s!\n", response.Username)
3333
}
3434

35-
ctxWithLogMetadata := context.WithValue(ctx, "metadata", metadata)
35+
ctxWithLogData := context.WithValue(ctx, "logData", logData)
3636

37-
return ctxWithLogMetadata, nil
37+
return ctxWithLogData, nil
3838
}
3939

4040
func (c *Command) getUserInfo(ctx context.Context) (*discover.Response, error) {

‎internal/command/discover/discover_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,14 +83,14 @@ func TestExecute(t *testing.T) {
8383
ReadWriter: &readwriter.ReadWriter{Out: buffer},
8484
}
8585

86-
ctxWithLogMetadata, err := cmd.Execute(context.Background())
86+
ctxWithLogData, err := cmd.Execute(context.Background())
8787

8888
expectedOutput := fmt.Sprintf("Welcome to GitLab, %s!\n", tc.expectedUsername)
8989
expectedUsername := strings.TrimLeft(tc.expectedUsername, "@")
9090

9191
require.NoError(t, err)
9292
require.Equal(t, expectedOutput, buffer.String())
93-
require.Equal(t, expectedUsername, ctxWithLogMetadata.Value("metadata").(command.LogMetadata).Username)
93+
require.Equal(t, expectedUsername, ctxWithLogData.Value("logData").(command.LogData).Username)
9494
})
9595
}
9696
}

‎internal/command/lfsauthenticate/lfsauthenticate.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,11 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
5858
return ctx, err
5959
}
6060

61-
metadata := command.NewLogMetadata(
61+
logData := command.NewLogData(
6262
accessResponse.Gitaly.Repo.GlProjectPath,
6363
accessResponse.Username,
6464
)
65-
ctxWithLogMetadata := context.WithValue(ctx, "metadata", metadata)
65+
ctxWithLogData := context.WithValue(ctx, "logData", logData)
6666

6767
payload, err := c.authenticate(ctx, operation, repo, accessResponse.UserId)
6868
if err != nil {
@@ -72,12 +72,12 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
7272
log.Fields{"operation": operation, "repo": repo, "user_id": accessResponse.UserId},
7373
).WithError(err).Debug("lfsauthenticate: execute: LFS authentication failed")
7474

75-
return ctxWithLogMetadata, nil
75+
return ctxWithLogData, nil
7676
}
7777

7878
fmt.Fprintf(c.ReadWriter.Out, "%s\n", payload)
7979

80-
return ctxWithLogMetadata, nil
80+
return ctxWithLogData, nil
8181
}
8282

8383
func actionFromOperation(operation string) (commandargs.CommandType, error) {

‎internal/command/lfsauthenticate/lfsauthenticate_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -152,15 +152,15 @@ func TestLfsAuthenticateRequests(t *testing.T) {
152152
ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output},
153153
}
154154

155-
ctxWithLogMetadata, err := cmd.Execute(context.Background())
155+
ctxWithLogData, err := cmd.Execute(context.Background())
156156

157157
require.NoError(t, err)
158158
require.Equal(t, tc.expectedOutput, output.String())
159159

160-
metadata := ctxWithLogMetadata.Value("metadata").(command.LogMetadata)
161-
require.Equal(t, "alex-doe", metadata.Username)
162-
require.Equal(t, "group/project-path", metadata.Project)
163-
require.Equal(t, "group", metadata.RootNamespace)
160+
data := ctxWithLogData.Value("logData").(command.LogData)
161+
require.Equal(t, "alex-doe", data.Username)
162+
require.Equal(t, "group/project-path", data.Meta.Project)
163+
require.Equal(t, "group", data.Meta.RootNamespace)
164164
})
165165
}
166166
}

‎internal/command/receivepack/receivepack.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
3131
return ctx, err
3232
}
3333

34-
ctxWithLogMetadata := context.WithValue(ctx, "metadata", command.NewLogMetadata(
34+
ctxWithLogData := context.WithValue(ctx, "logData", command.NewLogData(
3535
response.Gitaly.Repo.GlProjectPath,
3636
response.Username,
3737
))
@@ -48,18 +48,18 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
4848
Response: response,
4949
}
5050

51-
return ctxWithLogMetadata, cmd.Execute(ctx)
51+
return ctxWithLogData, cmd.Execute(ctx)
5252
}
5353

5454
customAction := customaction.Command{
5555
Config: c.Config,
5656
ReadWriter: c.ReadWriter,
5757
EOFSent: true,
5858
}
59-
return ctxWithLogMetadata, customAction.Execute(ctx, response)
59+
return ctxWithLogData, customAction.Execute(ctx, response)
6060
}
6161

62-
return ctxWithLogMetadata, c.performGitalyCall(ctx, response)
62+
return ctxWithLogData, c.performGitalyCall(ctx, response)
6363
}
6464

6565
func (c *Command) verifyAccess(ctx context.Context, repo string) (*accessverifier.Response, error) {

‎internal/command/receivepack/receivepack_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ func TestAllowedAccess(t *testing.T) {
2121
cmd, _ := setup(t, "1", requests)
2222
cmd.Config.GitalyClient.InitSidechannelRegistry(context.Background())
2323

24-
ctxWithLogMetadata, err := cmd.Execute(context.Background())
24+
ctxWithLogData, err := cmd.Execute(context.Background())
2525

2626
require.NoError(t, err)
27-
metadata := ctxWithLogMetadata.Value("metadata").(command.LogMetadata)
28-
require.Equal(t, "alex-doe", metadata.Username)
29-
require.Equal(t, "group/project-path", metadata.Project)
30-
require.Equal(t, "group", metadata.RootNamespace)
27+
data := ctxWithLogData.Value("logData").(command.LogData)
28+
require.Equal(t, "alex-doe", data.Username)
29+
require.Equal(t, "group/project-path", data.Meta.Project)
30+
require.Equal(t, "group", data.Meta.RootNamespace)
3131
}
3232

3333
func TestForbiddenAccess(t *testing.T) {

‎internal/command/uploadarchive/uploadarchive.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
2929
return ctx, err
3030
}
3131

32-
metadata := command.NewLogMetadata(
32+
logData := command.NewLogData(
3333
response.Gitaly.Repo.GlProjectPath,
3434
response.Username,
3535
)
36-
ctxWithLogMetadata := context.WithValue(ctx, "metadata", metadata)
36+
ctxWithLogData := context.WithValue(ctx, "logData", logData)
3737

38-
return ctxWithLogMetadata, c.performGitalyCall(ctx, response)
38+
return ctxWithLogData, c.performGitalyCall(ctx, response)
3939
}
4040

4141
func (c *Command) verifyAccess(ctx context.Context, repo string) (*accessverifier.Response, error) {

‎internal/command/uploadarchive/uploadarchive_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ func TestAllowedAccess(t *testing.T) {
2121
cmd, _ := setup(t, "1", requests)
2222
cmd.Config.GitalyClient.InitSidechannelRegistry(context.Background())
2323

24-
ctxWithLogMetadata, err := cmd.Execute(context.Background())
24+
ctxWithLogData, err := cmd.Execute(context.Background())
2525

2626
require.NoError(t, err)
27-
metadata := ctxWithLogMetadata.Value("metadata").(command.LogMetadata)
28-
require.Equal(t, "alex-doe", metadata.Username)
29-
require.Equal(t, "group/project-path", metadata.Project)
30-
require.Equal(t, "group", metadata.RootNamespace)
27+
data := ctxWithLogData.Value("logData").(command.LogData)
28+
require.Equal(t, "alex-doe", data.Username)
29+
require.Equal(t, "group/project-path", data.Meta.Project)
30+
require.Equal(t, "group", data.Meta.RootNamespace)
3131
}
3232

3333
func TestForbiddenAccess(t *testing.T) {

‎internal/command/uploadpack/uploadpack.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,22 +30,22 @@ func (c *Command) Execute(ctx context.Context) (context.Context, error) {
3030
return ctx, err
3131
}
3232

33-
metadata := command.NewLogMetadata(
33+
logData := command.NewLogData(
3434
response.Gitaly.Repo.GlProjectPath,
3535
response.Username,
3636
)
37-
ctxWithLogMetadata := context.WithValue(ctx, "metadata", metadata)
37+
ctxWithLogData := context.WithValue(ctx, "logData", logData)
3838

3939
if response.IsCustomAction() {
4040
customAction := customaction.Command{
4141
Config: c.Config,
4242
ReadWriter: c.ReadWriter,
4343
EOFSent: false,
4444
}
45-
return ctxWithLogMetadata, customAction.Execute(ctx, response)
45+
return ctxWithLogData, customAction.Execute(ctx, response)
4646
}
4747

48-
return ctxWithLogMetadata, c.performGitalyCall(ctx, response)
48+
return ctxWithLogData, c.performGitalyCall(ctx, response)
4949
}
5050

5151
func (c *Command) verifyAccess(ctx context.Context, repo string) (*accessverifier.Response, error) {

‎internal/command/uploadpack/uploadpack_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ func TestAllowedAccess(t *testing.T) {
2121
cmd, _ := setup(t, "1", requests)
2222
cmd.Config.GitalyClient.InitSidechannelRegistry(context.Background())
2323

24-
ctxWithLogMetadata, err := cmd.Execute(context.Background())
24+
ctxWithLogData, err := cmd.Execute(context.Background())
2525

2626
require.NoError(t, err)
27-
metadata := ctxWithLogMetadata.Value("metadata").(command.LogMetadata)
28-
require.Equal(t, "alex-doe", metadata.Username)
29-
require.Equal(t, "group/project-path", metadata.Project)
30-
require.Equal(t, "group", metadata.RootNamespace)
27+
data := ctxWithLogData.Value("logData").(command.LogData)
28+
require.Equal(t, "alex-doe", data.Username)
29+
require.Equal(t, "group/project-path", data.Meta.Project)
30+
require.Equal(t, "group", data.Meta.RootNamespace)
3131
}
3232

3333
func TestForbiddenAccess(t *testing.T) {

‎internal/sshd/session.go

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ type exitStatusReq struct {
5252
}
5353

5454
func (s *session) handle(ctx context.Context, requests <-chan *ssh.Request) (context.Context, error) {
55-
ctxWithLogMetadata := ctx
55+
ctxWithLogData := ctx
5656
ctxlog := log.ContextLogger(ctx)
5757

5858
ctxlog.Debug("session: handle: entering request loop")
@@ -73,13 +73,13 @@ func (s *session) handle(ctx context.Context, requests <-chan *ssh.Request) (con
7373
case "exec":
7474
// The command has been executed as `ssh user@host command` or `exec` channel has been used
7575
// in the app implementation
76-
ctxWithLogMetadata, shouldContinue, err = s.handleExec(ctx, req)
76+
ctxWithLogData, shouldContinue, err = s.handleExec(ctx, req)
7777
case "shell":
7878
// The command has been entered into the shell or `shell` channel has been used
7979
// in the app implementation
8080
shouldContinue = false
8181
var status uint32
82-
ctxWithLogMetadata, status, err = s.handleShell(ctx, req)
82+
ctxWithLogData, status, err = s.handleShell(ctx, req)
8383
s.exit(ctx, status)
8484
default:
8585
// Ignore unknown requests but don't terminate the session
@@ -102,7 +102,7 @@ func (s *session) handle(ctx context.Context, requests <-chan *ssh.Request) (con
102102

103103
ctxlog.Debug("session: handle: exiting request loop")
104104

105-
return ctxWithLogMetadata, err
105+
return ctxWithLogData, err
106106
}
107107

108108
func (s *session) handleEnv(ctx context.Context, req *ssh.Request) (bool, error) {
@@ -144,10 +144,10 @@ func (s *session) handleExec(ctx context.Context, req *ssh.Request) (context.Con
144144

145145
s.execCmd = execRequest.Command
146146

147-
ctxWithLogMetadata, status, err := s.handleShell(ctx, req)
148-
s.exit(ctxWithLogMetadata, status)
147+
ctxWithLogData, status, err := s.handleShell(ctx, req)
148+
s.exit(ctxWithLogData, status)
149149

150-
return ctxWithLogMetadata, false, err
150+
return ctxWithLogData, false, err
151151
}
152152

153153
func (s *session) handleShell(ctx context.Context, req *ssh.Request) (context.Context, uint32, error) {
@@ -201,7 +201,7 @@ func (s *session) handleShell(ctx context.Context, req *ssh.Request) (context.Co
201201
}).Info("session: handleShell: executing command")
202202
metrics.SshdSessionEstablishedDuration.Observe(establishSessionDuration)
203203

204-
ctxWithLogMetadata, err := cmd.Execute(ctx)
204+
ctxWithLogData, err := cmd.Execute(ctx)
205205
if err != nil {
206206
grpcStatus := grpcstatus.Convert(err)
207207
if grpcStatus.Code() != grpccodes.Internal {
@@ -213,7 +213,7 @@ func (s *session) handleShell(ctx context.Context, req *ssh.Request) (context.Co
213213

214214
ctxlog.Info("session: handleShell: command executed successfully")
215215

216-
return ctxWithLogMetadata, 0, nil
216+
return ctxWithLogData, 0, nil
217217
}
218218

219219
func (s *session) toStderr(ctx context.Context, format string, args ...interface{}) {

0 commit comments

Comments
 (0)