fix: runtime panic when storage param is empty

[teocns]

Disclaimer: new to Go lang. Feel free to author this PR, feedback is welcome.

The nil checks in func generateRedisClusterParams are ran before cr.Spec.Storage.$ is referenced, hence causing a runtime panic when the `storage field isn't defined in the manifest.

• Moved nil checks on objects before initializing the statefulSetParameters struct
• If cr.Spec.Storage fails nil check, it initializes it to a new instance of redisv1beta2.Storage{}

⚠️ This might not be the ideal place to perform objects initializations. Please carefully review whether this should exist elsewhere according to your design

Type of change

• Breaking change / bug fix

Checklist

• Tests have been added/modified and all tests pass.
• Functionality/bugs have been confirmed to be unchanged or fixed.
• I have performed a self-review of my own code.
• Documentation has been updated or added where necessary.

Additional Context

{"level":"info","ts":"2024-04-19T17:24:45Z","msg":"Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference","controller":"rediscluster","controllerGroup":"redis.redis.opstreelabs.in","controllerKind":"RedisCluster","RedisCluster":{"name":"yup","namespace":"redis"},"namespace":"redis","name":"yup","reconcileID":"a85b8c91-956f-4eaa-a387-1cdf9c9cdb46"}
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x128db1c]

goroutine 173 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:116 +0x1a4
panic({0x14ce4c0?, 0x2788ce0?})
    /usr/local/go/src/runtime/panic.go:914 +0x218
github.com/teocns/redis-operator/k8sutils.generateRedisClusterParams(_, _, _, {{_, _}, _, _, _, _, _, ...})
    /workspace/k8sutils/redis-cluster.go:41 +0xbc
github.com/teocns/redis-operator/k8sutils.RedisClusterSTS.CreateRedisClusterSetup({{0x1755ad1, 0x6}, 0x0, 0x0, 0x0, 0x0, 0x400063c390, 0x400063c3a8, 0x0, 0x0}, ...)
    /workspace/k8sutils/redis-cluster.go:275 +0x5d0
github.com/teocns/redis-operator/k8sutils.CreateRedisLeader(0x1755ad1?, {0x1a109f0?, 0x40004fd040?})
    /workspace/k8sutils/redis-cluster.go:222 +0xd0
github.com/teocns/redis-operator/controllers.(*RedisClusterReconciler).Reconcile(0x400002de50, {0x19f7088, 0x40006478c0}, {{{0x400062c269?, 0x5?}, {0x400062c266?, 0x4000577cf8?}}})
    /workspace/controllers/rediscluster_controller.go:125 +0x518
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile(0x19fa370?, {0x19f7088?, 0x40006478c0?}, {{{0x400062c269?, 0xb?}, {0x400062c266?, 0x0?}}})
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:119 +0x8c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).reconcileHandler(0x40000a6280, {0x19f70c0, 0x400002c320}, {0x15851c0?, 0x4000268820?})
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:316 +0x2e8
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).processNextWorkItem(0x40000a6280, {0x19f70c0, 0x400002c320})
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:266 +0x16c
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2.2()
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:227 +0x74
created by sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Start.func2 in goroutine 62
    /go/pkg/mod/sigs.k8s.io/[email protected]/pkg/internal/controller/controller.go:223 +0x43c
Stream closed EOF for kube-system/redis-operator-6d7bdcc486-879gf (redis-operator)

[drivebyer]

In my opinion, there's no need to perform a nil check on this non-exported function. We have control over it, and we know that cr and cr.spec will not be nil. This approach helps keep the code cleaner. :)

[teocns]

Tho if you deploy a RedisCluster resource without specifying the storage parameter you do face an unmanaged nil reference exception which is hard to frame for somebody that is not at hands with the codebase. IMO it should be managed or Storage needs to have defaults

[drivebyer]

IMO it should be managed or Storage needs to have defaults

I understand you. You could set default value in https://github.com/OT-CONTAINER-KIT/redis-operator/blob/master/api/v1beta2/rediscluster_default.go

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 39.56%. Comparing base (d121d86) to head (f2b29b7).
Report is 41 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #887      +/-   ##
==========================================
+ Coverage   35.20%   39.56%   +4.36%     
==========================================
  Files          19       19              
  Lines        3213     2674     -539     
==========================================
- Hits         1131     1058      -73     
+ Misses       2015     1543     -472     
- Partials       67       73       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[alita1991]

Hi @drivebyer , I reproduced the same issue on redis standalone, probably there is on other resources as well. was this fixed for every resource or just locally for cluster?

{"level":"info","ts":"2024-05-10T08:46:58Z","msg":"Observed a panic in reconciler: runtime error: invalid memory address or nil pointer dereference","controller":"redis","controllerGroup":"redis.redis.opstreelabs.in","controllerKind":"Redis","Redis":{"name":"redis-standalone","namespace":"integration"},"namespace":"integration","name":"redis-standalone","reconcileID":"7fa7d9ba-823f-4d01-9b79-5de377f5dae9"}
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x17854ba]