wilfred-s commented on code in PR #950:
URL: https://github.com/apache/yunikorn-k8shim/pull/950#discussion_r1950280848
##########
pkg/cache/placeholder_manager.go:
##########
@@ -173,7 +173,11 @@ func (mgr *PlaceholderManager) Stop() {
}
func (mgr *PlaceholderManager) isRunning() bool {
- return mgr.running.Load().(bool)
+ value, ok := mgr.running.Load().(bool)
+ if !ok {
+ panic("running flag corrupted: stored value is not a boolean")
Review Comment:
Change the definition of the `running` flag to `atomic.Bool` instead of this
change.
##########
pkg/cache/application_state.go:
##########
@@ -470,7 +471,11 @@ func newAppState() *fsm.FSM { //nolint:funlen
RejectApplication.String(): func(_ context.Context,
event *fsm.Event) {
app := event.Args[0].(*Application)
//nolint:errcheck
eventArgs := make([]string, 1)
- if err :=
events.GetEventArgsAsStrings(eventArgs, event.Args[1].([]interface{})); err !=
nil {
+ generic, ok := event.Args[1].([]interface{})
+ if !ok {
+ panic(fmt.Sprintf("invalid event args
type: %T", event.Args[1]))
+ }
Review Comment:
preferable solution is to use the nolint directive like we did for the
Application cast above. The `GetEventArgsAsStrings` already does further checks
and this is not something that is publicly accessible.
NO panics in prod code
##########
pkg/cache/scheduler_callback.go:
##########
@@ -189,6 +190,10 @@ func (callback *AsyncRMCallback) PreemptionPredicates(args
*si.PreemptionPredica
if !ok {
index = -1
}
+ if index < math.MinInt32 || index > math.MaxInt32 {
+ // Handle the overflow scenario appropriately
+ panic(fmt.Sprintf("index value %d is out of int32 range",
index))
Review Comment:
This is not really a possibility. The index will never be a number that
large. The worst case scenario would be the number of pods that can fit on a
node (100s not even 1000 would be possible). Preferred solution: use nolint
directive and add comment.
##########
pkg/dispatcher/dispatcher.go:
##########
@@ -71,7 +72,13 @@ func initDispatcher() {
}
dispatcher.setRunning(false)
DispatchTimeout = conf.GetSchedulerConf().DispatchTimeout
- AsyncDispatchLimit = max(10000, int32(eventChannelCapacity/10))
+ // Convert to int32 first to avoid potential integer overflow
+ if eventChannelCapacity < math.MinInt32 || eventChannelCapacity >
math.MaxInt32 {
+ // Handle the overflow scenario appropriately
+ panic(fmt.Sprintf("eventChannelCapacity value %d is out of
int32 range", eventChannelCapacity))
Review Comment:
eventChannelCapacity should be checked before we call make the channel. It
cannot be negative in any case, not sure if 0 makes sense either...
Upper bound should be checked after the division not before. Wondering why
we cast and then do a max, this handles both issues
```
if eventChannelCapacity > 10000 {
AsyncDispatchLimit = 10000
} else {
AsyncDispatchLimit = int32(eventChannelCapacity/10)
}
```
NO panic in production code we should fall back to defaults if not set
correctly in the config.
##########
pkg/plugin/predicates/predicate_manager_test.go:
##########
@@ -1094,6 +1095,10 @@ func makeResources(milliCPU, memory, pods, extendedA,
storage, hugePageA int64)
func newPodWithPort(hostPorts ...int) *v1.Pod {
var networkPorts []v1.ContainerPort
for _, port := range hostPorts {
+ // Check for integer overflow before conversion
+ if port < 0 || port > math.MaxInt32 {
+ continue // Skip invalid port numbers
+ }
Review Comment:
Port numbers based on the ContainPort definition must be 0 < x < 65536
If we check anything we should follow that range as the pod spec will
otherwise fail anyway.
##########
test/e2e/framework/helpers/common/utils.go:
##########
@@ -236,7 +236,7 @@ func RunShellCmdForeground(cmdStr string) (string, error) {
}
if errStr := errStream.String(); len(errStr) > 0 {
- return stdOutStream.String(), fmt.Errorf(errStr)
+ return stdOutStream.String(), fmt.Errorf("%s", errStr)
Review Comment:
use `errors.New(errStr)`
##########
pkg/conf/schedulerconf.go:
##########
@@ -246,7 +246,10 @@ func checkNonReloadableBool(name string, old *bool, new
*bool) {
func GetSchedulerConf() *SchedulerConf {
once.Do(createConfigs)
- return confHolder.Load().(*SchedulerConf)
+ if conf, ok := confHolder.Load().(*SchedulerConf); ok {
+ return conf
Review Comment:
This cannot happen as the only way to access the confHolder is inside the
package and that will always set the correct object.
nolint directive is a simpler solution
##########
pkg/dispatcher/dispatch_test.go:
##########
@@ -283,7 +283,14 @@ func TestExceedAsyncDispatchLimit(t *testing.T) {
Stop()
// check error
if err := recover(); err != nil {
- assert.Assert(t, strings.Contains(err.(error).Error(),
"dispatcher exceeds async-dispatch limit"))
+ errStr, ok := err.(error)
+ if !ok {
+ t.Errorf("Expected error type from panic, got
%T", err)
+ return
+ }
+ if !strings.Contains(errStr.Error(), "dispatcher
exceeds async-dispatch limit") {
+ t.Errorf("Expected panic with 'dispatcher
exceeds async-dispatch limit', got: %v", errStr)
+ }
Review Comment:
Why are we not using `assert.Error` or `assert.ErrorContains` to check what
we get back?
##########
pkg/client/apifactory_mock.go:
##########
@@ -398,11 +398,19 @@ func (m *MockedAPIProvider) UpdatePriorityClass(oldObj
*schedv1.PriorityClass, n
}
func (m *MockedAPIProvider) GetPodBindStats() BindStats {
- return m.clients.KubeClient.(*KubeClientMock).GetBindStats()
+ client, ok := m.clients.KubeClient.(*KubeClientMock)
+ if !ok {
+ panic("failed to get KubeClientMock")
+ }
+ return client.GetBindStats()
}
func (m *MockedAPIProvider) GetBoundPods(clear bool) []BoundPod {
- return m.clients.KubeClient.(*KubeClientMock).GetBoundPods(clear)
+ client, ok := m.clients.KubeClient.(*KubeClientMock)
+ if !ok {
+ panic("failed to get KubeClientMock")
Review Comment:
Log the failure and simply return a nil object if the conversion fails. It
should never happen and it will still crash the test in the end.
##########
pkg/dispatcher/dispatcher.go:
##########
@@ -146,7 +153,11 @@ func Dispatch(event events.SchedulingEvent) {
}
func (p *Dispatcher) isRunning() bool {
- return p.running.Load().(bool)
+ val, ok := p.running.Load().(bool)
Review Comment:
Change the definition of the `running` flag to `atomic.Bool` instead of this
change.
##########
pkg/log/logger.go:
##########
@@ -294,12 +294,14 @@ func parseLevel(level string) *zapcore.Level {
// parse numeric
levelNum, err := strconv.ParseInt(level, 10, 31)
Review Comment:
zap's level is a `int8` we should adjust ParseInt to return something that
fits in that value. Leave the rest of the code as is.
We also need to extend the TestParseLevel unit tests to make sure we catch
these outliers
##########
pkg/admission/admission_controller.go:
##########
@@ -607,7 +607,7 @@ func (c *AdmissionController) validateConfigMap(namespace
string, cm *v1.ConfigM
return nil
}
if !responseData.Allowed {
- err = fmt.Errorf(responseData.Reason)
+ err = fmt.Errorf("%s", responseData.Reason)
Review Comment:
Use `errors.New(responseData.Reason)` less overhead
##########
pkg/cache/task_state.go:
##########
@@ -402,7 +402,11 @@ func callbacks(states *TStates) fsm.Callbacks {
task := event.Args[0].(*Task) //nolint:errcheck
eventArgs := make([]string, 1)
reason := ""
- if err := events.GetEventArgsAsStrings(eventArgs,
event.Args[1].([]interface{})); err != nil {
+ generic, ok := event.Args[1].([]interface{})
Review Comment:
preferable solution is to use the nolint directive like we did for the
Application cast earlier. The GetEventArgsAsStrings already does further checks
and this is not something that is publicly accessible.
##########
pkg/client/apifactory_mock.go:
##########
@@ -398,11 +398,19 @@ func (m *MockedAPIProvider) UpdatePriorityClass(oldObj
*schedv1.PriorityClass, n
}
func (m *MockedAPIProvider) GetPodBindStats() BindStats {
- return m.clients.KubeClient.(*KubeClientMock).GetBindStats()
+ client, ok := m.clients.KubeClient.(*KubeClientMock)
+ if !ok {
+ panic("failed to get KubeClientMock")
Review Comment:
Log the failure and simply return a nil object if the conversion fails. It
should never happen and it will still crash the test in the end.
##########
pkg/dispatcher/dispatcher.go:
##########
@@ -146,7 +153,11 @@ func Dispatch(event events.SchedulingEvent) {
}
func (p *Dispatcher) isRunning() bool {
- return p.running.Load().(bool)
+ val, ok := p.running.Load().(bool)
+ if !ok {
+ panic("running value is not a boolean")
Review Comment:
NO panic in production code unless there is a real specific case.
##########
test/e2e/framework/helpers/k8s/gang_job.go:
##########
@@ -113,8 +118,13 @@ func InitTaskGroups(conf SleepPodConfig,
mainTaskGroupName, secondTaskGroupName
// create TG2 more with more members than needed, also make sure that
// placeholders will stay in Pending state
+ minMember := parallelism + 1
+ if minMember < math.MinInt32 || minMember > math.MaxInt32 {
+ // Handle the overflow scenario appropriately
+ panic(fmt.Sprintf("minMember value %d is out of int32 range",
minMember))
+ }
Review Comment:
This can be rolled into the checked above. use `math.MaxInt32-1` for the
upper bound. This is a test function only.
##########
test/e2e/framework/helpers/k8s/gang_job.go:
##########
@@ -102,6 +104,9 @@ func DecoratePodForGangScheduling(
}
func InitTaskGroups(conf SleepPodConfig, mainTaskGroupName,
secondTaskGroupName string, parallelism int) []*cache.TaskGroup {
+ if parallelism < math.MinInt32 || parallelism > math.MaxInt32 {
+ panic(fmt.Sprintf("parallelism value %d is out of int32 range",
parallelism))
+ }
Review Comment:
Return an error and handle the error in the caller using
Ω(callError).NotTo(gomega.HaveOccurred())
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
