From 77d7353f6bbb83249082bf740a96e6f650a37ce2 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:24:10 +0100 Subject: [PATCH 1/7] add test for nodes json output --- pkg/cmd/ssh/ssh_test.go | 42 ++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index 0cba7122..9cc726c4 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -107,6 +107,7 @@ var _ = Describe("SSH Command", func() { testShoot *gardencorev1beta1.Shoot testNode *corev1.Node testMachine *machinev1alpha1.Machine + monitoringMachine *machinev1alpha1.Machine seedKubeconfigSecret *corev1.Secret gardenClient client.Client shootClient client.Client @@ -255,7 +256,7 @@ var _ = Describe("SSH Command", func() { WithStatusSubresource(&operationsv1alpha1.Bastion{}). Build()) - // create a fake shoot cluster with a single node in it + // create a fake shoot cluster with two machines, where one node has already joined the cluster testNode = &corev1.Node{ ObjectMeta: metav1.ObjectMeta{ Name: "node1", @@ -272,10 +273,21 @@ var _ = Describe("SSH Command", func() { ObjectMeta: metav1.ObjectMeta{ Name: "machine1", Namespace: "shoot--prod1--test-shoot", - Labels: map[string]string{"node": "node1"}, + Labels: map[string]string{machinev1alpha1.NodeLabelKey: "node1"}, }, } + monitoringMachine = &machinev1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "monitoring1", + Namespace: "shoot--prod1--test-shoot", + Labels: map[string]string{machinev1alpha1.NodeLabelKey: "monitoring1"}, + }, + } + + shootClient = internalfake.NewClientWithObjects(testNode) + seedClient = internalfake.NewClientWithObjects(testMachine, monitoringMachine) + streams, _, out, _ = util.NewTestIOStreams() ctrl = gomock.NewController(GinkgoT()) @@ -303,14 +315,16 @@ var _ = Describe("SSH Command", func() { Describe("RunE", func() { BeforeEach(func() { + seedClientConfig, err := clientcmd.NewClientConfigFromBytes(seedKubeconfigSecret.Data["kubeconfig"]) + Expect(err).NotTo(HaveOccurred()) + clientProvider.EXPECT().FromClientConfig(gomock.Eq(seedClientConfig)).Return(seedClient, nil).AnyTimes() + clientProvider.EXPECT().FromClientConfig(gomock.Any()).Return(shootClient, nil).AnyTimes(). Do(func(clientConfig clientcmd.ClientConfig) { config, err := clientConfig.RawConfig() Expect(err).NotTo(HaveOccurred()) Expect(config.CurrentContext).To(Equal(testShoot.Namespace + "--" + testShoot.Name + "-" + testShoot.Status.AdvertisedAddresses[0].Name)) }) - - shootClient = internalfake.NewClientWithObjects(testNode) }) It("should reject bad options", func() { @@ -644,6 +658,15 @@ var _ = Describe("SSH Command", func() { Expect(info.Bastion.PreferredAddress).To(Equal("0.0.0.0")) Expect(info.Bastion.SSHPrivateKeyFile).To(Equal(options.SSHPrivateKeyFile)) Expect(info.Bastion.SSHPublicKeyFile).To(Equal(options.SSHPublicKeyFile)) + Expect(info.Nodes).To(ConsistOf([]ssh.Node{ + { + Name: testNode.Name, + Status: "Not Ready", + Address: ssh.Address{ + Hostname: nodeHostname, + }, + }, + })) Expect(info.NodePrivateKeyFiles).NotTo(BeEmpty()) }) @@ -669,17 +692,6 @@ var _ = Describe("SSH Command", func() { manager = targetmocks.NewMockManager(ctrl) client = gardenclientmocks.NewMockClient(ctrl) - monitoringMachine := &machinev1alpha1.Machine{ - ObjectMeta: metav1.ObjectMeta{ - Name: "monitoring1", - Namespace: "shoot--prod1--test-shoot", - Labels: map[string]string{"node": "monitoring1"}, - }, - } - - seedClient = internalfake.NewClientWithObjects(testMachine, monitoringMachine) - shootClient = internalfake.NewClientWithObjects(testNode) - factory.ManagerImpl = manager manager.EXPECT().CurrentTarget().Return(currentTarget, nil) manager.EXPECT().GardenClient(currentTarget.GardenName()).Return(client, nil) From 6562344afd57d2863cf49a6247231267978e2219 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:26:14 +0100 Subject: [PATCH 2/7] rename to pendingMachine --- pkg/cmd/ssh/ssh_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index 9cc726c4..df9a92f1 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -107,7 +107,7 @@ var _ = Describe("SSH Command", func() { testShoot *gardencorev1beta1.Shoot testNode *corev1.Node testMachine *machinev1alpha1.Machine - monitoringMachine *machinev1alpha1.Machine + pendingMachine *machinev1alpha1.Machine seedKubeconfigSecret *corev1.Secret gardenClient client.Client shootClient client.Client @@ -277,7 +277,7 @@ var _ = Describe("SSH Command", func() { }, } - monitoringMachine = &machinev1alpha1.Machine{ + pendingMachine = &machinev1alpha1.Machine{ ObjectMeta: metav1.ObjectMeta{ Name: "monitoring1", Namespace: "shoot--prod1--test-shoot", @@ -286,7 +286,7 @@ var _ = Describe("SSH Command", func() { } shootClient = internalfake.NewClientWithObjects(testNode) - seedClient = internalfake.NewClientWithObjects(testMachine, monitoringMachine) + seedClient = internalfake.NewClientWithObjects(testMachine, pendingMachine) streams, _, out, _ = util.NewTestIOStreams() From 6d3c1fbd61a9b8a571d719da76d9897aebdf8d06 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:37:08 +0100 Subject: [PATCH 3/7] initialize struct --- pkg/cmd/ssh/connect_information.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/ssh/connect_information.go b/pkg/cmd/ssh/connect_information.go index dc19308a..364343ec 100644 --- a/pkg/cmd/ssh/connect_information.go +++ b/pkg/cmd/ssh/connect_information.go @@ -92,9 +92,10 @@ func NewConnectInformation( var nodeList []Node for _, node := range nodes { - n := Node{} - n.Name = node.Name - n.Status = "Ready" + n := Node{ + Name: node.Name, + Status: "Ready", + } if !isNodeReady(node) { n.Status = "Not Ready" From c5d3f715a1252d52de6c2301c012255de4b97f83 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:37:46 +0100 Subject: [PATCH 4/7] add comment why forbidden error is not logged --- pkg/cmd/ssh/options.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index 85670af6..591bb569 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -835,6 +835,8 @@ func getNodeNamesFromMachinesOrNodes(ctx context.Context, manager target.Manager nodeNames, err := getNodeNamesFromMachines(ctx, manager, currentTarget) if err != nil { + // Regular users do not have the permission to fetch the machines. + // However, in this case, we do not want to log an error message. Instead, we will fallback to read the node names. if !apierrors.IsForbidden(err) { logger.Info("failed to fetch node names from machine objects", "err", err) } From 0cd9fc56c2429fe631964c8f15ee7c8e2082a691 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:38:25 +0100 Subject: [PATCH 5/7] support pending machines --- pkg/cmd/ssh/connect_information.go | 17 +++++++++++++++-- pkg/cmd/ssh/options.go | 13 +++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/ssh/connect_information.go b/pkg/cmd/ssh/connect_information.go index 364343ec..96b54516 100644 --- a/pkg/cmd/ssh/connect_information.go +++ b/pkg/cmd/ssh/connect_information.go @@ -87,9 +87,17 @@ func NewConnectInformation( sshPrivateKeyFile PrivateKeyFile, nodePrivateKeyFiles []PrivateKeyFile, nodes []corev1.Node, + pendingNodeNames []string, user string, ) (*ConnectInformation, error) { - var nodeList []Node + nodeSet := make(map[string]Node) + + for _, pendingNodeName := range pendingNodeNames { + nodeSet[pendingNodeName] = Node{ + Name: pendingNodeName, + Status: "Unknown", + } + } for _, node := range nodes { n := Node{ @@ -125,7 +133,12 @@ func NewConnectInformation( } } - nodeList = append(nodeList, n) + nodeSet[n.Name] = n + } + + nodeList := make([]Node, 0, len(nodeSet)) + for _, node := range nodeSet { + nodeList = append(nodeList, node) } return &ConnectInformation{ diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index 591bb569..0a211e3d 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -647,11 +647,23 @@ func (o *SSHOptions) Run(f util.Factory) error { if !o.Interactive { var nodes []corev1.Node + + var pendingNodeNames []string + if nodeHostname == "" { nodes, err = getNodes(ctx, shootClient) if err != nil { return fmt.Errorf("failed to list shoot cluster nodes: %w", err) } + + // We also want to determine if there are nodes that have not yet joined the cluster. + // This is an optional step, as only Gardener operators have access to the Seeds. + // Regular users will receive a 'Forbidden' error when trying to fetch the Machines. + // However, we do not want to log an error message in this case. + pendingNodeNames, err = getNodeNamesFromMachines(ctx, manager, currentTarget) + if err != nil && !apierrors.IsForbidden(err) { + logger.Info("failed to get shoot cluster node names from machines", "err", err) + } } connectInformation, err := NewConnectInformation( @@ -664,6 +676,7 @@ func (o *SSHOptions) Run(f util.Factory) error { o.SSHPrivateKeyFile, nodePrivateKeyFiles, nodes, + pendingNodeNames, o.User, ) if err != nil { From acebd86499027f83831c595dfd90f54e63ecfcfc Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 16 Jan 2024 16:38:33 +0100 Subject: [PATCH 6/7] add test --- pkg/cmd/ssh/ssh_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index df9a92f1..df0c3492 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -659,6 +659,10 @@ var _ = Describe("SSH Command", func() { Expect(info.Bastion.SSHPrivateKeyFile).To(Equal(options.SSHPrivateKeyFile)) Expect(info.Bastion.SSHPublicKeyFile).To(Equal(options.SSHPublicKeyFile)) Expect(info.Nodes).To(ConsistOf([]ssh.Node{ + { + Name: pendingMachine.Labels[machinev1alpha1.NodeLabelKey], + Status: "Unknown", + }, { Name: testNode.Name, Status: "Not Ready", From 89bb9c0dc8397b1c9406918d270cd16756f602b8 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Wed, 17 Jan 2024 15:39:26 +0100 Subject: [PATCH 7/7] PR feedback: rename to nodeMap --- pkg/cmd/ssh/connect_information.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/ssh/connect_information.go b/pkg/cmd/ssh/connect_information.go index 96b54516..daf517dc 100644 --- a/pkg/cmd/ssh/connect_information.go +++ b/pkg/cmd/ssh/connect_information.go @@ -90,10 +90,10 @@ func NewConnectInformation( pendingNodeNames []string, user string, ) (*ConnectInformation, error) { - nodeSet := make(map[string]Node) + nodeMap := make(map[string]Node) for _, pendingNodeName := range pendingNodeNames { - nodeSet[pendingNodeName] = Node{ + nodeMap[pendingNodeName] = Node{ Name: pendingNodeName, Status: "Unknown", } @@ -133,11 +133,11 @@ func NewConnectInformation( } } - nodeSet[n.Name] = n + nodeMap[n.Name] = n } - nodeList := make([]Node, 0, len(nodeSet)) - for _, node := range nodeSet { + nodeList := make([]Node, 0, len(nodeMap)) + for _, node := range nodeMap { nodeList = append(nodeList, node) }