libopenstorage / cloudops

Apache License 2.0
7 stars 13 forks source link

PWX-29108 : Handle nil pointer dereference in cloudops #140

Open rge-pure opened 1 year ago

rge-pure commented 1 year ago

What this PR does / why we need it: Following issue was reported on gs-rel 2.13 on Azure PWX-29069. This was related to nil drive size being passed during drive create which was resulting into nil pointer dereference. This was fixed for Azure.

We need to do a thorough check on all the supported cloud platforms supported in cloudops for all the params that can be passed as “nil” that can result into a panic and handle those.

Which issue(s) this PR fixes PWX-29108

Special notes for your reviewer: General Workflow : Be very cautious with nil pointer check, avoid any false-positive

Follow the following rules to detect nil pointer or index out of range

    • Dereference variable in the same function.
    • Pointer variable used to access field or element in the same function.
    • Potential index out of range.
    • Variable mandatory for corresponding cloudops.
    • Variable caused panic bug before. Add special comments for rule 4.

Also, did a go fmt, some changes are based on that.

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.50 :warning:

Comparison is base (9ecda22) 8.10% compared to head (f040508) 7.61%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #140 +/- ## ========================================= - Coverage 8.10% 7.61% -0.50% ========================================= Files 18 18 Lines 4664 4966 +302 ========================================= Hits 378 378 - Misses 4264 4566 +302 Partials 22 22 ``` | [Impacted Files](https://codecov.io/gh/libopenstorage/cloudops/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage) | Coverage Δ | | |---|---|---| | [aws/aws.go](https://codecov.io/gh/libopenstorage/cloudops/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage#diff-YXdzL2F3cy5nbw==) | `6.74% <0.00%> (-0.31%)` | :arrow_down: | | [azure/azure.go](https://codecov.io/gh/libopenstorage/cloudops/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage#diff-YXp1cmUvYXp1cmUuZ28=) | `0.55% <0.00%> (-0.02%)` | :arrow_down: | | [gce/gce.go](https://codecov.io/gh/libopenstorage/cloudops/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage#diff-Z2NlL2djZS5nbw==) | `1.04% <0.00%> (-0.01%)` | :arrow_down: | | [oracle/oracle.go](https://codecov.io/gh/libopenstorage/cloudops/pull/140?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage#diff-b3JhY2xlL29yYWNsZS5nbw==) | `6.56% <0.00%> (-0.19%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://codecov.io/gh/libopenstorage/cloudops/pull/140/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=libopenstorage)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

pp511 commented 1 year ago

Discussed offline. Summarizing here: Some params like *Encryption can be passed as nil. It won't cause panic but would just end up creating a non encrypted disk. Let's only check for mandatory params Eg: size. type could be a mandatory param in some clouds(not sure which) but is not in AWS as it defaults to gp2.

rge-pure commented 1 year ago

Thanks Priyanshu for the advice.