Fix NPE with ApiKeyPair during listApis call (from cmk)#13149
Fix NPE with ApiKeyPair during listApis call (from cmk)#13149sureshanaparti wants to merge 1 commit into
Conversation
|
@KlausDornsbach can you review this. |
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13149 +/- ##
============================================
- Coverage 18.08% 18.08% -0.01%
+ Complexity 16718 16714 -4
============================================
Files 6037 6037
Lines 542611 542617 +6
Branches 66433 66435 +2
============================================
- Hits 98136 98123 -13
- Misses 433448 433466 +18
- Partials 11027 11028 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17820 |
@sureshanaparti, @KlausDornsbach is not active in the community anymore. I am asking @bernardodemarco to review this, as he was responsible for maintaning the keypairs PR. |
|
@sureshanaparti, thanks for the PR! I'll try to review and test it during this week. |
|
@blueorangutan test |
|
@blueorangutan test |
|
@sureshanaparti a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16105)
|
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
Getting the following error on ol8
(localcloud) 🐱 > sync
panic: interface conversion: interface {} is nil, not []interface {}
goroutine 1 [running]:
github.com/apache/cloudstack-cloudmonkey/config.(*Config).UpdateCache(0xc0000e0120?, 0xc0013b0390)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/config/cache.go:115 +0x1338
github.com/apache/cloudstack-cloudmonkey/cmd.init.4.func1(0xc0011ab740)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmd/sync.go:35 +0xd0
github.com/apache/cloudstack-cloudmonkey/cli.ExecCmd({0xc001330d30, 0x1, 0x1}, 0x0)
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:64 +0x1bb
github.com/apache/cloudstack-cloudmonkey/cli.ExecLine({0xc00133d420, 0x4})
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/exec.go:51 +0x21b
github.com/apache/cloudstack-cloudmonkey/cli.ExecPrompt()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cli/prompt.go:84 +0x26e
main.main()
/home/shwstppr/lab/shapeblue/cloudstack-cloudmonkey/cmk.go:101 +0x7f1
@kiranchavala no user with that api key, re-check with valid key. cmk also need a fix to address this error: (created issue: apache/cloudstack-cloudmonkey#211) |
sureshanaparti
force-pushed
the
api-keypair-npe-fix
branch
from
492b5d1 to
aa6f16b
Compare
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17930 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16144)
|
kiranchavala
left a comment
kiranchavala
left a comment
There was a problem hiding this comment.
LGTM
generatated the keys for a user > configured in cmk > sync
Able to execute the api calls
I don’t think these are related, up to you @winterhazel @weizhouapache |
| ApiKeyPair accessingKeyPair = apiKeyPairService.findByApiKey(apiKey); | ||
| if (accessingKeyPair == null) { | ||
| logger.warn("Unable to find API key pair for the accessing API key: {}", apiKey); | ||
| return Boolean.TRUE; |
There was a problem hiding this comment.
I did not check the whole workflow, I am not sure if it is fine to return true here
it would be better others can review and evaluate the side-effects
There was a problem hiding this comment.
I think that it is better to return false here instead. Could you have a look @bernardodemarco?
This method validates whether the keypair being used is a superset of the keypair being accessed in order to prevent a key with less privileges to get keys with more privileges. true is returned when an apiKey is not being used to call the API, which makes sense as this check is not necessary in this case. However, as a keypair is being used here, I think that it would make more sense to either inform that it is not a superset, as we could not obtain the key to confirm, or throw a runtime exception.
There was a problem hiding this comment.
@weizhouapache @winterhazel great catch guys... you are correct, we should return Boolean.FALSE here.
When I first reviewed the PR, I had not paid too much attention and I had inferred that we would have to return Boolean.TRUE because of the first conditional statement. But, as Fabricio has explained here https://github.com/apache/cloudstack/pull/13149/changes#r3320265882, the first conditional structure checks whether the API call is performed via session key.
bernardodemarco
left a comment
bernardodemarco
left a comment
There was a problem hiding this comment.
@sureshanaparti, I think that we only need to adjust this topic https://github.com/apache/cloudstack/pull/13149/changes#r3287789004. Then, the PR will be ready to go
7 participants
Description
This PR fixes NPE issue with ApiKeyPair during listApis call (from cmk).
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?