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.

Sign up for GitHub

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

Jump to bottom

OCPBUGS-16227: make sure sshKey are not emptied out on firstboot #3829

Merged
merged 1 commit into from
Aug 3, 2023
Merged

OCPBUGS-16227: make sure sshKey are not emptied out on firstboot #3829

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 15 additions & 11 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,19 +461,23 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig, skipCertifi
return err
}

if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
return err
}
// only update passwd if it has changed (do not nullify)
// we do not need to include SetPasswordHash in this, since only updateSSHKeys has issues on firstboot.
if diff.passwd {
if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
return err
}

defer func() {
if retErr != nil {
if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
errs := kubeErrs.NewAggregate([]error{err, retErr})
retErr = fmt.Errorf("error rolling back SSH keys updates: %w", errs)
return
defer func() {
if retErr != nil {
if err := dn.updateSSHKeys(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
errs := kubeErrs.NewAggregate([]error{err, retErr})
retErr = fmt.Errorf("error rolling back SSH keys updates: %w", errs)
return
}
}
}
}()
}()
}

// Set password hash
if err := dn.SetPasswordHash(newIgnConfig.Passwd.Users, oldIgnConfig.Passwd.Users); err != nil {
Expand Down
75 changes: 75 additions & 0 deletions test/e2e/mcd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package e2e_test

import (
"bytes"
"context"
"fmt"
"os/exec"
"path/filepath"
"strconv"
"strings"
Expand Down Expand Up @@ -1007,6 +1009,79 @@ func TestMCDRotatesCertsOnPausedPool(t *testing.T) {

}

func TestFirstBootHasSSHKeys(t *testing.T) {
cs := framework.NewClientSet("")
outNodeYoungest := bytes.NewBuffer([]byte{})
// get nodes by newest
outNodeYoungest = runCmd(t, "oc get nodes --sort-by .metadata.creationTimestamp | tail -n 1")
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): You can do these node queries via the Clientset without having to shell out and parse oc output:

package main

import (
	"context"
	"fmt"
	"sort"

	"github.com/openshift/machine-config-operator/test/framework"

	corev1 "k8s.io/api/core/v1"
	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

func printNodeStatusLine(node *corev1.Node) {
	fmt.Printf("Node %s was created at %s. Is ready? %v\n", node.Name, node.CreationTimestamp, isNodeReady(node))
}

// This will determine whether or not a node is ready. There are additional conditions one can look at and consider, though this should be enough for this particular use-case. See https://github.com/openshift/machine-config-operator/blob/94fc0354f154e2daa923dd96defdf86448b2b327/pkg/controller/node/status.go#L237-L263 for more info.
func isNodeReady(node *corev1.Node) bool {
        for _, condition := range node.Status.Conditions {
                if condition.Reason == "KubeletReady" && condition.Type == corev1.NodeReady && condition.Status == corev1.ConditionTrue {
                        return true
                }
        }

        return false
}

func getYoungestNodeFromPool(nodeList *corev1.NodeList) *corev1.Node {
	// Sort the nodes in descending order by the creation timestamp. The youngest node will be in position '0'.
	sort.Slice(nodeList.Items, func(i, j int) bool {
		return nodeList.Items[i].CreationTimestamp.Time.Unix() > nodeList.Items[j].CreationTimestamp.Time.Unix()
	})

	youngestNode := nodeList.Items[0]
	return &youngestNode
}

func main() {
	cs := framework.NewClientSet("")

	nodeList, err := cs.CoreV1Interface.Nodes().List(context.TODO(), metav1.ListOptions{
		LabelSelector: "node-role.kubernetes.io/worker=",
	})

	if err != nil {
		panic(err)
	}

	youngestNode := getYoungestNodeFromPool(nodeList)
	fmt.Printf("Youngest node: ")
	printNodeStatusLine(youngestNode)

	fmt.Println("Nodes in worker pool:")
	for _, node := range nodeList.Items {
		printNodeStatusLine(&node)
	}
}

This will produce output like this:

Youngest node: Node ip-10-0-30-153.ec2.internal was created at 2023-08-03 08:23:03 -0400 EDT. Is ready? true
Nodes in worker pool:
Node ip-10-0-30-153.ec2.internal was created at 2023-08-03 08:23:03 -0400 EDT. Is ready? true
Node ip-10-0-2-150.ec2.internal was created at 2023-08-03 08:20:39 -0400 EDT. Is ready? true
Node ip-10-0-45-21.ec2.internal was created at 2023-08-03 08:20:31 -0400 EDT. Is ready? true

Copy link
Member

Choose a reason for hiding this comment

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

Since my code sample above deals with timestamps, I'd like to bring attention to the fact that the Golang Time types in the stdlib are not marshalable. That means if you convert it to JSON and try to convert it back, you'll get weird conversion errors; if it even converts at all!

With that in mind, you may be wondering how Kubernetes handles this problem, given that we have timestamps. Here's the answer: https://github.com/kubernetes/apimachinery/blob/5cb236977966cfc829e3b94b57fa69b44e0a95bc/pkg/apis/meta/v1/time.go

outMSet := runCmd(t, "oc -n openshift-machine-api -o name get machinesets | head -n 1")
mset := strings.Trim(strings.Split(outMSet.String(), "/")[1], "\n")
runCmd(t, "oc scale --replicas=2 machineset "+mset+" -n openshift-machine-api")
// scale a 2nd machine
outNodeYoungestNew := &bytes.Buffer{}
nodeStr := strings.Split(outNodeYoungest.String(), " ")[0]
t.Cleanup(func() {
if len(outNodeYoungestNew.String()) > 0 && strings.Split(outNodeYoungestNew.String(), " ")[0] != nodeStr {
// scale down
outMSet = runCmd(t, "oc scale --replicas=1 machineset "+mset+" -n openshift-machine-api")
splitNodes := []string{}
if err := wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, 20*time.Minute, false, func(ctx context.Context) (bool, error) {
// get all nodes
nodes := runCmd(t, "oc get nodes")
splitNodes = strings.Split(nodes.String(), "\n")
for _, n := range splitNodes {
// find the one with scheduling disabled and delete it
if strings.Contains(n, "SchedulingDisabled") {
return false, nil
}
}
return true, nil
}); err != nil {
t.Fatalf("did not get old node upon cleanup: %s", splitNodes)
}
}

})
nodeSplit := []string{"", ""}
if err := wait.PollUntilContextTimeout(context.TODO(), 2*time.Second, 20*time.Minute, false, func(ctx context.Context) (bool, error) {
outNodeYoungestNew = bytes.NewBuffer([]byte{})
// get nodes over and over
outNodeYoungestNew = runCmd(t, "oc get nodes --sort-by .metadata.creationTimestamp | tail -n 1")
nodeSplit = strings.SplitN(outNodeYoungestNew.String(), " ", 2)
// if node name != first node name and it is ready, we have a node
if nodeSplit[0] != nodeStr && strings.Contains(nodeSplit[1], "Ready") && !strings.Contains(nodeSplit[1], "NotReady") {
return true, nil
}
return false, nil
}); err != nil {
t.Fatal("did not get new node")
}

nodes, err := helpers.GetNodesByRole(cs, "worker")
require.Nil(t, err)
foundNode := false
for _, node := range nodes {
if node.Name == nodeSplit[0] && strings.Contains(nodeSplit[1], "Ready") && !strings.Contains(nodeSplit[1], "NotReady") {
foundNode = true
out := helpers.ExecCmdOnNode(t, cs, node, "cat", "/rootfs/home/core/.ssh/authorized_keys.d/ignition")
t.Logf("Got ssh key file data: %s", out)
require.NotEmpty(t, out)
}
}
require.True(t, foundNode)
}

func runCmd(t *testing.T, cmds string) *bytes.Buffer {
stdout := bytes.NewBuffer([]byte{})
stderr := bytes.NewBuffer([]byte{})
cmd := exec.Command("bash", "-c", cmds)
cmd.Stdout = stdout
cmd.Stderr = stderr
err := cmd.Run()
require.NoError(t, err, "Got stdout: %s and stderr: %s, error: %s", stdout.String(), stderr.String(), err)
return stdout
}

func createMCToAddFileForRole(name, role, filename, data string) *mcfgv1.MachineConfig {
mcadd := helpers.CreateMC(fmt.Sprintf("%s-%s", name, uuid.NewUUID()), role)

Expand Down