From a6aa2c39c40f031869edf5bd5a3173946a89d51c Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Mon, 20 Mar 2023 17:21:31 +0100 Subject: [PATCH 01/10] use String() helps on test failures --- pkg/cmd/ssh/ssh_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index f497c1d9..747cd3b1 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -305,9 +305,9 @@ var _ = Describe("SSH Command", func() { Expect(cmd.RunE(cmd, nil)).To(Succeed()) // assert the output - Expect(logs).To(ContainSubstring(bastionName)) - Expect(logs).To(ContainSubstring(bastionHostname)) - Expect(out).To(ContainSubstring(bastionIP)) + Expect(logs.String()).To(ContainSubstring(bastionName)) + Expect(logs.String()).To(ContainSubstring(bastionHostname)) + Expect(out.String()).To(ContainSubstring(bastionIP)) // assert that the bastion has been cleaned up key := types.NamespacedName{Name: bastionName, Namespace: *testProject.Spec.Namespace} @@ -357,9 +357,9 @@ var _ = Describe("SSH Command", func() { // assert output Expect(executedCommands).To(Equal(1)) - Expect(logs).To(ContainSubstring(bastionName)) - Expect(logs).To(ContainSubstring(bastionHostname)) - Expect(out).To(ContainSubstring(bastionIP)) + Expect(logs.String()).To(ContainSubstring(bastionName)) + Expect(logs.String()).To(ContainSubstring(bastionHostname)) + Expect(out.String()).To(ContainSubstring(bastionIP)) // assert that the bastion has been cleaned up key := types.NamespacedName{Name: bastionName, Namespace: *testProject.Spec.Namespace} @@ -411,10 +411,10 @@ var _ = Describe("SSH Command", func() { // assert output Expect(executedCommands).To(Equal(1)) - Expect(logs).To(ContainSubstring(bastionName)) - Expect(logs).To(ContainSubstring(bastionHostname)) - Expect(out).To(ContainSubstring(bastionIP)) - Expect(logs).To(ContainSubstring("node did not yet join the cluster")) + Expect(logs.String()).To(ContainSubstring(bastionName)) + Expect(logs.String()).To(ContainSubstring(bastionHostname)) + Expect(out.String()).To(ContainSubstring(bastionIP)) + Expect(logs.String()).To(ContainSubstring("node did not yet join the cluster")) // assert that the bastion has been cleaned up key := types.NamespacedName{Name: bastionName, Namespace: *testProject.Spec.Namespace} @@ -523,7 +523,7 @@ var _ = Describe("SSH Command", func() { Expect(cmd.RunE(cmd, nil)).To(Succeed()) - Expect(logs).To(ContainSubstring("Bastion is ready, skipping availability check")) + Expect(logs.String()).To(ContainSubstring("Bastion is ready, skipping availability check")) }) It("should not keep alive the bastion", func() { @@ -550,7 +550,7 @@ var _ = Describe("SSH Command", func() { Expect(cmd.RunE(cmd, nil)).To(Succeed()) - Expect(logs).To(ContainSubstring("Bastion host became available.")) + Expect(logs.String()).To(ContainSubstring("Bastion host became available.")) }) }) From 2ff410caa7a263bd912454de417dc76dbe823ddd Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Mon, 20 Mar 2023 17:31:31 +0100 Subject: [PATCH 02/10] use sigs.k8s.io yaml encoder --- go.mod | 2 +- pkg/cmd/base/options.go | 11 ++++------- pkg/cmd/base/options_test.go | 19 ++++++++++--------- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index 175ae655..4ae262b2 100644 --- a/go.mod +++ b/go.mod @@ -27,6 +27,7 @@ require ( k8s.io/klog/v2 v2.70.1 k8s.io/utils v0.0.0-20220728103510-ee6ede2d64ed sigs.k8s.io/controller-runtime v0.13.0 + sigs.k8s.io/yaml v1.3.0 ) require ( @@ -131,7 +132,6 @@ require ( sigs.k8s.io/kustomize/api v0.12.1 // indirect sigs.k8s.io/kustomize/kyaml v0.13.9 // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect - sigs.k8s.io/yaml v1.3.0 // indirect ) replace k8s.io/client-go => k8s.io/client-go v0.25.0 diff --git a/pkg/cmd/base/options.go b/pkg/cmd/base/options.go index 7403e3e3..68b8db8a 100644 --- a/pkg/cmd/base/options.go +++ b/pkg/cmd/base/options.go @@ -13,7 +13,7 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" - "gopkg.in/yaml.v3" + "sigs.k8s.io/yaml" "github.com/gardener/gardenctl-v2/internal/util" ) @@ -75,15 +75,12 @@ func (o *Options) PrintObject(obj interface{}) error { fmt.Fprintf(o.IOStreams.Out, "%v", obj) case "yaml": - yamlEncoder := yaml.NewEncoder(o.IOStreams.Out) - defer yamlEncoder.Close() - - yamlEncoder.SetIndent(2) - - err := yamlEncoder.Encode(&obj) + marshalled, err := yaml.Marshal(&obj) if err != nil { return err } + + fmt.Fprintln(o.IOStreams.Out, string(marshalled)) case "json": marshalled, err := json.MarshalIndent(&obj, "", " ") if err != nil { diff --git a/pkg/cmd/base/options_test.go b/pkg/cmd/base/options_test.go index 9fb595b7..a13a61fb 100644 --- a/pkg/cmd/base/options_test.go +++ b/pkg/cmd/base/options_test.go @@ -24,12 +24,12 @@ import ( var _ = Describe("Base Options", func() { Describe("given an instance", func() { type barType struct { - Baz string + Baz string `json:"baz"` } type fooType struct { - Foo string - Bar barType + Foo string `json:"foo"` + Bar barType `json:"bar"` } var ( @@ -78,9 +78,9 @@ var _ = Describe("Base Options", func() { Expect(options.PrintObject(foo)).To(Succeed()) Expect(buf.String()).To(Equal(fmt.Sprintf( `{ - "Foo": %q, - "Bar": { - "Baz": %q + "foo": %q, + "bar": { + "baz": %q } } `, foo.Foo, foo.Bar.Baz))) @@ -99,11 +99,12 @@ var _ = Describe("Base Options", func() { It("should print with yaml format", func() { Expect(options.PrintObject(foo)).To(Succeed()) Expect(buf.String()).To(Equal(fmt.Sprintf( - `foo: %s -bar: + `bar: baz: %s +foo: %s + `, - foo.Foo, foo.Bar.Baz))) + foo.Bar.Baz, foo.Foo))) }) }) From c77df6c6034c317096ba13a899d7466d65572aec Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Mon, 20 Mar 2023 17:51:52 +0100 Subject: [PATCH 03/10] fix tests --- pkg/cmd/ssh/ssh_test.go | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index 747cd3b1..dbafe9b4 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -587,7 +587,8 @@ var _ = Describe("SSH Command", func() { var _ = Describe("SSH Options", func() { var ( streams util.IOStreams - publicSSHKeyFile string + publicSSHKeyFile ssh.PublicKeyFile + o *ssh.SSHOptions ) BeforeEach(func() { @@ -601,46 +602,46 @@ var _ = Describe("SSH Options", func() { _, err = io.WriteString(tmpFile, "ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDouNkxsNuApuKVIfgL6Yz3Ep+DqX84Yde9DArwLBSWgLnl/pH9AbbcDcAmdB2CPVXAATo4qxK7xprvyyZp52SQRCcAZpAy4D6gAWwAG3OfzrRbxRiB5pQDaaWATSzNbLtoy0ecVwFeTJe2w71q+wxbI7tfxbvo9XbXIN4I0cQy2KLICzkYkQmygGnHztv1Mvi338+sgcG7Gwq2tdSyggDaAggwDIuT39S4/L7QpR27tWH79J4Ls8tTHud2eRbkOcF98vXlQAIzb6w8iHBXylOjMM/oODwoA7V4mtRL9o13AoocvZSsD1UvfOjGxDHuLrCfFXN+/rEw0hEiYo0cnj7F") Expect(err).NotTo(HaveOccurred()) - publicSSHKeyFile = tmpFile.Name() + publicSSHKeyFile = ssh.PublicKeyFile(tmpFile.Name()) + + o = ssh.NewSSHOptions(streams) + o.CIDRs = []string{"8.8.8.8/32"} + o.SSHPublicKeyFile = publicSSHKeyFile }) AfterEach(func() { - Expect(os.Remove(publicSSHKeyFile)).To(Succeed()) + Expect(os.Remove(publicSSHKeyFile.String())).To(Succeed()) }) It("should validate", func() { - o := ssh.NewSSHOptions(streams) - o.CIDRs = []string{"8.8.8.8/32"} - o.SSHPublicKeyFile = publicSSHKeyFile - Expect(o.Validate()).To(Succeed()) }) It("should require a non-zero wait time", func() { - o := ssh.NewSSHOptions(streams) - o.CIDRs = []string{"8.8.8.8/32"} - o.SSHPublicKeyFile = publicSSHKeyFile o.WaitTimeout = 0 Expect(o.Validate()).NotTo(Succeed()) }) Context("no-keepalive", func() { - It("should require non-interactive mode", func() { - o := ssh.NewSSHOptions(streams) + BeforeEach(func() { o.NoKeepalive = true + o.KeepBastion = true + o.Interactive = false + }) + + It("should validate", func() { + Expect(o.Validate()).Should(Succeed()) + }) + It("should require non-interactive mode", func() { o.Interactive = true Expect(o.Validate()).NotTo(Succeed()) }) It("should require keep bastion", func() { - o := ssh.NewSSHOptions(streams) - o.NoKeepalive = true - o.Interactive = false - o.KeepBastion = false Expect(o.Validate()).NotTo(Succeed()) From f39be42890fee98fd6873357743607d661307aa7 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Mon, 20 Mar 2023 18:00:50 +0100 Subject: [PATCH 04/10] introduce types for public and private key file --- pkg/cmd/ssh/options.go | 36 +++++++++++++++++------------------ pkg/cmd/ssh/ssh_test.go | 14 +++++++------- pkg/cmd/ssh/types.go | 42 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 25 deletions(-) create mode 100644 pkg/cmd/ssh/types.go diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index c1039f29..a78b9970 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -270,12 +270,12 @@ type SSHOptions struct { // SSHPublicKeyFile is the full path to the file containing the user's // public SSH key. If not given, gardenctl will create a new temporary keypair. - SSHPublicKeyFile string + SSHPublicKeyFile PublicKeyFile // SSHPrivateKeyFile is the full path to the file containing the user's // private SSH key. This is only set if no key was given and a temporary keypair // was generated. Otherwise gardenctl relies on the user's SSH agent. - SSHPrivateKeyFile string + SSHPrivateKeyFile PrivateKeyFile // generatedSSHKeys is true if the public and private SSH keys have been generated // instead of being provided by the user. This will then be used for the cleanup. @@ -317,7 +317,7 @@ func NewSSHOptions(ioStreams util.IOStreams) *SSHOptions { func (o *SSHOptions) AddFlags(flagSet *pflag.FlagSet) { flagSet.BoolVar(&o.Interactive, "interactive", o.Interactive, "Open an SSH connection instead of just providing the bastion host (only if NODE_NAME is provided).") - flagSet.StringVar(&o.SSHPublicKeyFile, "public-key-file", "", "Path to the file that contains a public SSH key. If not given, a temporary keypair will be generated.") + flagSet.Var(&o.SSHPublicKeyFile, "public-key-file", "Path to the file that contains a public SSH key. If not given, a temporary keypair will be generated.") flagSet.DurationVar(&o.WaitTimeout, "wait-timeout", o.WaitTimeout, "Maximum duration to wait for the bastion to become available.") flagSet.BoolVar(&o.KeepBastion, "keep-bastion", o.KeepBastion, "Do not delete immediately when gardenctl exits (Bastions will be garbage-collected after some time)") flagSet.BoolVar(&o.SkipAvailabilityCheck, "skip-availability-check", o.SkipAvailabilityCheck, "Skip checking for SSH bastion host availability.") @@ -404,7 +404,7 @@ func (o *SSHOptions) Validate() error { } } - content, err := os.ReadFile(o.SSHPublicKeyFile) + content, err := os.ReadFile(o.SSHPublicKeyFile.String()) if err != nil { return fmt.Errorf("invalid SSH public key file: %w", err) } @@ -416,7 +416,7 @@ func (o *SSHOptions) Validate() error { return nil } -func createSSHKeypair(tempDir string, keyName string) (string, string, error) { +func createSSHKeypair(tempDir string, keyName string) (PrivateKeyFile, PublicKeyFile, error) { if keyName == "" { id, err := utils.GenerateRandomString(8) if err != nil { @@ -440,17 +440,17 @@ func createSSHKeypair(tempDir string, keyName string) (string, string, error) { tempDir = os.TempDir() } - privateKeyFile := filepath.Join(tempDir, keyName) - if err := writeKeyFile(privateKeyFile, encodePrivateKey(privateKey)); err != nil { + sshPrivateKeyFile := PrivateKeyFile(filepath.Join(tempDir, keyName)) + if err := writeKeyFile(sshPrivateKeyFile.String(), encodePrivateKey(privateKey)); err != nil { return "", "", fmt.Errorf("failed to write private key: %w", err) } - publicKeyFile := filepath.Join(tempDir, fmt.Sprintf("%s.pub", keyName)) - if err := writeKeyFile(publicKeyFile, encodePublicKey(publicKey)); err != nil { + sshPublicKeyFile := PublicKeyFile(filepath.Join(tempDir, fmt.Sprintf("%s.pub", keyName))) + if err := writeKeyFile(sshPublicKeyFile.String(), encodePublicKey(publicKey)); err != nil { return "", "", fmt.Errorf("failed to write public key: %w", err) } - return privateKeyFile, publicKeyFile, nil + return sshPrivateKeyFile, sshPublicKeyFile, nil } func createSSHPrivateKey() (*rsa.PrivateKey, error) { @@ -610,7 +610,7 @@ func (o *SSHOptions) Run(f util.Factory) error { return fmt.Errorf("failed to get bastion ingress policies: %w", err) } - sshPublicKey, err := os.ReadFile(o.SSHPublicKeyFile) + sshPublicKey, err := os.ReadFile(o.SSHPublicKeyFile.String()) if err != nil { return fmt.Errorf("failed to read SSH public key: %w", err) } @@ -761,11 +761,11 @@ func cleanup(ctx context.Context, o *SSHOptions, gardenClient client.Client, bas } if o.generatedSSHKeys { - if err := os.Remove(o.SSHPublicKeyFile); err != nil { + if err := os.Remove(o.SSHPublicKeyFile.String()); err != nil { logger.Error(err, "Failed to delete SSH public key file", "path", o.SSHPublicKeyFile) } - if err := os.Remove(o.SSHPrivateKeyFile); err != nil { + if err := os.Remove(o.SSHPrivateKeyFile.String()); err != nil { logger.Error(err, "Failed to delete SSH private key file", "path", o.SSHPrivateKeyFile) } } @@ -851,7 +851,7 @@ func waitForBastion(ctx context.Context, o *SSHOptions, gardenClient client.Clie logger := klog.FromContext(ctx) if o.SSHPrivateKeyFile != "" { - privateKeyBytes, err = os.ReadFile(o.SSHPrivateKeyFile) + privateKeyBytes, err = os.ReadFile(o.SSHPrivateKeyFile.String()) if err != nil { return fmt.Errorf("failed to read SSH private key from %q: %w", o.SSHPrivateKeyFile, err) } @@ -905,7 +905,7 @@ func getShootNode(ctx context.Context, o *SSHOptions, shootClient client.Client) func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string) error { bastionAddr := preferredBastionAddress(bastion) - connectCmd := sshCommandLine(o, bastionAddr, nodePrivateKeyFiles, nodeHostname) + connectCmd := sshCommandLine(o.SSHPrivateKeyFile, bastionAddr, nodePrivateKeyFiles, nodeHostname) fmt.Fprintln(o.IOStreams.Out, "You can open additional SSH sessions using the command below:") fmt.Fprintln(o.IOStreams.Out, "") @@ -949,10 +949,10 @@ func isNodeReady(node corev1.Node) bool { return false } -func sshCommandLine(o *SSHOptions, bastionAddr string, nodePrivateKeyFiles []string, nodeName string) string { +func sshCommandLine(sshPrivateKeyFile PrivateKeyFile, bastionAddr string, nodePrivateKeyFiles []string, nodeName string) string { proxyPrivateKeyFlag := "" - if o.SSHPrivateKeyFile != "" { - proxyPrivateKeyFlag = fmt.Sprintf(" -o IdentitiesOnly=yes -i %s", o.SSHPrivateKeyFile) + if sshPrivateKeyFile != "" { + proxyPrivateKeyFlag = fmt.Sprintf(" -o IdentitiesOnly=yes -i %s", sshPrivateKeyFile) } proxyCmd := fmt.Sprintf( diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index dbafe9b4..fa9eaf9f 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -316,10 +316,10 @@ var _ = Describe("SSH Command", func() { Expect(gardenClient.Get(ctx, key, bastion)).NotTo(Succeed()) // assert that no temporary SSH keypair remained on disk - _, err := os.Stat(options.SSHPublicKeyFile) + _, err := os.Stat(options.SSHPublicKeyFile.String()) Expect(err).To(HaveOccurred()) - _, err = os.Stat(options.SSHPrivateKeyFile) + _, err = os.Stat(options.SSHPrivateKeyFile.String()) Expect(err).To(HaveOccurred()) }) @@ -368,10 +368,10 @@ var _ = Describe("SSH Command", func() { Expect(gardenClient.Get(ctx, key, bastion)).NotTo(Succeed()) // assert that no temporary SSH keypair remained on disk - _, err := os.Stat(options.SSHPublicKeyFile) + _, err := os.Stat(options.SSHPublicKeyFile.String()) Expect(err).To(HaveOccurred()) - _, err = os.Stat(options.SSHPrivateKeyFile) + _, err = os.Stat(options.SSHPrivateKeyFile.String()) Expect(err).To(HaveOccurred()) }) @@ -423,10 +423,10 @@ var _ = Describe("SSH Command", func() { Expect(gardenClient.Get(ctx, key, bastion)).NotTo(Succeed()) // assert that no temporary SSH keypair remained on disk - _, err := os.Stat(options.SSHPublicKeyFile) + _, err := os.Stat(options.SSHPublicKeyFile.String()) Expect(err).To(HaveOccurred()) - _, err = os.Stat(options.SSHPrivateKeyFile) + _, err = os.Stat(options.SSHPrivateKeyFile.String()) Expect(err).To(HaveOccurred()) }) @@ -656,7 +656,7 @@ var _ = Describe("SSH Options", func() { }) It("should require a valid public SSH key file", func() { - Expect(os.WriteFile(publicSSHKeyFile, []byte("not a key"), 0o644)).To(Succeed()) + Expect(os.WriteFile(publicSSHKeyFile.String(), []byte("not a key"), 0o644)).To(Succeed()) o := ssh.NewSSHOptions(streams) o.CIDRs = []string{"8.8.8.8/32"} diff --git a/pkg/cmd/ssh/types.go b/pkg/cmd/ssh/types.go new file mode 100644 index 00000000..edff11c6 --- /dev/null +++ b/pkg/cmd/ssh/types.go @@ -0,0 +1,42 @@ +/* +SPDX-FileCopyrightText: 2023 SAP SE or an SAP affiliate company and Gardener contributors + +SPDX-License-Identifier: Apache-2.0 +*/ + +package ssh + +import ( + "fmt" + + "github.com/spf13/pflag" +) + +type PublicKeyFile string + +var ( + _ pflag.Value = (*PublicKeyFile)(nil) + _ fmt.Stringer = (*PublicKeyFile)(nil) +) + +func (s *PublicKeyFile) Set(val string) error { + *s = PublicKeyFile(val) + + return nil +} + +func (s *PublicKeyFile) Type() string { + return "string" +} + +func (s *PublicKeyFile) String() string { + return string(*s) +} + +type PrivateKeyFile string + +var _ fmt.Stringer = (*PrivateKeyFile)(nil) + +func (s *PrivateKeyFile) String() string { + return string(*s) +} From 85c9cafd6490d0e7fcc1dc35791f33d422b784ac Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 21 Mar 2023 11:19:51 +0100 Subject: [PATCH 05/10] reduce unnecessary complexity --- pkg/cmd/ssh/options.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index a78b9970..555b3d2c 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -665,17 +665,12 @@ func (o *SSHOptions) Run(f util.Factory) error { logger.Info("Waiting for bastion to be ready…", "waitTimeout", o.WaitTimeout) err = waitForBastion(ctx, o, gardenClient.RuntimeClient(), bastion) - if err == wait.ErrWaitTimeout { - logger.Info("Timed out waiting for the bastion to be ready.") + return errors.New("timed out waiting for the bastion to be ready") } else if err != nil { - logger.Error(err, "An error occurred while waiting for the bastion to be ready.") + return fmt.Errorf("an error occurred while waiting for the bastion to be ready: %w", err) } - if err != nil { - // actual error has already been printed - return errors.New("precondition failed") - } ingress := bastion.Status.Ingress printAddr := "" From 21e528a1f4f0705178c8d37e47a48ed04bdb7a47 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 21 Mar 2023 11:23:37 +0100 Subject: [PATCH 06/10] extract to String() --- pkg/cmd/ssh/options.go | 12 +----------- pkg/cmd/ssh/types.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index 555b3d2c..5d0fcf18 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -671,20 +671,10 @@ func (o *SSHOptions) Run(f util.Factory) error { return fmt.Errorf("an error occurred while waiting for the bastion to be ready: %w", err) } + logger.Info("Bastion host became available.", "address", toAdress(bastion.Status.Ingress).String()) - ingress := bastion.Status.Ingress - printAddr := "" - switch { - case ingress.Hostname != "" && ingress.IP != "": - printAddr = fmt.Sprintf("%s (%s)", ingress.IP, ingress.Hostname) - case ingress.Hostname != "": - printAddr = ingress.Hostname - default: - printAddr = ingress.IP - } - logger.Info("Bastion host became available.", "address", printAddr) if o.NoKeepalive { return nil diff --git a/pkg/cmd/ssh/types.go b/pkg/cmd/ssh/types.go index edff11c6..598ce265 100644 --- a/pkg/cmd/ssh/types.go +++ b/pkg/cmd/ssh/types.go @@ -10,6 +10,7 @@ import ( "fmt" "github.com/spf13/pflag" + corev1 "k8s.io/api/core/v1" ) type PublicKeyFile string @@ -40,3 +41,32 @@ var _ fmt.Stringer = (*PrivateKeyFile)(nil) func (s *PrivateKeyFile) String() string { return string(*s) } + +// Address holds information about an IP address and hostname. +type Address struct { + Hostname string `json:"hostname"` + IP string `json:"ip"` +} + +var _ fmt.Stringer = &Address{} +func toAdress(ingress *corev1.LoadBalancerIngress) *Address { + if ingress == nil { + return nil + } + + return &Address{ + Hostname: ingress.Hostname, + IP: ingress.IP, + } +} + +func (a *Address) String() string { + switch { + case a.Hostname != "" && a.IP != "": + return fmt.Sprintf("%s (%s)", a.IP, a.Hostname) + case a.Hostname != "": + return a.Hostname + default: + return a.IP + } +} From 7a470a1c7fbbceafc7c78729a43ca53d82c918e0 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 21 Mar 2023 11:39:59 +0100 Subject: [PATCH 07/10] add output flag --- pkg/cmd/base/options.go | 7 +- pkg/cmd/ssh/export_test.go | 5 +- pkg/cmd/ssh/options.go | 151 +++++++++---------------------- pkg/cmd/ssh/ssh_test.go | 61 +++++++++++-- pkg/cmd/ssh/types.go | 176 +++++++++++++++++++++++++++++++++++++ 5 files changed, 280 insertions(+), 120 deletions(-) diff --git a/pkg/cmd/base/options.go b/pkg/cmd/base/options.go index 68b8db8a..1f16eb01 100644 --- a/pkg/cmd/base/options.go +++ b/pkg/cmd/base/options.go @@ -72,8 +72,11 @@ func (o *Options) AddFlags(flags *pflag.FlagSet) { func (o *Options) PrintObject(obj interface{}) error { switch o.Output { case "": - fmt.Fprintf(o.IOStreams.Out, "%v", obj) - + if _, ok := obj.(fmt.Stringer); ok { + fmt.Fprintf(o.IOStreams.Out, "%s", obj) + } else { + fmt.Fprintf(o.IOStreams.Out, "%v", obj) + } case "yaml": marshalled, err := yaml.Marshal(&obj) if err != nil { diff --git a/pkg/cmd/ssh/export_test.go b/pkg/cmd/ssh/export_test.go index a680ab8c..9dba57d4 100644 --- a/pkg/cmd/ssh/export_test.go +++ b/pkg/cmd/ssh/export_test.go @@ -10,9 +10,6 @@ import ( "context" "os" "time" - - operationsv1alpha1 "github.com/gardener/gardener/pkg/apis/operations/v1alpha1" - "sigs.k8s.io/controller-runtime/pkg/client" ) func SetBastionAvailabilityChecker(f func(hostname string, privateKey []byte) error) { @@ -46,6 +43,6 @@ func SetKeepAliveInterval(d time.Duration) { keepAliveInterval = d } -func SetWaitForSignal(f func(ctx context.Context, o *SSHOptions, shootClient client.Client, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string, signalChan <-chan struct{}) error) { +func SetWaitForSignal(f func(ctx context.Context, o *SSHOptions, signalChan <-chan struct{})) { waitForSignal = f } diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index 5d0fcf18..5de98bcd 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -40,10 +40,8 @@ import ( networkingv1 "k8s.io/api/networking/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/cli-runtime/pkg/printers" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" @@ -152,102 +150,15 @@ var ( return cmd.Run() } - // waitForSignal informs the user about their SSHOptions and keeps the + // waitForSignal informs the user how to stop gardenctl and keeps the // bastion alive until gardenctl exits. - waitForSignal = func(ctx context.Context, o *SSHOptions, shootClient client.Client, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string, signalChan <-chan struct{}) error { - if nodeHostname == "" { - nodeHostname = "IP_OR_HOSTNAME" - - nodes, err := getNodes(ctx, shootClient) - if err != nil { - return fmt.Errorf("failed to list shoot cluster nodes: %w", err) - } - - table := &metav1beta1.Table{ - ColumnDefinitions: []metav1.TableColumnDefinition{ - { - Name: "Node Name", - Type: "string", - Format: "name", - }, - { - Name: "Status", - Type: "string", - }, - { - Name: "IP", - Type: "string", - }, - { - Name: "Hostname", - Type: "string", - }, - }, - Rows: []metav1.TableRow{}, - } - - for _, node := range nodes { - ip := "" - hostname := "" - status := "Ready" - - if !isNodeReady(node) { - status = "Not Ready" - } - - for _, addr := range node.Status.Addresses { - switch addr.Type { - case corev1.NodeInternalIP: - ip = addr.Address - - case corev1.NodeInternalDNS: - hostname = addr.Address - - // internal names have priority, as we jump via a bastion host, - // but in case the cloud provider does not offer internal IPs, - // we fallback to external values - - case corev1.NodeExternalIP: - if ip == "" { - ip = addr.Address - } - - case corev1.NodeExternalDNS: - if hostname == "" { - hostname = addr.Address - } - } - } - - table.Rows = append(table.Rows, metav1.TableRow{ - Cells: []interface{}{node.Name, status, ip, hostname}, - }) - } - - fmt.Fprintln(o.IOStreams.Out, "The shoot cluster has the following nodes:") - fmt.Fprintln(o.IOStreams.Out, "") - - printer := printers.NewTablePrinter(printers.PrintOptions{}) - if err := printer.PrintObj(table, o.IOStreams.Out); err != nil { - return fmt.Errorf("failed to output node table: %w", err) - } - - fmt.Fprintln(o.IOStreams.Out, "") + waitForSignal = func(ctx context.Context, o *SSHOptions, signalChan <-chan struct{}) { + if o.Output == "" { + fmt.Fprintln(o.IOStreams.Out, "Press Ctrl-C to stop gardenctl, after which the bastion will be removed.") } - bastionAddr := preferredBastionAddress(bastion) - connectCmd := sshCommandLine(o, bastionAddr, nodePrivateKeyFiles, nodeHostname) - - fmt.Fprintln(o.IOStreams.Out, "Connect to shoot nodes by using the bastion as a proxy/jump host, for example:") - fmt.Fprintln(o.IOStreams.Out, "") - fmt.Fprintln(o.IOStreams.Out, connectCmd) - fmt.Fprintln(o.IOStreams.Out, "") - - fmt.Fprintln(o.IOStreams.Out, "Press Ctrl-C to stop gardenctl, after which the bastion will be removed.") - + // keep the bastion alive until gardenctl exits <-signalChan - - return nil } ) @@ -322,10 +233,14 @@ func (o *SSHOptions) AddFlags(flagSet *pflag.FlagSet) { flagSet.BoolVar(&o.KeepBastion, "keep-bastion", o.KeepBastion, "Do not delete immediately when gardenctl exits (Bastions will be garbage-collected after some time)") flagSet.BoolVar(&o.SkipAvailabilityCheck, "skip-availability-check", o.SkipAvailabilityCheck, "Skip checking for SSH bastion host availability.") flagSet.BoolVar(&o.NoKeepalive, "no-keepalive", o.NoKeepalive, "Exit after the bastion host became available without keeping the bastion alive or establishing an SSH connection. Note that this flag requires the flags --interactive=false and --keep-bastion to be set") + o.Options.AddFlags(flagSet) } // Complete adapts from the command line args to the data required. func (o *SSHOptions) Complete(f util.Factory, cmd *cobra.Command, args []string) error { + ctx := f.Context() + logger := klog.FromContext(ctx) + if err := o.AccessConfig.Complete(f, cmd, args); err != nil { return err } @@ -352,6 +267,12 @@ func (o *SSHOptions) Complete(f util.Factory, cmd *cobra.Command, args []string) o.NodeName = strings.TrimSpace(args[0]) } + if o.NodeName == "" && o.Interactive { + logger.V(4).Info("no node name given, switching to non-interactive mode") + + o.Interactive = false + } + return nil } @@ -404,6 +325,12 @@ func (o *SSHOptions) Validate() error { } } + if o.Output != "" { + if o.Interactive { + return errors.New("set --interactive=false when using the output flag") + } + } + content, err := os.ReadFile(o.SSHPublicKeyFile.String()) if err != nil { return fmt.Errorf("invalid SSH public key file: %w", err) @@ -673,15 +600,31 @@ func (o *SSHOptions) Run(f util.Factory) error { logger.Info("Bastion host became available.", "address", toAdress(bastion.Status.Ingress).String()) + if !o.Interactive { + var nodes []corev1.Node + if nodeHostname == "" { + nodes, err = getNodes(ctx, shootClient) + if err != nil { + return fmt.Errorf("failed to list shoot cluster nodes: %w", err) + } + } + connectInformation, err := NewConnectInformation(bastion, nodeHostname, o.SSHPublicKeyFile, o.SSHPrivateKeyFile, nodePrivateKeyFiles, nodes) + if err != nil { + return err + } + if err := o.PrintObject(connectInformation); err != nil { + return err + } - if o.NoKeepalive { - return nil - } + if o.NoKeepalive { + return nil + } - if nodeHostname == "" || !o.Interactive { - return waitForSignal(ctx, o, shootClient, bastion, nodeHostname, nodePrivateKeyFiles, ctx.Done()) + waitForSignal(ctx, o, ctx.Done()) + + return nil } return remoteShell(ctx, o, bastion, nodeHostname, nodePrivateKeyFiles) @@ -924,16 +867,6 @@ func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1 return execCommand(ctx, "ssh", args, o) } -func isNodeReady(node corev1.Node) bool { - for _, cond := range node.Status.Conditions { - if cond.Type == corev1.NodeReady { - return cond.Status == corev1.ConditionTrue - } - } - - return false -} - func sshCommandLine(sshPrivateKeyFile PrivateKeyFile, bastionAddr string, nodePrivateKeyFiles []string, nodeName string) string { proxyPrivateKeyFlag := "" if sshPrivateKeyFile != "" { diff --git a/pkg/cmd/ssh/ssh_test.go b/pkg/cmd/ssh/ssh_test.go index fa9eaf9f..f64732d6 100644 --- a/pkg/cmd/ssh/ssh_test.go +++ b/pkg/cmd/ssh/ssh_test.go @@ -8,6 +8,7 @@ package ssh_test import ( "context" + "encoding/json" "errors" "fmt" "io" @@ -491,13 +492,15 @@ var _ = Describe("SSH Command", func() { }) // Once the waitForSignal function is called we delete the bastion - ssh.SetWaitForSignal(func(ctx context.Context, o *ssh.SSHOptions, shootClient client.Client, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string, signalChan <-chan struct{}) error { + ssh.SetWaitForSignal(func(ctx context.Context, o *ssh.SSHOptions, signalChan <-chan struct{}) { By("deleting bastion") + bastion := &operationsv1alpha1.Bastion{} + key := types.NamespacedName{Name: bastionName, Namespace: *testProject.Spec.Namespace} + Expect(gardenClient.Get(ctx, key, bastion)).To(Succeed()) + Expect(gardenClient.Delete(ctx, bastion)).To(Succeed()) <-signalChan - - return nil }) // let the magic happen @@ -534,11 +537,36 @@ var _ = Describe("SSH Command", func() { cmd := ssh.NewCmdSSH(factory, options) - ssh.SetWaitForSignal(func(ctx context.Context, o *ssh.SSHOptions, shootClient client.Client, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string, signalChan <-chan struct{}) error { + ssh.SetWaitForSignal(func(ctx context.Context, o *ssh.SSHOptions, signalChan <-chan struct{}) { + Fail("this function should not be executed as of NoKeepalive = true") + }) + ssh.SetExecCommand(func(ctx context.Context, command string, args []string, o *ssh.SSHOptions) error { err := errors.New("this function should not be executed as of NoKeepalive = true") Fail(err.Error()) return err }) + + // simulate an external controller processing the bastion and proving a successful status + go waitForBastionThenSetBastionReady(ctx, gardenClient, bastionName, *testProject.Spec.Namespace, bastionHostname, bastionIP) + + Expect(cmd.RunE(cmd, nil)).To(Succeed()) + + Expect(logs.String()).To(ContainSubstring("Bastion host became available.")) + }) + + It("should output as json", func() { + options := ssh.NewSSHOptions(streams) + options.NoKeepalive = true + options.KeepBastion = true + options.Interactive = false + + options.Output = "json" + + cmd := ssh.NewCmdSSH(factory, options) + + ssh.SetWaitForSignal(func(ctx context.Context, o *ssh.SSHOptions, signalChan <-chan struct{}) { + Fail("this function should not be executed as of NoKeepalive = true") + }) ssh.SetExecCommand(func(ctx context.Context, command string, args []string, o *ssh.SSHOptions) error { err := errors.New("this function should not be executed as of NoKeepalive = true") Fail(err.Error()) @@ -550,7 +578,13 @@ var _ = Describe("SSH Command", func() { Expect(cmd.RunE(cmd, nil)).To(Succeed()) - Expect(logs.String()).To(ContainSubstring("Bastion host became available.")) + var info ssh.ConnectInformation + Expect(json.Unmarshal([]byte(out.String()), &info)).To(Succeed()) + Expect(info.Bastion.Name).To(Equal(bastionName)) + 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.NodePrivateKeyFiles).NotTo(BeEmpty()) }) }) @@ -648,6 +682,23 @@ var _ = Describe("SSH Options", func() { }) }) + Describe("output flag not empty", func() { + BeforeEach(func() { + o.Output = "yaml" // or json - does not matter + + o.Interactive = false + }) + + It("should validate", func() { + Expect(o.Validate()).Should(Succeed()) + }) + + It("should require non-interactive mode", func() { + o.Interactive = true + + Expect(o.Validate()).NotTo(Succeed()) + }) + }) It("should require a public SSH key file", func() { o := ssh.NewSSHOptions(streams) o.CIDRs = []string{"8.8.8.8/32"} diff --git a/pkg/cmd/ssh/types.go b/pkg/cmd/ssh/types.go index 598ce265..3225d924 100644 --- a/pkg/cmd/ssh/types.go +++ b/pkg/cmd/ssh/types.go @@ -7,10 +7,16 @@ SPDX-License-Identifier: Apache-2.0 package ssh import ( + "bytes" "fmt" + operationsv1alpha1 "github.com/gardener/gardener/pkg/apis/operations/v1alpha1" "github.com/spf13/pflag" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1beta1 "k8s.io/apimachinery/pkg/apis/meta/v1beta1" + "k8s.io/cli-runtime/pkg/printers" + "k8s.io/klog/v2" ) type PublicKeyFile string @@ -42,6 +48,50 @@ func (s *PrivateKeyFile) String() string { return string(*s) } +// ConnectInformation holds connect information required to establish an SSH connection +// to Shoot worker nodes. +type ConnectInformation struct { + // Bastion holds information about the bastion host used to connect to the worker nodes. + Bastion Bastion `json:"bastion"` + + // NodeHostname is the name of the Shoot cluster node that the user wants to connect to. + NodeHostname string `json:"nodeHostname,omitempty"` + + // NodePrivateKeyFiles is a list of file paths containing the private SSH keys for the worker nodes. + NodePrivateKeyFiles []string `json:"nodePrivateKeyFiles"` + + // Nodes is a list of Node objects containing information about the worker nodes. + Nodes []Node `json:"nodes"` +} + +var _ fmt.Stringer = &ConnectInformation{} + +// Bastion holds information about the bastion host used to connect to the worker nodes. +type Bastion struct { + // Name is the name of the Bastion resource. + Name string `json:"name"` + // Namespace is the namespace of the Bastion resource. + Namespace string `json:"namespace"` + // PreferredAddress is the preferred IP address or hostname to use when connecting to the bastion host. + PreferredAddress string `json:"preferredAddress"` + // Address holds information about the IP address and hostname of the bastion host. + Address + // SSHPublicKeyFile is the full path to the file containing the public SSH key. + SSHPublicKeyFile PublicKeyFile `json:"publicKeyFile"` + // SSHPrivateKeyFile is the full path to the file containing the private SSH key. + SSHPrivateKeyFile PrivateKeyFile `json:"privateKeyFile"` +} + +// Node holds information about a worker node. +type Node struct { + // Name is the name of the worker node. + Name string `json:"name"` + // Status is the current status of the worker node. + Status string `json:"status"` + // Address holds information about the IP address and hostname of the worker node. + Address +} + // Address holds information about an IP address and hostname. type Address struct { Hostname string `json:"hostname"` @@ -49,6 +99,132 @@ type Address struct { } var _ fmt.Stringer = &Address{} + +func NewConnectInformation(bastion *operationsv1alpha1.Bastion, nodeHostname string, sshPublicKeyFile PublicKeyFile, sshPrivateKeyFile PrivateKeyFile, nodePrivateKeyFiles []string, nodes []corev1.Node) (*ConnectInformation, error) { + var nodeList []Node + + for _, node := range nodes { + n := Node{} + n.Name = node.Name + n.Status = "Ready" + + if !isNodeReady(node) { + n.Status = "Not Ready" + } + + for _, addr := range node.Status.Addresses { + switch addr.Type { + case corev1.NodeInternalIP: + n.IP = addr.Address + + case corev1.NodeInternalDNS: + n.Hostname = addr.Address + + // internal names have priority, as we jump via a bastion host, + // but in case the cloud provider does not offer internal IPs, + // we fallback to external values + + case corev1.NodeExternalIP: + if n.IP == "" { + n.IP = addr.Address + } + + case corev1.NodeExternalDNS: + if n.Hostname == "" { + n.Hostname = addr.Address + } + } + } + + nodeList = append(nodeList, n) + } + + return &ConnectInformation{ + Bastion: Bastion{ + Name: bastion.Name, + Namespace: bastion.Namespace, + PreferredAddress: preferredBastionAddress(bastion), + Address: Address{ + IP: bastion.Status.Ingress.IP, + Hostname: bastion.Status.Ingress.Hostname, + }, + SSHPublicKeyFile: sshPublicKeyFile, + SSHPrivateKeyFile: sshPrivateKeyFile, + }, + NodeHostname: nodeHostname, + NodePrivateKeyFiles: nodePrivateKeyFiles, + Nodes: nodeList, + }, nil +} + +func (p *ConnectInformation) String() string { + buf := bytes.Buffer{} + + nodeHostname := p.NodeHostname + if nodeHostname == "" { + nodeHostname = "IP_OR_HOSTNAME" + + table := &metav1beta1.Table{ + ColumnDefinitions: []metav1.TableColumnDefinition{ + { + Name: "Node Name", + Type: "string", + Format: "name", + }, + { + Name: "Status", + Type: "string", + }, + { + Name: "IP", + Type: "string", + }, + { + Name: "Hostname", + Type: "string", + }, + }, + Rows: []metav1.TableRow{}, + } + + for _, node := range p.Nodes { + table.Rows = append(table.Rows, metav1.TableRow{ + Cells: []interface{}{node.Name, node.Status, node.IP, node.Hostname}, + }) + } + + fmt.Fprintln(&buf, "The shoot cluster has the following nodes:") + fmt.Fprintln(&buf, "") + + printer := printers.NewTablePrinter(printers.PrintOptions{}) + if err := printer.PrintObj(table, &buf); err != nil { + klog.Background().Error(err, "failed to output node table: %w") + return "" + } + + fmt.Fprintln(&buf, "") + } + + connectCmd := sshCommandLine(p.Bastion.SSHPrivateKeyFile, p.Bastion.PreferredAddress, p.NodePrivateKeyFiles, nodeHostname) + + fmt.Fprintln(&buf, "Connect to shoot nodes by using the bastion as a proxy/jump host, for example:") + fmt.Fprintln(&buf, "") + fmt.Fprintln(&buf, connectCmd) + fmt.Fprintln(&buf, "") + + return buf.String() +} + +func isNodeReady(node corev1.Node) bool { + for _, cond := range node.Status.Conditions { + if cond.Type == corev1.NodeReady { + return cond.Status == corev1.ConditionTrue + } + } + + return false +} + func toAdress(ingress *corev1.LoadBalancerIngress) *Address { if ingress == nil { return nil From ce533350cc8c36bd259da2919ea0bdcfcf281b88 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 21 Mar 2023 11:40:09 +0100 Subject: [PATCH 08/10] make gen-markdown --- docs/help/gardenctl_ssh.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/help/gardenctl_ssh.md b/docs/help/gardenctl_ssh.md index fce3709c..2818a568 100644 --- a/docs/help/gardenctl_ssh.md +++ b/docs/help/gardenctl_ssh.md @@ -16,6 +16,7 @@ gardenctl ssh [NODE_NAME] [flags] --interactive Open an SSH connection instead of just providing the bastion host (only if NODE_NAME is provided). (default true) --keep-bastion Do not delete immediately when gardenctl exits (Bastions will be garbage-collected after some time) --no-keepalive Exit after the bastion host became available without keeping the bastion alive or establishing an SSH connection. Note that this flag requires the flags --interactive=false and --keep-bastion to be set + -o, --output string One of 'yaml' or 'json'. --project string target the given project --public-key-file string Path to the file that contains a public SSH key. If not given, a temporary keypair will be generated. --seed string target the given seed cluster From cadff5a303525c01b744de557573d6e4fb540c73 Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 28 Mar 2023 18:12:02 +0200 Subject: [PATCH 09/10] PR feedback - use PrivateKeyFile type --- pkg/cmd/ssh/options.go | 14 +++++++------- pkg/cmd/ssh/types.go | 4 ++-- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/ssh/options.go b/pkg/cmd/ssh/options.go index 5de98bcd..e32a135e 100644 --- a/pkg/cmd/ssh/options.go +++ b/pkg/cmd/ssh/options.go @@ -498,7 +498,7 @@ func (o *SSHOptions) Run(f util.Factory) error { } // save the keys into temporary files that we try to clean up when exiting - nodePrivateKeyFiles := []string{} + var nodePrivateKeyFiles []PrivateKeyFile for _, pk := range nodePrivateKeys { filename, err := writeToTemporaryFile(pk) @@ -506,7 +506,7 @@ func (o *SSHOptions) Run(f util.Factory) error { return err } - nodePrivateKeyFiles = append(nodePrivateKeyFiles, filename) + nodePrivateKeyFiles = append(nodePrivateKeyFiles, PrivateKeyFile(filename)) } shootClient, err := manager.ShootClient(ctx, sshTarget) @@ -678,7 +678,7 @@ func printTargetInformation(logger klog.Logger, t target.Target) { logger.Info("Preparing SSH access", "target", target, "garden", t.GardenName()) } -func cleanup(ctx context.Context, o *SSHOptions, gardenClient client.Client, bastion *operationsv1alpha1.Bastion, nodePrivateKeyFiles []string) { +func cleanup(ctx context.Context, o *SSHOptions, gardenClient client.Client, bastion *operationsv1alpha1.Bastion, nodePrivateKeyFiles []PrivateKeyFile) { logger := klog.FromContext(ctx) if !o.KeepBastion { @@ -702,7 +702,7 @@ func cleanup(ctx context.Context, o *SSHOptions, gardenClient client.Client, bas // these files remaining, the user would not be able to use the SSH // command we provided to connect to the shoot nodes for _, filename := range nodePrivateKeyFiles { - if err := os.Remove(filename); err != nil { + if err := os.Remove(filename.String()); err != nil { logger.Error(err, "Failed to delete node private key", "path", filename) } } @@ -831,7 +831,7 @@ func getShootNode(ctx context.Context, o *SSHOptions, shootClient client.Client) return node, nil } -func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []string) error { +func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1.Bastion, nodeHostname string, nodePrivateKeyFiles []PrivateKeyFile) error { bastionAddr := preferredBastionAddress(bastion) connectCmd := sshCommandLine(o.SSHPrivateKeyFile, bastionAddr, nodePrivateKeyFiles, nodeHostname) @@ -859,7 +859,7 @@ func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1 } for _, file := range nodePrivateKeyFiles { - args = append(args, "-i", file) + args = append(args, "-i", file.String()) } args = append(args, fmt.Sprintf("%s@%s", SSHNodeUsername, nodeHostname)) @@ -867,7 +867,7 @@ func remoteShell(ctx context.Context, o *SSHOptions, bastion *operationsv1alpha1 return execCommand(ctx, "ssh", args, o) } -func sshCommandLine(sshPrivateKeyFile PrivateKeyFile, bastionAddr string, nodePrivateKeyFiles []string, nodeName string) string { +func sshCommandLine(sshPrivateKeyFile PrivateKeyFile, bastionAddr string, nodePrivateKeyFiles []PrivateKeyFile, nodeName string) string { proxyPrivateKeyFlag := "" if sshPrivateKeyFile != "" { proxyPrivateKeyFlag = fmt.Sprintf(" -o IdentitiesOnly=yes -i %s", sshPrivateKeyFile) diff --git a/pkg/cmd/ssh/types.go b/pkg/cmd/ssh/types.go index 3225d924..fa1931bf 100644 --- a/pkg/cmd/ssh/types.go +++ b/pkg/cmd/ssh/types.go @@ -58,7 +58,7 @@ type ConnectInformation struct { NodeHostname string `json:"nodeHostname,omitempty"` // NodePrivateKeyFiles is a list of file paths containing the private SSH keys for the worker nodes. - NodePrivateKeyFiles []string `json:"nodePrivateKeyFiles"` + NodePrivateKeyFiles []PrivateKeyFile `json:"nodePrivateKeyFiles"` // Nodes is a list of Node objects containing information about the worker nodes. Nodes []Node `json:"nodes"` @@ -100,7 +100,7 @@ type Address struct { var _ fmt.Stringer = &Address{} -func NewConnectInformation(bastion *operationsv1alpha1.Bastion, nodeHostname string, sshPublicKeyFile PublicKeyFile, sshPrivateKeyFile PrivateKeyFile, nodePrivateKeyFiles []string, nodes []corev1.Node) (*ConnectInformation, error) { +func NewConnectInformation(bastion *operationsv1alpha1.Bastion, nodeHostname string, sshPublicKeyFile PublicKeyFile, sshPrivateKeyFile PrivateKeyFile, nodePrivateKeyFiles []PrivateKeyFile, nodes []corev1.Node) (*ConnectInformation, error) { var nodeList []Node for _, node := range nodes { From cfb2bcc20e3c9dc843f9505ced1e45ecb663d35c Mon Sep 17 00:00:00 2001 From: Peter Sutter Date: Tue, 28 Mar 2023 18:12:45 +0200 Subject: [PATCH 10/10] PR feedback - consistently use sigs.k8s.io/yaml --- go.mod | 2 +- pkg/ac/access_restriction.go | 14 ++++++------ pkg/cmd/config/config_test.go | 2 +- pkg/config/config.go | 36 +++++++++++++++++-------------- pkg/config/config_suite_test.go | 13 ----------- pkg/config/config_test.go | 38 +++++++++++++++++++++++++++++++++ pkg/target/target.go | 10 ++++----- pkg/target/target_provider.go | 19 +++++++++++------ 8 files changed, 84 insertions(+), 50 deletions(-) diff --git a/go.mod b/go.mod index 4ae262b2..775acfb6 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,6 @@ require ( github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.15.0 golang.org/x/crypto v0.7.0 - gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.25.0 k8s.io/apimachinery v0.25.0 k8s.io/apiserver v0.25.0 @@ -120,6 +119,7 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/ini.v1 v1.67.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect istio.io/api v0.0.0-20221013011440-bc935762d2b9 // indirect istio.io/client-go v1.15.3 // indirect k8s.io/apiextensions-apiserver v0.25.0 // indirect diff --git a/pkg/ac/access_restriction.go b/pkg/ac/access_restriction.go index 316cdbeb..24bbd378 100644 --- a/pkg/ac/access_restriction.go +++ b/pkg/ac/access_restriction.go @@ -20,23 +20,23 @@ import ( // AccessRestriction is used to define an access restriction. type AccessRestriction struct { // Key is the identifier of an access restriction - Key string `yaml:"key,omitempty" json:"key,omitempty"` + Key string `json:"key,omitempty"` // NotifyIf controls which value the annotation must have for a notification to be sent - NotifyIf bool `yaml:"notifyIf,omitempty" json:"notifyIf,omitempty"` + NotifyIf bool `json:"notifyIf,omitempty"` // Msg is the notification text that is sent - Msg string `yaml:"msg,omitempty" json:"msg,omitempty"` + Msg string `json:"msg,omitempty"` // Options is a list of access restriction options - Options []AccessRestrictionOption `yaml:"options,omitempty" json:"options,omitempty"` + Options []AccessRestrictionOption `json:"options,omitempty"` } // AccessRestrictionOption is used to define an access restriction option. type AccessRestrictionOption struct { // Key is the identifier of an access restriction option - Key string `yaml:"key,omitempty" json:"key,omitempty"` + Key string `json:"key,omitempty"` // NotifyIf controls which value the annotation must have for a notification to be sent - NotifyIf bool `yaml:"notifyIf,omitempty" json:"notifyIf,omitempty"` + NotifyIf bool `json:"notifyIf,omitempty"` // Msg is the notification text that is sent - Msg string `yaml:"msg,omitempty" json:"msg,omitempty"` + Msg string `json:"msg,omitempty"` } // AccessRestrictionHandler is a function that should display a single AccessRestrictionMessage to the user. diff --git a/pkg/cmd/config/config_test.go b/pkg/cmd/config/config_test.go index 1e0c57e5..b387035c 100644 --- a/pkg/cmd/config/config_test.go +++ b/pkg/cmd/config/config_test.go @@ -12,7 +12,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/spf13/cobra" - "gopkg.in/yaml.v3" + "sigs.k8s.io/yaml" cmdconfig "github.com/gardener/gardenctl-v2/pkg/cmd/config" "github.com/gardener/gardenctl-v2/pkg/config" diff --git a/pkg/config/config.go b/pkg/config/config.go index 66f4bfbd..abc8ddc0 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -15,10 +15,10 @@ import ( "strconv" "github.com/mitchellh/go-homedir" - "gopkg.in/yaml.v3" "k8s.io/client-go/tools/clientcmd" clientcmdapi "k8s.io/client-go/tools/clientcmd/api" "k8s.io/klog/v2" + "sigs.k8s.io/yaml" "github.com/gardener/gardenctl-v2/pkg/ac" ) @@ -26,33 +26,33 @@ import ( // Config holds the gardenctl configuration. type Config struct { // Filename is the name of the gardenctl configuration file - Filename string `yaml:"-" json:"-"` + Filename string `json:"-"` // LinkKubeconfig defines if kubeconfig is symlinked with the target - LinkKubeconfig *bool `yaml:"linkKubeconfig,omitempty" json:"linkKubeconfig,omitempty"` + LinkKubeconfig *bool `json:"linkKubeconfig,omitempty"` // Gardens is a list of known Garden clusters - Gardens []Garden `yaml:"gardens" json:"gardens"` + Gardens []Garden `json:"gardens"` } // Garden represents one garden cluster. type Garden struct { // Name is a unique identifier of this Garden that can be used to target this Garden - Name string `yaml:"identity" json:"identity"` + Name string `json:"identity"` // Alias is a unique identifier of this Garden that can be used as an alternate name to target this Garden // +optional - Alias string `yaml:"name,omitempty" json:"name,omitempty"` + Alias string `json:"name,omitempty"` // Kubeconfig holds the path for the kubeconfig of the garden cluster - Kubeconfig string `yaml:"kubeconfig" json:"kubeconfig"` + Kubeconfig string `json:"kubeconfig"` // Context overrides the current-context of the garden cluster kubeconfig // +optional - Context string `yaml:"context,omitempty" json:"context,omitempty"` + Context string `json:"context,omitempty"` // Patterns is a list of regex patterns that can be defined to use custom input formats for targeting // Use named capturing groups to match target values. // Supported capturing groups: project, namespace, shoot // +optional - Patterns []string `yaml:"patterns,omitempty" json:"patterns,omitempty"` + Patterns []string `json:"patterns,omitempty"` // AccessRestrictions is a list of access restriction definitions // +optional - AccessRestrictions []ac.AccessRestriction `yaml:"accessRestrictions,omitempty" json:"accessRestrictions,omitempty"` + AccessRestrictions []ac.AccessRestriction `json:"accessRestrictions,omitempty"` } // LoadFromFile parses a gardenctl config file and returns a Config struct. @@ -75,7 +75,12 @@ func LoadFromFile(filename string) (*Config, error) { } if stat.Size() > 0 { - if err := yaml.NewDecoder(f).Decode(config); err != nil { + buf, err := os.ReadFile(filename) + if err != nil { + return nil, fmt.Errorf("failed to read config file: %w", err) + } + + if err = yaml.Unmarshal(buf, config); err != nil { return nil, fmt.Errorf("failed to decode as YAML: %w", err) } @@ -147,14 +152,13 @@ func (config *Config) Save() error { return fmt.Errorf("failed to create directory: %w", err) } - f, err := os.OpenFile(config.Filename, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600) + buf, err := yaml.Marshal(config) if err != nil { - return fmt.Errorf("failed to create file: %w", err) + return fmt.Errorf("failed to encode as YAML: %w", err) } - defer f.Close() - if err := yaml.NewEncoder(f).Encode(config); err != nil { - return fmt.Errorf("failed to encode as YAML: %w", err) + if err := os.WriteFile(config.Filename, buf, 0o600); err != nil { + return fmt.Errorf("failed to write file: %w", err) } return nil diff --git a/pkg/config/config_suite_test.go b/pkg/config/config_suite_test.go index c3e77085..33a093ea 100644 --- a/pkg/config/config_suite_test.go +++ b/pkg/config/config_suite_test.go @@ -7,7 +7,6 @@ SPDX-License-Identifier: Apache-2.0 package config_test import ( - "os" "testing" . "github.com/onsi/ginkgo/v2" @@ -18,15 +17,3 @@ func TestConfiguration(t *testing.T) { RegisterFailHandler(Fail) RunSpecs(t, "Config Test Suite") } - -var gardenHomeDir string - -var _ = BeforeSuite(func() { - dir, err := os.MkdirTemp("", "garden-*") - Expect(err).NotTo(HaveOccurred()) - gardenHomeDir = dir -}) - -var _ = AfterSuite(func() { - Expect(os.RemoveAll(gardenHomeDir)).To(Succeed()) -}) diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f2c77ebd..addb3436 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -20,6 +20,7 @@ import ( var _ = Describe("Config", func() { var ( + gardenHomeDir string clusterIdentity1 = "garden1" clusterIdentity2 = "garden2" clusterAlias1 = "gardenalias1" @@ -31,6 +32,10 @@ var _ = Describe("Config", func() { ) BeforeEach(func() { + dir, err := os.MkdirTemp("", "garden-*") + Expect(err).NotTo(HaveOccurred()) + gardenHomeDir = dir + cfg = &config.Config{ LinkKubeconfig: pointer.Bool(false), Gardens: []config.Garden{ @@ -53,6 +58,10 @@ var _ = Describe("Config", func() { } }) + AfterEach(func() { + Expect(os.RemoveAll(gardenHomeDir)).To(Succeed()) + }) + patternValue := func(prefix string) string { value := fmt.Sprintf("shoot--%s--%s", project, shoot) @@ -181,4 +190,33 @@ var _ = Describe("Config", func() { } Expect(cfg.Save()).NotTo(HaveOccurred()) }) + + Describe("#LoadFromFile", func() { + It("should succeed when file does not exist", func() { + filename := filepath.Join(gardenHomeDir, "gardenctl-v2.yaml") + cfg = &config.Config{ + Filename: filename, + } + + cfg, err := config.LoadFromFile(filename) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.Filename).To(Equal(filename)) + Expect(cfg.Gardens).To(BeNil()) + }) + + It("should succeed when file is empty", func() { + filename := filepath.Join(gardenHomeDir, "gardenctl-v2.yaml") + _, err := os.Create(filename) + Expect(err).NotTo(HaveOccurred()) + + cfg = &config.Config{ + Filename: filename, + } + + cfg, err := config.LoadFromFile(filename) + Expect(err).NotTo(HaveOccurred()) + Expect(cfg.Filename).To(Equal(filename)) + Expect(cfg.Gardens).To(BeNil()) + }) + }) }) diff --git a/pkg/target/target.go b/pkg/target/target.go index e9c1b3b3..2cc7585c 100644 --- a/pkg/target/target.go +++ b/pkg/target/target.go @@ -63,11 +63,11 @@ type Target interface { } type targetImpl struct { - Garden string `yaml:"garden,omitempty" json:"garden,omitempty"` - Project string `yaml:"project,omitempty" json:"project,omitempty"` - Seed string `yaml:"seed,omitempty" json:"seed,omitempty"` - Shoot string `yaml:"shoot,omitempty" json:"shoot,omitempty"` - ControlPlaneFlag bool `yaml:"controlPlane,omitempty" json:"controlPlane,omitempty"` + Garden string `json:"garden,omitempty"` + Project string `json:"project,omitempty"` + Seed string `json:"seed,omitempty"` + Shoot string `json:"shoot,omitempty"` + ControlPlaneFlag bool `json:"controlPlane,omitempty"` } var _ Target = &targetImpl{} diff --git a/pkg/target/target_provider.go b/pkg/target/target_provider.go index da0970f0..ced468ef 100644 --- a/pkg/target/target_provider.go +++ b/pkg/target/target_provider.go @@ -10,7 +10,7 @@ import ( "fmt" "os" - "gopkg.in/yaml.v3" + "sigs.k8s.io/yaml" ) // TargetReader can read targets. @@ -65,8 +65,14 @@ func (p *fsTargetProvider) Read() (Target, error) { } target := &targetImpl{} + if stat.Size() > 0 { - if err := yaml.NewDecoder(f).Decode(target); err != nil { + buf, err := os.ReadFile(p.targetFile) + if err != nil { + return nil, fmt.Errorf("failed to read target file: %w", err) + } + + if err = yaml.Unmarshal(buf, target); err != nil { return nil, fmt.Errorf("failed to decode as YAML: %w", err) } } @@ -80,14 +86,13 @@ func (p *fsTargetProvider) Read() (Target, error) { // Write takes a target and saves it permanently. func (p *fsTargetProvider) Write(t Target) error { - f, err := os.OpenFile(p.targetFile, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0o600) + buf, err := yaml.Marshal(t) if err != nil { - return fmt.Errorf("failed to create file: %w", err) + return fmt.Errorf("failed to encode as YAML: %w", err) } - defer f.Close() - if err := yaml.NewEncoder(f).Encode(t); err != nil { - return fmt.Errorf("failed to encode as YAML: %w", err) + if err := os.WriteFile(p.targetFile, buf, 0o600); err != nil { + return fmt.Errorf("failed to write file: %w", err) } return nil