Skip to content

Commit

Permalink
Merge pull request #1596 from robpblake/ocm-4784-validate-max-pids-limit
Browse files Browse the repository at this point in the history
OCM-4784 | fix: Ensure we validate maximum pids limit when creating/editing KubeletConfig
  • Loading branch information
openshift-merge-bot[bot] authored Nov 13, 2023
2 parents d1be2a3 + 3bf3cc6 commit b0b615b
Show file tree
Hide file tree
Showing 9 changed files with 313 additions and 78 deletions.
34 changes: 3 additions & 31 deletions cmd/create/kubeletconfig/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,36 +88,8 @@ func run(_ *cobra.Command, _ []string) {
os.Exit(1)
}

if args.podPidsLimit == PodPidsLimitOptionDefaultValue && !interactive.Enabled() {
interactive.Enable()
r.Reporter.Infof("Enabling interactive mode")
}

if interactive.Enabled() {
args.podPidsLimit, err = interactive.GetInt(interactive.Input{
Question: InteractivePodPidsLimitPrompt,
Help: InteractivePodPidsLimitHelp,
Options: nil,
Default: nil,
Required: true,
Validators: []interactive.Validator{
// We only validate the minimum as some customers have capability to exceed maximum
// We rely on backend validation for that
interactive.Min(MinPodPidsLimit),
},
})

if err != nil {
r.Reporter.Errorf("Failed creating KubeletConfig for cluster '%s': '%s'",
clusterKey, err)
os.Exit(1)
}

}

if args.podPidsLimit < MinPodPidsLimit {
r.Reporter.Errorf("The minimum value for --pod-pids-limit is '%d'. You have supplied '%d'",
MinPodPidsLimit, args.podPidsLimit)
requestedPids, err := ValidateOrPromptForRequestedPidsLimit(args.podPidsLimit, clusterKey, nil, r)
if err != nil {
os.Exit(1)
}

Expand All @@ -127,7 +99,7 @@ func run(_ *cobra.Command, _ []string) {
if confirm.ConfirmRaw(prompt) {

r.Reporter.Debugf("Creating KubeletConfig for cluster '%s'", clusterKey)
kubeletConfigArgs := ocm.KubeletConfigArgs{PodPidsLimit: args.podPidsLimit}
kubeletConfigArgs := ocm.KubeletConfigArgs{PodPidsLimit: requestedPids}

_, err = r.OCMClient.CreateKubeletConfig(cluster.ID(), kubeletConfigArgs)
if err != nil {
Expand Down
33 changes: 3 additions & 30 deletions cmd/edit/kubeletconfig/cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,35 +92,8 @@ func run(_ *cobra.Command, _ []string) {

r.Reporter.Debugf("Updating KubeletConfig for cluster '%s'", clusterKey)

if args.podPidsLimit == PodPidsLimitOptionDefaultValue && !interactive.Enabled() {
interactive.Enable()
r.Reporter.Infof("Enabling interactive mode")
}

if interactive.Enabled() {
args.podPidsLimit, err = interactive.GetInt(interactive.Input{
Question: InteractivePodPidsLimitPrompt,
Help: InteractivePodPidsLimitHelp,
Options: nil,
Default: kubeletconfig.PodPidsLimit(),
Required: true,
Validators: []interactive.Validator{
// We only validate the minimum as some customers have capability to exceed maximum
// We rely on backend validation for that
interactive.Min(MinPodPidsLimit),
},
})

if err != nil {
r.Reporter.Errorf("Failed updating KubeletConfig for cluster '%s': '%s'",
clusterKey, err)
os.Exit(1)
}
}

if args.podPidsLimit < MinPodPidsLimit {
r.Reporter.Errorf("The minimum value for --pod-pids-limit is '%d'. You have supplied '%d'",
MinPodPidsLimit, args.podPidsLimit)
requestedPids, err := ValidateOrPromptForRequestedPidsLimit(args.podPidsLimit, clusterKey, kubeletconfig, r)
if err != nil {
os.Exit(1)
}

Expand All @@ -129,7 +102,7 @@ func run(_ *cobra.Command, _ []string) {

if confirm.ConfirmRaw(prompt) {
r.Reporter.Debugf("Updating KubeletConfig for cluster '%s'", clusterKey)
_, err = r.OCMClient.UpdateKubeletConfig(cluster.ID(), ocm.KubeletConfigArgs{PodPidsLimit: args.podPidsLimit})
_, err = r.OCMClient.UpdateKubeletConfig(cluster.ID(), ocm.KubeletConfigArgs{PodPidsLimit: requestedPids})
if err != nil {
r.Reporter.Errorf("Failed creating custom KubeletConfig for cluster '%s': %s",
cluster.ID(), err)
Expand Down
29 changes: 25 additions & 4 deletions pkg/interactive/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,12 +124,33 @@ func IsCIDR(val interface{}) error {
return fmt.Errorf("can only validate strings, got %v", val)
}

func Min(min int) Validator {
// MaxValue returns a Validator that validates the entered number is less than or equal to max
func MaxValue(max int) Validator {
return func(ans interface{}) error {
if i, ok := ans.(string); ok {
val, err := strconv.Atoi(i)
if str, ok := ans.(string); ok {
val, err := strconv.Atoi(str)
if err != nil {
return fmt.Errorf("please enter an integer value, you entered '%s'", i)
return fmt.Errorf("please enter an integer value, you entered '%s'", str)
}
if val > max {
return fmt.Errorf(
"'%d' is greater than the permitted maximum of '%d'", val, max)
}

return nil
}

return fmt.Errorf("can only validate strings, got %v", ans)
}
}

// MinValue returns a validator that validates the entered number is greater than or equal to min
func MinValue(min int) Validator {
return func(ans interface{}) error {
if str, ok := ans.(string); ok {
val, err := strconv.Atoi(str)
if err != nil {
return fmt.Errorf("please enter an integer value, you entered '%s'", str)
}
if val < min {
return fmt.Errorf(
Expand Down
49 changes: 43 additions & 6 deletions pkg/interactive/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,39 +6,76 @@ import (
)

var _ = Describe("Validation", func() {
Context("Min", func() {
Context("MinValue", func() {
It("Fails validation if the answer is less than the minimum", func() {
validator := Min(50)
validator := MinValue(50)
err := validator("25")

Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("'25' is less than the permitted minimum of '50'"))
})

It("Fails validation if the answer is not an integer", func() {
validator := Min(50)
validator := MinValue(50)
err := validator("hello")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("please enter an integer value, you entered 'hello'"))
})

It("Fails validation if the answer is not a string", func() {
validator := Min(50)
validator := MinValue(50)
err := validator(45)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("can only validate strings, got int"))
})

It("Passes validation if the answer is greater than the min", func() {
validator := Min(50)
validator := MinValue(50)
err := validator("55")
Expect(err).NotTo(HaveOccurred())
})

It("Passes validation if the answer is equal to the min", func() {
validator := Min(50)
validator := MinValue(50)
err := validator("50")
Expect(err).NotTo(HaveOccurred())
})
})

Context("MaxValue", func() {
It("Fails validation if the answer is greater than the maximum", func() {
validator := MaxValue(50)
err := validator("52")

Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("'52' is greater than the permitted maximum of '50'"))
})

It("Fails validation if the answer is not an integer", func() {
validator := MaxValue(50)
err := validator("hello")
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("please enter an integer value, you entered 'hello'"))
})

It("Fails validation if the answer is not a string", func() {
validator := MaxValue(50)
err := validator(45)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(ContainSubstring("can only validate strings, got int"))
})

It("Passes validation if the answer is less than the max", func() {
validator := MaxValue(50)
err := validator("49")
Expect(err).NotTo(HaveOccurred())
})

It("Passes validation if the answer is equal to the max", func() {
validator := MaxValue(50)
err := validator("50")
Expect(err).NotTo(HaveOccurred())
})
})

})
98 changes: 98 additions & 0 deletions pkg/kubeletconfig/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
package kubeletconfig

import (
"fmt"
v1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
"github.com/openshift/rosa/pkg/rosa"

"github.com/openshift/rosa/pkg/interactive"
)

//go:generate mockgen -source=config.go -package=kubeletconfig -destination=mock_capability_checker.go
type CapabilityChecker interface {
IsCapabilityEnabled(capability string) (bool, error)
}

// GetMaxPidsLimit - returns the maximum pids limit for the current organization
// the maximum is varied depending on whether the current organizaton has
// the capability.organization.bypass_pids_limit capability
func GetMaxPidsLimit(client CapabilityChecker) (int, error) {
enabled, err := client.IsCapabilityEnabled(ByPassPidsLimitCapability)
if err != nil {
return -1, err
}

if enabled {
return MaxUnsafePodPidsLimit, nil
}
return MaxPodPidsLimit, nil
}

func GetInteractiveMaxPidsLimitHelp(maxPidsLimit int) string {
return fmt.Sprintf(InteractivePodPidsLimitHelp, maxPidsLimit)
}

func GetInteractiveInput(maxPidsLimit int, kubeletConfig *v1.KubeletConfig) interactive.Input {

var defaultLimit = PodPidsLimitOptionDefaultValue
if kubeletConfig != nil {
defaultLimit = kubeletConfig.PodPidsLimit()
}

return interactive.Input{
Question: InteractivePodPidsLimitPrompt,
Help: GetInteractiveMaxPidsLimitHelp(maxPidsLimit),
Options: nil,
Default: defaultLimit,
Required: true,
Validators: []interactive.Validator{
interactive.MinValue(MinPodPidsLimit),
interactive.MaxValue(maxPidsLimit),
},
}
}

// ValidateOrPromptForRequestedPidsLimit validates user provided limits or prompts via interactive mode
// if the user hasn't specified any limit on the command line.
func ValidateOrPromptForRequestedPidsLimit(
requestedPids int,
clusterKey string,
kubeletConfig *v1.KubeletConfig,
r *rosa.Runtime) (int, error) {

if requestedPids == PodPidsLimitOptionDefaultValue && !interactive.Enabled() {
interactive.Enable()
r.Reporter.Infof("Enabling interactive mode")
}

maxPidsLimit, err := GetMaxPidsLimit(r.OCMClient)
if err != nil {
return PodPidsLimitOptionDefaultValue,
r.Reporter.Errorf("Failed to check maximum allowed Pids limit for cluster '%s'",
clusterKey)
}

if interactive.Enabled() {
requestedPids, err = interactive.GetInt(GetInteractiveInput(maxPidsLimit, kubeletConfig))

if err != nil {
return PodPidsLimitOptionDefaultValue,
r.Reporter.Errorf("Failed reading requested Pids limit for cluster '%s': '%s'",
clusterKey, err)
}
}

if requestedPids < MinPodPidsLimit {
return PodPidsLimitOptionDefaultValue,
r.Reporter.Errorf("The minimum value for --pod-pids-limit is '%d'. You have supplied '%d'",
MinPodPidsLimit, requestedPids)
}

if requestedPids > maxPidsLimit {
return PodPidsLimitOptionDefaultValue,
r.Reporter.Errorf("The maximum value for --pod-pids-limit is '%d'. You have supplied '%d'",
maxPidsLimit, requestedPids)
}

return requestedPids, nil
}
71 changes: 71 additions & 0 deletions pkg/kubeletconfig/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package kubeletconfig

import (
"github.com/golang/mock/gomock"
. "github.com/onsi/ginkgo/v2/dsl/core"
. "github.com/onsi/gomega"
v1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1"
)

var _ = Describe("KubeletConfig Config", func() {

Context("GetMaxPidsLimit", func() {

var ctrl *gomock.Controller
var capabilityChecker *MockCapabilityChecker

BeforeEach(func() {
ctrl = gomock.NewController(GinkgoT())
capabilityChecker = NewMockCapabilityChecker(ctrl)
})

It("Returns Correct Max Pids Limit When Org Has Capability", func() {
capabilityChecker.EXPECT().IsCapabilityEnabled(ByPassPidsLimitCapability).Return(true, nil)

max, err := GetMaxPidsLimit(capabilityChecker)
Expect(err).NotTo(HaveOccurred())
Expect(max).To(Equal(MaxUnsafePodPidsLimit))
})

It("Returns Correct Max Pids Limit When Org Does Not Have Capability", func() {
capabilityChecker.EXPECT().IsCapabilityEnabled(ByPassPidsLimitCapability).Return(false, nil)

max, err := GetMaxPidsLimit(capabilityChecker)
Expect(err).NotTo(HaveOccurred())
Expect(max).To(Equal(MaxPodPidsLimit))
})
})

Context("GetInteractiveMaxPidsLimitHelp", func() {
It("Correctly generates the Max Pids Limit Interactive Help", func() {
help := GetInteractiveMaxPidsLimitHelp(5000)
Expect(help).To(Equal("Set the Pod Pids Limit field to a value between 4096 and 5000"))
})
})

Context("GetInteractiveInput", func() {
It("Correctly generates Interactive Input for pre-existing KubeletConfig", func() {

builder := v1.KubeletConfigBuilder{}
kubeletConfig, err := builder.PodPidsLimit(10000).Build()

Expect(err).NotTo(HaveOccurred())

input := GetInteractiveInput(5000, kubeletConfig)
Expect(input.Required).To(BeTrue())
Expect(input.Question).To(Equal(InteractivePodPidsLimitPrompt))
Expect(input.Help).To(Equal(GetInteractiveMaxPidsLimitHelp(5000)))
Expect(len(input.Validators)).To(Equal(2))
Expect(input.Default).To(Equal(kubeletConfig.PodPidsLimit()))
})

It("Correctly generates Interactive Input for new KubeletConfig", func() {
input := GetInteractiveInput(5000, nil)
Expect(input.Required).To(BeTrue())
Expect(input.Question).To(Equal(InteractivePodPidsLimitPrompt))
Expect(input.Help).To(Equal(GetInteractiveMaxPidsLimitHelp(5000)))
Expect(len(input.Validators)).To(Equal(2))
Expect(input.Default).To(Equal(PodPidsLimitOptionDefaultValue))
})
})
})
Loading

0 comments on commit b0b615b

Please sign in to comment.