Skip to content

Commit

Permalink
fix: Use LimaUser method instead of host user name (#712)
Browse files Browse the repository at this point in the history 
Issue #, if available: #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 <hwhaibarawataru@gmail.com>
  • Loading branch information
@w-haibara
w-haibara authored Dec 2, 2023
1 parent 4d1e6cf commit 7c02e08
Show file tree
Hide file tree
Showing 3 changed files with 111 additions and 59 deletions.
8 changes: 5 additions & 3 deletions cmd/finch/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,22 +90,23 @@ var newApp = func(logger flog.Logger, fp path.Finch, fs afero.Fs, fc *config.Fin
fp.QEMUBinDir(),
system.NewStdLib(),
)
lima := wrapper.NewLimaWrapper()
supportBundleBuilder := support.NewBundleBuilder(
logger,
fs,
support.NewBundleConfig(fp, system.NewStdLib().Env("HOME")),
fp,
ecc,
lcc,
wrapper.NewLimaWrapper(),
lima,
)

// append nerdctl commands
allCommands := initializeNerdctlCommands(lcc, ecc, logger, fs, fc)
// append finch specific commands
allCommands = append(allCommands,
newVersionCommand(lcc, logger, stdOut),
virtualMachineCommands(logger, fp, lcc, ecc, fs, fc),
virtualMachineCommands(logger, fp, lcc, ecc, fs, fc, lima),
newSupportBundleCommand(logger, supportBundleBuilder, lcc),
newGenDocsCommand(rootCmd, logger, fs, system.NewStdLib()),
)
Expand All @@ -122,6 +123,7 @@ func virtualMachineCommands(
ecc *command.ExecCmdCreator,
fs afero.Fs,
fc *config.Finch,
lima wrapper.LimaWrapper,
) *cobra.Command {
optionalDepGroups := []*dependency.Group{
vmnet.NewDependencyGroup(ecc, lcc, fs, fp, logger),
Expand All @@ -133,7 +135,7 @@ func virtualMachineCommands(
logger,
optionalDepGroups,
config.NewLimaApplier(fc, ecc, fs, fp.LimaOverrideConfigPath(), system.NewStdLib()),
config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath(), system.NewStdLib().Env("USER")),
config.NewNerdctlApplier(fssh.NewDialer(), fs, fp.LimaSSHPrivateKeyPath(), system.NewStdLib().Env("USER"), lima),
fp,
fs,
disk.NewUserDataDiskManager(lcc, ecc, &afero.OsFs{}, fp, system.NewStdLib().Env("HOME"), fc),
Expand Down
89 changes: 48 additions & 41 deletions pkg/config/nerdctl_config_applier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/spf13/afero/sftpfs"

"github.com/runfinch/finch/pkg/fssh"
"github.com/runfinch/finch/pkg/lima/wrapper"
)

const (
Expand All @@ -27,18 +28,20 @@ type nerdctlConfigApplier struct {
fs afero.Fs
privateKeyPath string
hostUser string
lima wrapper.LimaWrapper
}

var _ NerdctlConfigApplier = (*nerdctlConfigApplier)(nil)

// NewNerdctlApplier creates a new NerdctlConfigApplier that
// applies nerdctl configuration changes by SSHing to the lima VM to update the nerdctl configuration file in it.
func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath, hostUser string) NerdctlConfigApplier {
func NewNerdctlApplier(dialer fssh.Dialer, fs afero.Fs, privateKeyPath string, hostUser string, lima wrapper.LimaWrapper) NerdctlConfigApplier {
return &nerdctlConfigApplier{
dialer: dialer,
fs: fs,
privateKeyPath: privateKeyPath,
hostUser: hostUser,
lima: lima,
}
}

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

// updateEnvironment adds variables to the user's shell's environment. Currently it uses ~/.bashrc because
// Bash is the default shell and Bash will not load ~/.profile if ~/.bash_profile exists (which it does).
// ~/.bash_profile sources ~/.bashrc, so ~/.bashrc is currently the best place to define additional variables.
// The [GNU docs for Bash] explain how these files work together in more details.
// The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to
// ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json).
// The reason that we don't set environment variables inside /root/.bashrc is that the vars inside it are
// 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
// normally we would access the VM as the regular user.
// For more information on the variable, see the registry nerdctl docs.
//
// [GNU docs for Bash]: https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html
//
// [registry nerdctl docs]: https://github.com/containerd/nerdctl/blob/master/docs/registry.md

func updateEnvironment(fs afero.Fs, user string) error {
cmdArr := [4]string{
fmt.Sprintf("export DOCKER_CONFIG=\"/Users/%s/.finch\"", user),
fmt.Sprintf("[ -L /usr/local/bin/docker-credential-ecr-login ] "+
"|| sudo ln -s /Users/%s/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/", user),
fmt.Sprintf("[ -L /root/.aws ] || sudo ln -fs /Users/%s/.aws /root/.aws", user),
}

profileFilePath := fmt.Sprintf("/home/%s.linux/.bashrc", user)
profBuf, err := afero.ReadFile(fs, profileFilePath)
if err != nil {
return fmt.Errorf("failed to read config file: %w", err)
}
profStr := string(profBuf)
for _, element := range cmdArr {
profStr, err = addLineToBashrc(fs, profileFilePath, profStr, element)
if err != nil {
return err
}
}

return nil
}

// updateNerdctlConfig reads from the nerdctl config and updates values.
func updateNerdctlConfig(fs afero.Fs, user string, rootless bool) error {
nerdctlRootlessCfgPath := fmt.Sprintf("/home/%s.linux/.config/nerdctl/nerdctl.toml", user)
Expand Down Expand Up @@ -137,6 +101,49 @@ func updateNerdctlConfig(fs afero.Fs, user string, rootless bool) error {
return nil
}

// updateEnvironment adds variables to the user's shell's environment. Currently it uses ~/.bashrc because
// Bash is the default shell and Bash will not load ~/.profile if ~/.bash_profile exists (which it does).
// ~/.bash_profile sources ~/.bashrc, so ~/.bashrc is currently the best place to define additional variables.
// The [GNU docs for Bash] explain how these files work together in more details.
// The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to
// ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json).
// The reason that we don't set environment variables inside /root/.bashrc is that the vars inside it are
// 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
// normally we would access the VM as the regular user.
// For more information on the variable, see the registry nerdctl docs.
//
// [GNU docs for Bash]: https://www.gnu.org/software/bash/manual/html_node/Bash-Startup-Files.html
//
// [registry nerdctl docs]: https://github.com/containerd/nerdctl/blob/master/docs/registry.md
func (nca *nerdctlConfigApplier) updateEnvironment(fs afero.Fs) error {
cmdArr := [4]string{
fmt.Sprintf("export DOCKER_CONFIG=\"/Users/%s/.finch\"", nca.hostUser),
fmt.Sprintf("[ -L /usr/local/bin/docker-credential-ecr-login ] "+
"|| sudo ln -s /Users/%s/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/", nca.hostUser),
fmt.Sprintf("[ -L /root/.aws ] || sudo ln -fs /Users/%s/.aws /root/.aws", nca.hostUser),
}

limaUser, err := nca.lima.LimaUser(false)
if err != nil {
return fmt.Errorf("failed to get lima user: %w", err)
}

profileFilePath := fmt.Sprintf("/home/%s.linux/.bashrc", limaUser.Username)
profBuf, err := afero.ReadFile(fs, profileFilePath)
if err != nil {
return fmt.Errorf("failed to read config file: %w", err)
}
profStr := string(profBuf)
for _, element := range cmdArr {
profStr, err = addLineToBashrc(fs, profileFilePath, profStr, element)
if err != nil {
return err
}
}

return nil
}

// Apply gets SSH and SFTP clients and uses them to update the nerdctl config.
func (nca *nerdctlConfigApplier) Apply(remoteAddr string) error {
user := "root"
Expand All @@ -162,7 +169,7 @@ func (nca *nerdctlConfigApplier) Apply(remoteAddr string) error {
return fmt.Errorf("failed to update the nerdctl config file: %w", err)
}

if err := updateEnvironment(sftpFs, nca.hostUser); err != nil {
if err := nca.updateEnvironment(sftpFs); err != nil {
return fmt.Errorf("failed to update the user's .profile file: %w", err)
}
return nil
Expand Down
73 changes: 58 additions & 15 deletions pkg/config/nerdctl_config_applier_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"errors"
"fmt"
"io/fs"
"os/user"
"testing"

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

func Test_updateEnvironment(t *testing.T) {
func TestNerdctlConfigApplier_updateEnvironment(t *testing.T) {
t.Parallel()

testCases := []struct {
name string
user string
mockSvc func(t *testing.T, fs afero.Fs)
hostUser string
mockSvc func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper)
postRunCheck func(t *testing.T, fs afero.Fs)
want error
}{
{
name: "happy path",
user: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs) {
name: "happy path",
hostUser: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
require.NoError(t, afero.WriteFile(fs, "/home/mock_user.linux/.bashrc", []byte(""), 0o644))

mockUser := &user.User{
Username: "mock_user",
}
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
},
postRunCheck: func(t *testing.T, fs afero.Fs) {
fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
Expand All @@ -56,9 +62,9 @@ func Test_updateEnvironment(t *testing.T) {
want: nil,
},
{
name: "happy path, file already exists and already contains expected variables",
user: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs) {
name: "happy path, file already exists and already contains expected variables",
hostUser: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
require.NoError(
t,
afero.WriteFile(
Expand All @@ -70,6 +76,11 @@ func Test_updateEnvironment(t *testing.T) {
0o644,
),
)

mockUser := &user.User{
Username: "mock_user",
}
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
},
postRunCheck: func(t *testing.T, fs afero.Fs) {
fileBytes, err := afero.ReadFile(fs, "/home/mock_user.linux/.bashrc")
Expand All @@ -82,26 +93,57 @@ func Test_updateEnvironment(t *testing.T) {
want: nil,
},
{
name: ".bashrc file doesn't exist",
user: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs) {},
name: ".bashrc file doesn't exist",
hostUser: "mock_user",
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
mockUser := &user.User{
Username: "mock_user",
}
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
},
postRunCheck: func(t *testing.T, fs afero.Fs) {},
want: fmt.Errorf(
"failed to read config file: %w",
&fs.PathError{Op: "open", Path: "/home/mock_user.linux/.bashrc", Err: errors.New("file does not exist")},
),
},
{
name: "host user is not a valid linux username",
hostUser: "invalid.user",
mockSvc: func(t *testing.T, fs afero.Fs, lima *mocks.MockLimaWrapper) {
require.NoError(t, afero.WriteFile(fs, "/home/lima.linux/.bashrc", []byte(""), 0o644))

mockUser := &user.User{
Username: "lima",
}
lima.EXPECT().LimaUser(false).Return(mockUser, nil).AnyTimes()
},
postRunCheck: func(t *testing.T, fs afero.Fs) {
fileBytes, err := afero.ReadFile(fs, "/home/lima.linux/.bashrc")
require.NoError(t, err)
assert.Equal(t,
[]byte("\nexport DOCKER_CONFIG=\"/Users/invalid.user/.finch\""+
"\n[ -L /usr/local/bin/docker-credential-ecr-login ] || sudo ln -s "+
"/Users/invalid.user/.finch/cred-helpers/docker-credential-ecr-login /usr/local/bin/"+
"\n"+"[ -L /root/.aws ] || sudo ln -fs /Users/invalid.user/.aws /root/.aws"), fileBytes)
},
want: nil,
},
}

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

ctrl := gomock.NewController(t)
fs := afero.NewMemMapFs()
d := mocks.NewDialer(ctrl)
lima := mocks.NewMockLimaWrapper(ctrl)

tc.mockSvc(t, fs)
got := updateEnvironment(fs, tc.user)
tc.mockSvc(t, fs, lima)
nca, _ := NewNerdctlApplier(d, fs, "/private-key", tc.hostUser, lima).(*nerdctlConfigApplier)
got := nca.updateEnvironment(fs)
require.Equal(t, tc.want, got)

tc.postRunCheck(t, fs)
Expand Down Expand Up @@ -241,9 +283,10 @@ func TestNerdctlConfigApplier_Apply(t *testing.T) {
ctrl := gomock.NewController(t)
fs := afero.NewMemMapFs()
d := mocks.NewDialer(ctrl)
lima := mocks.NewMockLimaWrapper(ctrl)

tc.mockSvc(t, fs, d)
got := NewNerdctlApplier(d, fs, tc.path, tc.hostUser).Apply(tc.remoteAddr)
got := NewNerdctlApplier(d, fs, tc.path, tc.hostUser, lima).Apply(tc.remoteAddr)

assert.Equal(t, tc.want, got)
})
Expand Down

0 comments on commit 7c02e08

Please sign in to comment.