Skip to content

Commit 7c02e08

Browse files
authored
fix: Use LimaUser method instead of host user name (runfinch#712)
Issue #, if available: runfinch#399 *Description of changes:* In `nerdctl_config_applier.go`, editing `.bashrc` uses a file path based on the host's username. However, if the host's username contains `.` or `@`, lima uses `lima` as the username instead of the host's username. (ref: https://github.com/lima-vm/lima/blob/a8c703bf8b66d213d00542ef68271cd7b73612ef/pkg/osutil/user.go#L114-L119) Therefore, if the host's user name contains `.` or `@`, editing `.bashrc` fails. This PR fixes this so that you can get the actual username from `LimaWrapper.LimaUser()`. *Testing done:* yes - [x] I've reviewed the guidance in CONTRIBUTING.md #### License Acceptance By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --------- Signed-off-by: Wataru Haibara <[email protected]>
1 parent 4d1e6cf commit 7c02e08

File tree

3 files changed

+111
-59
lines changed

3 files changed

+111
-59
lines changed

Diff for: cmd/finch/main.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -90,22 +90,23 @@ var newApp = func(logger flog.Logger, fp path.Finch, fs afero.Fs, fc *config.Fin
9090
fp.QEMUBinDir(),
9191
system.NewStdLib(),
9292
)
93+
lima := wrapper.NewLimaWrapper()
9394
supportBundleBuilder := support.NewBundleBuilder(
9495
logger,
9596
fs,
9697
support.NewBundleConfig(fp, system.NewStdLib().Env("HOME")),
9798
fp,
9899
ecc,
99100
lcc,
100-
wrapper.NewLimaWrapper(),
101+
lima,
101102
)
102103

103104
// append nerdctl commands
104105
allCommands := initializeNerdctlCommands(lcc, ecc, logger, fs, fc)
105106
// append finch specific commands
106107
allCommands = append(allCommands,
107108
newVersionCommand(lcc, logger, stdOut),
108-
virtualMachineCommands(logger, fp, lcc, ecc, fs, fc),
109+
virtualMachineCommands(logger, fp, lcc, ecc, fs, fc, lima),
109110
newSupportBundleCommand(logger, supportBundleBuilder, lcc),
110111
newGenDocsCommand(rootCmd, logger, fs, system.NewStdLib()),
111112
)
@@ -122,6 +123,7 @@ func virtualMachineCommands(
122123
ecc *command.ExecCmdCreator,
123124
fs afero.Fs,
124125
fc *config.Finch,
126+
lima wrapper.LimaWrapper,
125127
) *cobra.Command {
126128
optionalDepGroups := []*dependency.Group{
127129
vmnet.NewDependencyGroup(ecc, lcc, fs, fp, logger),
@@ -133,7 +135,7 @@ func virtualMachineCommands(
133135
logger,
134136
optionalDepGroups,
135137
config.NewLimaApplier(fc, ecc, fs, fp.LimaOverrideConfigPath(), system.NewStdLib()),
136-
config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath(), system.NewStdLib().Env("USER")),
138+
config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath(), system.NewStdLib().Env("USER"), lima),
137139
fp,
138140
fs,
139141
disk.NewUserDataDiskManager(lcc, ecc, &afero.OsFs{}, fp, system.NewStdLib().Env("HOME"), fc),

Diff for: pkg/config/nerdctl_config_applier.go

+48-41
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import (
1515
"github.com/spf13/afero/sftpfs"
1616

1717
"github.com/runfinch/finch/pkg/fssh"
18+
"github.com/runfinch/finch/pkg/lima/wrapper"
1819
)
1920

2021
const (
@@ -27,18 +28,20 @@ type nerdctlConfigApplier struct {
2728
fs afero.Fs
2829
privateKeyPath string
2930
hostUser string
31+
lima wrapper.LimaWrapper
3032
}
3133

3234
var _ NerdctlConfigApplier = (*nerdctlConfigApplier)(nil)
3335

3436
// NewNerdctlApplier creates a new NerdctlConfigApplier that
3537
// applies nerdctl configuration changes by SSHing to the lima VM to update the nerdctl configuration file in it.
36-
func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath, hostUser string) NerdctlConfigApplier {
38+
func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath string, hostUser string, lima wrapper.LimaWrapper) NerdctlConfigApplier {
3739
return &nerdctlConfigApplier{
3840
dialer: dialer,
3941
fs: fs,
4042
privateKeyPath: privateKeyPath,
4143
hostUser: hostUser,
44+
lima: lima,
4245
}
4346
}
4447

@@ -53,45 +56,6 @@ func addLineToBashrc(fs afero.Fs, profileFilePath string, profStr string, cmd st
5356
return profStr, nil
5457
}
5558

56-
// updateEnvironment adds variables to the user's shell's environment. Currently it uses ~/.bashrc because
57-
// Bash is the default shell and Bash will not load ~/.profile if ~/.bash_profile exists (which it does).
58-
// ~/.bash_profile sources ~/.bashrc, so ~/.bashrc is currently the best place to define additional variables.
59-
// The [GNU docs for Bash] explain how these files work together in more details.
60-
// The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to
61-
// ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json).
62-
// The reason that we don't set environment variables inside /root/.bashrc is that the vars inside it are
63-
// not able to be picked up even if we specify `sudo -E`. We have to switch to root user in order to access them, while
64-
// normally we would access the VM as the regular user.
65-
// For more information on the variable, see the registry nerdctl docs.
66-
//
67-
// [GNU docs for Bash]: https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html
68-
//
69-
// [registry nerdctl docs]: https://github.com/containerd/nerdctl/blob/master/docs/registry.md
70-
71-
func updateEnvironment(fs afero.Fs, user string) error {
72-
cmdArr := [4]string{
73-
fmt.Sprintf("export DOCKER_CONFIG=\"/Users/%s/.finch\"", user),
74-
fmt.Sprintf("[ -L /usr/local/bin/docker-credential-ecr-login ] "+
75-
"|| sudo ln -s /Users/%s/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/", user),
76-
fmt.Sprintf("[ -L /root/.aws ] || sudo ln -fs /Users/%s/.aws /root/.aws", user),
77-
}
78-
79-
profileFilePath := fmt.Sprintf("/home/%s.linux/.bashrc", user)
80-
profBuf, err := afero.ReadFile(fs, profileFilePath)
81-
if err != nil {
82-
return fmt.Errorf("failed to read config file: %w", err)
83-
}
84-
profStr := string(profBuf)
85-
for _, element := range cmdArr {
86-
profStr, err = addLineToBashrc(fs, profileFilePath, profStr, element)
87-
if err != nil {
88-
return err
89-
}
90-
}
91-
92-
return nil
93-
}
94-
9559
// updateNerdctlConfig reads from the nerdctl config and updates values.
9660
func updateNerdctlConfig(fs afero.Fs, user string, rootless bool) error {
9761
nerdctlRootlessCfgPath := fmt.Sprintf("/home/%s.linux/.config/nerdctl/nerdctl.toml", user)
@@ -137,6 +101,49 @@ func updateNerdctlConfig(fs afero.Fs, user string, rootless bool) error {
137101
return nil
138102
}
139103

104+
// updateEnvironment adds variables to the user's shell's environment. Currently it uses ~/.bashrc because
105+
// Bash is the default shell and Bash will not load ~/.profile if ~/.bash_profile exists (which it does).
106+
// ~/.bash_profile sources ~/.bashrc, so ~/.bashrc is currently the best place to define additional variables.
107+
// The [GNU docs for Bash] explain how these files work together in more details.
108+
// The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to
109+
// ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json).
110+
// The reason that we don't set environment variables inside /root/.bashrc is that the vars inside it are
111+
// not able to be picked up even if we specify `sudo -E`. We have to switch to root user in order to access them, while
112+
// normally we would access the VM as the regular user.
113+
// For more information on the variable, see the registry nerdctl docs.
114+
//
115+
// [GNU docs for Bash]: https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html
116+
//
117+
// [registry nerdctl docs]: https://github.com/containerd/nerdctl/blob/master/docs/registry.md
118+
func (nca *nerdctlConfigApplier) updateEnvironment(fs afero.Fs) error {
119+
cmdArr := [4]string{
120+
fmt.Sprintf("export DOCKER_CONFIG=\"/Users/%s/.finch\"", nca.hostUser),
121+
fmt.Sprintf("[ -L /usr/local/bin/docker-credential-ecr-login ] "+
122+
"|| sudo ln -s /Users/%s/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/", nca.hostUser),
123+
fmt.Sprintf("[ -L /root/.aws ] || sudo ln -fs /Users/%s/.aws /root/.aws", nca.hostUser),
124+
}
125+
126+
limaUser, err := nca.lima.LimaUser(false)
127+
if err != nil {
128+
return fmt.Errorf("failed to get lima user: %w", err)
129+
}
130+
131+
profileFilePath := fmt.Sprintf("/home/%s.linux/.bashrc", limaUser.Username)
132+
profBuf, err := afero.ReadFile(fs, profileFilePath)
133+
if err != nil {
134+
return fmt.Errorf("failed to read config file: %w", err)
135+
}
136+
profStr := string(profBuf)
137+
for _, element := range cmdArr {
138+
profStr, err = addLineToBashrc(fs, profileFilePath, profStr, element)
139+
if err != nil {
140+
return err
141+
}
142+
}
143+
144+
return nil
145+
}
146+
140147
// Apply gets SSH and SFTP clients and uses them to update the nerdctl config.
141148
func (nca *nerdctlConfigApplier) Apply(remoteAddr string) error {
142149
user := "root"
@@ -162,7 +169,7 @@ func (nca *nerdctlConfigApplier) Apply(remoteAddr string) error {
162169
return fmt.Errorf("failed to update the nerdctl config file: %w", err)
163170
}
164171

165-
if err := updateEnvironment(sftpFs, nca.hostUser); err != nil {
172+
if err := nca.updateEnvironment(sftpFs); err != nil {
166173
return fmt.Errorf("failed to update the user's .profile file: %w", err)
167174
}
168175
return nil

Diff for: pkg/config/nerdctl_config_applier_test.go

+58-15
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"errors"
88
"fmt"
99
"io/fs"
10+
"os/user"
1011
"testing"
1112

1213
"github.com/golang/mock/gomock"
@@ -28,21 +29,26 @@ TZ6coT6ILioXcs0kX17JAAAAI2FsdmFqdXNAODg2NjVhMGJmN2NhLmFudC5hbWF6b24uY2
2829
9tAQI=
2930
-----END OPENSSH PRIVATE KEY-----`
3031

31-
func Test_updateEnvironment(t *testing.T) {
32+
func TestNerdctlConfigApplier_updateEnvironment(t *testing.T) {
3233
t.Parallel()
3334

3435
testCases := []struct {
3536
name string
36-
user string
37-
mockSvc func(t *testing.T, fs afero.Fs)
37+
hostUser string
38+
mockSvc func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper)
3839
postRunCheck func(t *testing.T, fs afero.Fs)
3940
want error
4041
}{
4142
{
42-
name: "happy path",
43-
user: "mock_user",
44-
mockSvc: func(t *testing.T, fs afero.Fs) {
43+
name: "happy path",
44+
hostUser: "mock_user",
45+
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
4546
require.NoError(t, afero.WriteFile(fs, "/home/mock_user.linux/.bashrc", []byte(""), 0o644))
47+
48+
mockUser := &user.User{
49+
Username: "mock_user",
50+
}
51+
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
4652
},
4753
postRunCheck: func(t *testing.T, fs afero.Fs) {
4854
fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
@@ -56,9 +62,9 @@ func Test_updateEnvironment(t *testing.T) {
5662
want: nil,
5763
},
5864
{
59-
name: "happy path, file already exists and already contains expected variables",
60-
user: "mock_user",
61-
mockSvc: func(t *testing.T, fs afero.Fs) {
65+
name: "happy path, file already exists and already contains expected variables",
66+
hostUser: "mock_user",
67+
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
6268
require.NoError(
6369
t,
6470
afero.WriteFile(
@@ -70,6 +76,11 @@ func Test_updateEnvironment(t *testing.T) {
7076
0o644,
7177
),
7278
)
79+
80+
mockUser := &user.User{
81+
Username: "mock_user",
82+
}
83+
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
7384
},
7485
postRunCheck: func(t *testing.T, fs afero.Fs) {
7586
fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
@@ -82,26 +93,57 @@ func Test_updateEnvironment(t *testing.T) {
8293
want: nil,
8394
},
8495
{
85-
name: ".bashrc file doesn't exist",
86-
user: "mock_user",
87-
mockSvc: func(t *testing.T, fs afero.Fs) {},
96+
name: ".bashrc file doesn't exist",
97+
hostUser: "mock_user",
98+
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
99+
mockUser := &user.User{
100+
Username: "mock_user",
101+
}
102+
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
103+
},
88104
postRunCheck: func(t *testing.T, fs afero.Fs) {},
89105
want: fmt.Errorf(
90106
"failed to read config file: %w",
91107
&fs.PathError{Op: "open", Path: "/home/mock_user.linux/.bashrc", Err: errors.New("file does not exist")},
92108
),
93109
},
110+
{
111+
name: "host user is not a valid linux username",
112+
hostUser: "invalid.user",
113+
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
114+
require.NoError(t, afero.WriteFile(fs, "/home/lima.linux/.bashrc", []byte(""), 0o644))
115+
116+
mockUser := &user.User{
117+
Username: "lima",
118+
}
119+
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
120+
},
121+
postRunCheck: func(t *testing.T, fs afero.Fs) {
122+
fileBytes, err := afero.ReadFile(fs, "/home/lima.linux/.bashrc")
123+
require.NoError(t, err)
124+
assert.Equal(t,
125+
[]byte("\nexport DOCKER_CONFIG=\"/Users/invalid.user/.finch\""+
126+
"\n[ -L /usr/local/bin/docker-credential-ecr-login ] || sudo ln -s "+
127+
"/Users/invalid.user/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/"+
128+
"\n"+"[ -L /root/.aws ] || sudo ln -fs /Users/invalid.user/.aws /root/.aws"), fileBytes)
129+
},
130+
want: nil,
131+
},
94132
}
95133

96134
for _, tc := range testCases {
97135
tc := tc
98136
t.Run(tc.name, func(t *testing.T) {
99137
t.Parallel()
100138

139+
ctrl := gomock.NewController(t)
101140
fs := afero.NewMemMapFs()
141+
d := mocks.NewDialer(ctrl)
142+
lima := mocks.NewMockLimaWrapper(ctrl)
102143

103-
tc.mockSvc(t, fs)
104-
got := updateEnvironment(fs, tc.user)
144+
tc.mockSvc(t, fs, lima)
145+
nca, _ := NewNerdctlApplier(d, fs, "/private-key", tc.hostUser, lima).(*nerdctlConfigApplier)
146+
got := nca.updateEnvironment(fs)
105147
require.Equal(t, tc.want, got)
106148

107149
tc.postRunCheck(t, fs)
@@ -241,9 +283,10 @@ func TestNerdctlConfigApplier_Apply(t *testing.T) {
241283
ctrl := gomock.NewController(t)
242284
fs := afero.NewMemMapFs()
243285
d := mocks.NewDialer(ctrl)
286+
lima := mocks.NewMockLimaWrapper(ctrl)
244287

245288
tc.mockSvc(t, fs, d)
246-
got := NewNerdctlApplier(d, fs, tc.path, tc.hostUser).Apply(tc.remoteAddr)
289+
got := NewNerdctlApplier(d, fs, tc.path, tc.hostUser, lima).Apply(tc.remoteAddr)
247290

248291
assert.Equal(t, tc.want, got)
249292
})

0 commit comments

Comments
 (0)