Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: remove duplicate line in known_hosts when minikube deletes #16965

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions cmd/minikube/cmd/delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"github.com/docker/machine/libmachine"
"github.com/spf13/cobra"
"github.com/spf13/viper"
"k8s.io/client-go/util/homedir"
"k8s.io/klog/v2"
cmdcfg "k8s.io/minikube/cmd/minikube/cmd/config"
"k8s.io/minikube/pkg/drivers/kic"
Expand All @@ -53,6 +54,7 @@ import (
"k8s.io/minikube/pkg/minikube/reason"
"k8s.io/minikube/pkg/minikube/sshagent"
"k8s.io/minikube/pkg/minikube/style"
"k8s.io/minikube/pkg/util"
)

var (
Expand Down Expand Up @@ -232,7 +234,15 @@ func runDelete(_ *cobra.Command, args []string) {
delCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
defer cancel()

// use a background goroutine to run this delete
// so that this will not block for a long time
deleteKnownHostsChannel := make(chan struct{})
go func() {
deleteKnownHosts()
deleteKnownHostsChannel <- struct{}{}
}()
if deleteAll {

deleteContainersAndVolumes(delCtx, oci.Docker)
deleteContainersAndVolumes(delCtx, oci.Podman)

Expand All @@ -244,6 +254,7 @@ func runDelete(_ *cobra.Command, args []string) {
} else {
out.Step(style.DeletingHost, "Successfully deleted all profiles")
}

} else {
if len(args) > 0 {
exit.Message(reason.Usage, "usage: minikube delete")
Expand All @@ -270,6 +281,8 @@ func runDelete(_ *cobra.Command, args []string) {
delete.PossibleLeftOvers(delCtx, cname, driver.Podman)
}
}
// if deleteKnownHosts hasn't finished, then wait
<-deleteKnownHostsChannel

// If the purge flag is set, go ahead and delete the .minikube directory.
if purge {
Expand Down Expand Up @@ -725,3 +738,44 @@ func getPids(path string) ([]int, error) {

return pids, nil
}

// deleteKnownHosts reads minikube nodes' keys from the minikube home folder
// and removes them from the known_hosts file. This is the long term solution
// for issue https://github.com/kubernetes/minikube/issues/16868
func deleteKnownHosts() {
ComradeProgrammer marked this conversation as resolved.
Show resolved Hide resolved
// remove this line from known_hosts file if it exists
knownHosts := filepath.Join(homedir.HomeDir(), ".ssh", "known_hosts")
machinesDir := filepath.Join(localpath.MiniPath(), "machines")
fileInfo, err := os.ReadDir(machinesDir)
if err != nil {
klog.Warningf("error reading machines in minikube home: %v", err)
return
}
if _, err := os.Stat(knownHosts); err != nil {
klog.Warningf("no host files found when cleaning host files: %v", err)
return
}
for _, file := range fileInfo {
if file.IsDir() {
nodeName := file.Name()

knownHostPath := filepath.Join(localpath.MiniPath(), "machines", nodeName, "known_host")
if _, err := os.Stat(knownHostPath); err == nil {
// if this file exists, remove this line from known_hosts
key, err := os.ReadFile(knownHostPath)
if err != nil {
klog.Warningf("error reading keys from %s: %v", knownHostPath, err)
continue
}
if err := util.RemoveLineFromFile(string(key), knownHosts); err != nil {
klog.Warningf("failed to remove key: %v", err)
}
// and, remove the file which stores this key
if err := os.Remove(knownHostPath); err != nil {
klog.Warningf("failed to remove key: %v", err)
}
}
}
ComradeProgrammer marked this conversation as resolved.
Show resolved Hide resolved
}

}
6 changes: 4 additions & 2 deletions cmd/minikube/cmd/docker-env.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ docker-cli install instructions: https://minikube.sigs.k8s.io/docs/tutorials/doc
dockerPath = ""
}

if dockerPath != "" {
if dockerPath != "" && co.Config.KubernetesConfig.ContainerRuntime == constants.Docker {
out, err := tryDockerConnectivity("docker", ec)
if err != nil { // docker might be up but been loaded with wrong certs/config
// to fix issues like this #8185
Expand Down Expand Up @@ -427,7 +427,9 @@ docker-cli install instructions: https://minikube.sigs.k8s.io/docs/tutorials/doc
// TODO: refactor to work with docker, temp fix to resolve regression
ComradeProgrammer marked this conversation as resolved.
Show resolved Hide resolved
if cr == constants.Containerd {
// eventually, run something similar to ssh --append-known
appendKnownHelper(nodeName, true)
if err := appendKnownHelper(nodeName, true); err != nil {
exit.Error(reason.AppendKnownError, "failed to append keys to known_hosts", err)
}
}
}
},
Expand Down
19 changes: 15 additions & 4 deletions cmd/minikube/cmd/ssh-host.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"k8s.io/minikube/pkg/minikube/config"
"k8s.io/minikube/pkg/minikube/driver"
"k8s.io/minikube/pkg/minikube/exit"
"k8s.io/minikube/pkg/minikube/localpath"
"k8s.io/minikube/pkg/minikube/machine"
"k8s.io/minikube/pkg/minikube/mustload"
"k8s.io/minikube/pkg/minikube/node"
Expand All @@ -45,11 +46,13 @@ var sshHostCmd = &cobra.Command{
Short: "Retrieve the ssh host key of the specified node",
Long: "Retrieve the ssh host key of the specified node.",
Run: func(_ *cobra.Command, _ []string) {
appendKnownHelper(nodeName, appendKnown)
if err := appendKnownHelper(nodeName, appendKnown); err != nil {
exit.Error(reason.AppendKnownError, "failed to append keys to known_hosts", err)
}
},
}

func appendKnownHelper(nodeName string, appendKnown bool) {
func appendKnownHelper(nodeName string, appendKnown bool) error {
cname := ClusterFlagValue()
co := mustload.Running(cname)
if co.CP.Host.DriverName == driver.None {
Expand Down Expand Up @@ -99,7 +102,7 @@ func appendKnownHelper(nodeName string, appendKnown bool) {
knownHosts := filepath.Join(sshDir, "known_hosts")

if sshutil.KnownHost(host, knownHosts) {
return
return nil
}

f, err := os.OpenFile(knownHosts, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600)
Expand All @@ -122,8 +125,16 @@ func appendKnownHelper(nodeName string, appendKnown bool) {

fmt.Fprintf(os.Stderr, "Host added: %s (%s)\n", knownHosts, host)

return
// then store it into minikube home folder
// so that when we execute `minikube delete`
// these keys can be removed properly
_, cc := mustload.Partial(ClusterFlagValue())
knownHostPath := filepath.Join(localpath.MiniPath(), "machines", config.MachineName(*cc, *n), "known_host")
if err := os.WriteFile(knownHostPath, []byte(keys), 0666); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this needs to be 666 vs 644?

return fmt.Errorf("WriteFile to %s: %v", knownHostPath, err)
}
}
return nil
}

func init() {
Expand Down
2 changes: 2 additions & 0 deletions pkg/minikube/reason/reason.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,8 @@ var (
InternalSemverParse = Kind{ID: "MK_SEMVER_PARSE", ExitCode: ExProgramError}
// minikube was unable to daemonize the minikube process
DaemonizeError = Kind{ID: "MK_DAEMONIZE", ExitCode: ExProgramError}
// minikube was unable to add keys to known_hosts
AppendKnownError = Kind{ID: "MK_APPEND_KNOWN", ExitCode: ExProgramError}
spowelljr marked this conversation as resolved.
Show resolved Hide resolved

// insufficient cores available for use by minikube and kubernetes
RsrcInsufficientCores = Kind{ID: "RSRC_INSUFFICIENT_CORES", ExitCode: ExInsufficientCores, Style: style.UnmetRequirement}
Expand Down
37 changes: 37 additions & 0 deletions pkg/util/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package util

import (
"bufio"
"fmt"
"os"
"os/user"
Expand Down Expand Up @@ -163,3 +164,39 @@ func MaskProxyPasswordWithKey(v string) string {
}
return v
}

// RemoveLineFromFile removes the specified line from the given file
func RemoveLineFromFile(knownHostLine string, filePath string) error {
ComradeProgrammer marked this conversation as resolved.
Show resolved Hide resolved
fd, err := os.OpenFile(filePath, os.O_CREATE|os.O_RDWR, 0666)
if err != nil {
return fmt.Errorf("failed to open %s: %v", filePath, err)
}
defer fd.Close()

// read each line from known_hosts and find the line we want to delete
scanner := bufio.NewScanner(fd)
newLines := make([]string, 0)
for scanner.Scan() {
line := string(scanner.Bytes())
if strings.TrimSpace(line) != strings.TrimSpace(knownHostLine) {
// remove the line which is identical with the key
newLines = append(newLines, line)
}
}
// remove the contents and move to the head of this file
if err := fd.Truncate(0); err != nil {
return fmt.Errorf("failed to truncate %s: %v", filePath, err)
}
if _, err := fd.Seek(0, 0); err != nil {
return fmt.Errorf("failed to seek %s: %v", filePath, err)
}

// write the new content into the file

spowelljr marked this conversation as resolved.
Show resolved Hide resolved
for _, line := range newLines {
if _, err := fmt.Fprintln(fd, line); err != nil {
return fmt.Errorf("failed to write to %s: %v", filePath, err)
}
}
return nil
}
33 changes: 33 additions & 0 deletions pkg/util/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package util
import (
"os"
"os/user"
"strings"
"syscall"
"testing"

Expand Down Expand Up @@ -310,3 +311,35 @@ func TestMaskProxyPasswordWithKey(t *testing.T) {
}
}
}

var testFileContent = `
127.0.0.1 localhost
127.0.0.1 mbp13
255.255.255.255 broadcasthost
::1 localhost
35.244.161.222 remove.test
`

func TestRemoveLineFromFile(t *testing.T) {
filePath := "./test_known_hosts"
// first we write the content into the test file
if err := os.WriteFile(filePath, []byte(testFileContent), 0666); err != nil {
t.Errorf("Failed to write to test file, err=%v", err)
}
defer os.Remove(filePath)

line := "35.244.161.222 remove.test"
if err := RemoveLineFromFile(line, filePath); err != nil {
t.Errorf("Failed to run RemoveLineFromFile, err=%v", err)
}
// now read the file again to see whether this line still exists

newContent, err := os.ReadFile(filePath)
if err != nil {
t.Errorf("Failed to read file, err=%v", err)
ComradeProgrammer marked this conversation as resolved.
Show resolved Hide resolved
}
if strings.Contains(string(newContent), line) {
t.Errorf("Failed to remove the line")
}

}