Skip to content

Commit

Permalink
OCM-8075 | fix: Ensure users can only supply a single kubeletconfig f…
Browse files Browse the repository at this point in the history
…or HCP MachinePools
  • Loading branch information
robpblake committed May 16, 2024
1 parent d4bffcb commit 87232a4
Show file tree
Hide file tree
Showing 6 changed files with 120 additions and 9 deletions.
4 changes: 2 additions & 2 deletions cmd/create/machinepool/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,9 @@ func init() {
&args.kubeletConfigs,
"kubelet-configs",
"",
"Name of the kubelet configs to be applied to the machine pool. Format should be a comma-separated list. "+
"Name of the kubelet config to be applied to the machine pool. A single kubelet config is allowed. "+
"Kubelet config must already exist. "+
"This list will overwrite any modifications made to node kubelet configs on an ongoing basis.",
"This will overwrite any modifications made to node kubelet configs on an ongoing basis.",
)

flags.StringVar(&args.rootDiskSize,
Expand Down
14 changes: 12 additions & 2 deletions cmd/create/machinepool/nodepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/openshift/rosa/pkg/helper/versions"
"github.com/openshift/rosa/pkg/interactive"
"github.com/openshift/rosa/pkg/interactive/securitygroups"
"github.com/openshift/rosa/pkg/machinepool"
"github.com/openshift/rosa/pkg/output"
"github.com/openshift/rosa/pkg/rosa"
)
Expand Down Expand Up @@ -407,19 +408,28 @@ func addNodePool(cmd *cobra.Command, clusterKey string, cluster *cmv1.Cluster, r
// Skip if no kubelet configs are available
if len(availableKubeletConfigs) > 0 {
inputKubeletConfigs, err = interactive.GetMultipleOptions(interactive.Input{
Question: "Kubelet configs",
Question: "Kubelet config",
Help: cmd.Flags().Lookup("kubelet-configs").Usage,
Options: availableKubeletConfigs,
Default: inputKubeletConfigs,
Required: false,
Validators: []interactive.Validator{
machinepool.ValidateKubeletConfig,
},
})
if err != nil {
r.Reporter.Errorf("Expected a valid value for kubelet configs: %s", err)
r.Reporter.Errorf("Expected a valid value for kubelet config: %s", err)
os.Exit(1)
}
}
}

err = machinepool.ValidateKubeletConfig(inputKubeletConfigs)
if err != nil {
r.Reporter.Errorf(err.Error())
os.Exit(1)
}

if len(inputKubeletConfigs) != 0 {
npBuilder.KubeletConfigs(inputKubeletConfigs...)
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/edit/machinepool/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ func init() {
&args.kubeletConfigs,
"kubelet-configs",
"",
"Name of the kubelet configs to be applied to the machine pool. Format should be a comma-separated list. "+
"Name of the kubelet config to be applied to the machine pool. A single kubelet config is allowed. "+
"Kubelet config must already exist. "+
"This list will overwrite any modifications made to node kubelet configs on an ongoing basis.",
"This will overwrite any modifications made to node kubelet configs on an ongoing basis.",
)

flags.StringVar(&args.nodeDrainGracePeriod,
Expand Down
15 changes: 12 additions & 3 deletions cmd/edit/machinepool/nodepool.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package machinepool

import (
"fmt"
"os"
"strings"

cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/spf13/cobra"

"github.com/openshift/rosa/pkg/helper/machinepools"
"github.com/openshift/rosa/pkg/interactive"
"github.com/openshift/rosa/pkg/machinepool"
rprtr "github.com/openshift/rosa/pkg/reporter"
"github.com/openshift/rosa/pkg/rosa"
)
Expand Down Expand Up @@ -181,18 +183,25 @@ func editNodePool(cmd *cobra.Command, nodePoolID string,
// Skip if no tuning configs are available
if len(availableKubeletConfigs) > 0 {
inputKubeletConfig, err = interactive.GetMultipleOptions(interactive.Input{
Question: "Kubelet configs",
Question: "Kubelet config",
Help: cmd.Flags().Lookup("kubelet-configs").Usage,
Options: availableKubeletConfigs,
Default: inputKubeletConfig,
Required: false,
Validators: []interactive.Validator{
machinepool.ValidateKubeletConfig,
},
})
if err != nil {
return fmt.Errorf("Expected a valid value for kubelet configs: %s", err)
return fmt.Errorf("Expected a valid value for kubelet config: %s", err)
}
}
}

err = machinepool.ValidateKubeletConfig(inputKubeletConfig)
if err != nil {
r.Reporter.Errorf(err.Error())
os.Exit(1)
}
npBuilder.KubeletConfigs(inputKubeletConfig...)
}

Expand Down
24 changes: 24 additions & 0 deletions pkg/machinepool/validation.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package machinepool

import (
"fmt"

"github.com/AlecAivazis/survey/v2/core"
)

func ValidateKubeletConfig(input interface{}) error {
if strings, ok := input.([]string); ok {
return validateCount(strings)
} else if answers, ok := input.([]core.OptionAnswer); ok {
return validateCount(answers)
}

return fmt.Errorf("Input for kubelet config flag is not valid")
}

func validateCount[K any](kubeletConfigs []K) error {
if len(kubeletConfigs) > 1 {
return fmt.Errorf("Only a single kubelet config is supported for Machine Pools")
}
return nil
}
68 changes: 68 additions & 0 deletions pkg/machinepool/validation_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package machinepool

import (
"github.com/AlecAivazis/survey/v2/core"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

var _ = Describe("MachinePool validation", func() {
Context("KubeletConfigs", func() {

It("Fails if customer requests more than 1 kubelet config via []string", func() {
kubeletConfigs := []string{"foo", "bar"}
err := ValidateKubeletConfig(kubeletConfigs)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Only a single kubelet config is supported for Machine Pools"))
})

It("Fails if customer requests more than 1 kubelet config via []core.OptionAnswer", func() {
kubeletConfigs := []core.OptionAnswer{
{
Value: "foo",
Index: 0,
},
{
Value: "bar",
Index: 1,
},
}
err := ValidateKubeletConfig(kubeletConfigs)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Only a single kubelet config is supported for Machine Pools"))
})

It("Passes if a customer selects only a single kubelet config via []core.OptionAnswer", func() {
kubeletConfigs := []core.OptionAnswer{
{
Value: "foo",
Index: 0,
},
}
err := ValidateKubeletConfig(kubeletConfigs)
Expect(err).NotTo(HaveOccurred())
})

It("Passes if a customer selects only a single kubelet config via []string", func() {
kubeletConfigs := []string{"foo"}
err := ValidateKubeletConfig(kubeletConfigs)
Expect(err).NotTo(HaveOccurred())
})

It("Passes with empty selection via []string", func() {
err := ValidateKubeletConfig([]string{})
Expect(err).NotTo(HaveOccurred())
})

It("Passes with empty selection via []core.OptionAnswer", func() {
err := ValidateKubeletConfig([]core.OptionAnswer{})
Expect(err).NotTo(HaveOccurred())
})

It("Fails if the input is not a []string or []core.OptionAnswer", func() {
err := ValidateKubeletConfig("foo")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("Input for kubelet config flag is not valid"))
})
})
})

0 comments on commit 87232a4

Please sign in to comment.