Uh oh!
There was an error while loading. Please reload this page.
BUG-ID: CLOUDSTACK-8922: Unable to delete IP tag - #905
Conversation
wilderrodrigues
commented
Oct 1, 2015
Hi @yvsubhash Any details about how you tested it? Cheers, |
yvsubhash
commented
Oct 1, 2015
It is tested manually with the data similar to the one mentioned in the bug |
sebgoa
commented
Oct 5, 2015
@yvsubhash we should try to remove manual testing. Can you write a marvin test for it. thanks. cc @wilderrodrigues |
rajesh-battala
commented
Apr 21, 2016
Marvin test really helps for validating this bug fix. |
yadvr
commented
May 2, 2016
@yvsubhash please rebase against latest master and push -f, update on status of your PR; please add a marvin test that can validate the fix |
bvbharatk
commented
Jun 9, 2016
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no):https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0 Failed tests: Skipped tests: Passed test suits: |
ef500e8 to
2969780Compareniteshsarda
commented
Mar 27, 2017
I have tested this and LGTM for test. |
2d7ea7a to
d40fbc7Compareyvsubhash
commented
Apr 9, 2017
tag:mergeready |
| "List tags should return empty response" | ||
| ) | ||
| return | ||
| @attr(tags=["advanced"], required_hardware="false") |
There was a problem hiding this comment.
Do we really need hardware to test this, @yvsubhash ?
There was a problem hiding this comment.
@DaanHoogland it can be run on simulator. that is the reason required_hardware is set to false. May I now the concern here
There was a problem hiding this comment.
I frased my question wrong. can this be moved to the smoke tests?
There was a problem hiding this comment.
@DaanHoogland all the tests in test_tags moved to smoke as they all run on simulator
| @ActionEvent(eventType = EventTypes.EVENT_TAGS_DELETE, eventDescription = "deleting resource tags") | ||
| public boolean deleteTags(List<String> resourceIds, ResourceObjectType resourceType, Map<String, String> tags) { | ||
| Account caller = CallContext.current().getCallingAccount(); | ||
| List<String> uuids = new ArrayList<String>(); |
There was a problem hiding this comment.
deleteTags() is already to big. Please factor out the new code in a separate set of methods.
karuturi
commented
Jun 6, 2017
@DaanHoogland Can you review? |
9bf58c5 to
a7ab48eComparecloudmonger
commented
Sep 4, 2017
ACS CI BVT RunSumarry: Link to logs Folder (search by build_no):https://www.dropbox.com/sh/r2si930m8xxzavs/AAAzNrnoF1fC3auFrvsKo_8-a?dl=0 Failed tests:
Skipped tests: Passed test suits: |
niteshsarda
commented
Oct 30, 2017
Tested this fix and it seems to be working as per expectations. LGTM for test. Following are the results of the test cases that I have executed locally : Test Create tag on LB rule and remove the LB rule ... === TestName: test_01_lbrule_tag | Status : SUCCESS === Ran 24 tests in 502.723s OK (SKIP=1) |
a7ab48e to
21a267eCompare21a267e to
b3308baCompareyadvr
commented
Nov 22, 2017
LGTM, pending travis results. |
blueorangutan
commented
Nov 22, 2017
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. |
blueorangutan
commented
Nov 22, 2017
Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1284 |
yadvr
commented
Nov 22, 2017
@blueorangutan test |
blueorangutan
commented
Nov 22, 2017
@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
rafaelweingartner
left a comment
There was a problem hiding this comment.
@yvsubhash Thanks for the PR.
I have some remarks for you.
| try { | ||
| internalId = Long.parseLong(resourceId); | ||
| } catch (final NumberFormatException e) { | ||
| internalId = null; |
There was a problem hiding this comment.
What about throwing the InvalidParameterValueException here?
| private List<? extends ResourceTag> getResourceTags(final List<String> resourceIds, ResourceObjectType resourceType) { | ||
| List<String> uuids = new ArrayList<String>(); | ||
| List<String> internalIds = new ArrayList<String>(); |
There was a problem hiding this comment.
The type of this List should be Long, right?
There was a problem hiding this comment.
No, having internalIds as interger does not help in anyway, as we get are using them again to construct the query
There was a problem hiding this comment.
What do you mean by using them to construct query?
| }else{ | ||
| Long internalId = null; | ||
| try { | ||
| internalId = Long.parseLong(resourceId); |
There was a problem hiding this comment.
What about executing the code internalIds.add(resourceId); here?
Then we do not need that IF there. We also would not need to declare Long internalId = null;
| List<? extends ResourceTag> resourceTags = _resourceTagDao.search(sc, null); | ||
| ; | ||
| Account caller = CallContext.current().getCallingAccount(); |
There was a problem hiding this comment.
This variable called is not used?
There was a problem hiding this comment.
It seems be about caller. that got used in
_accountMgr.checkAccess(caller, null, false, owner);
| List<String> uuids = new ArrayList<String>(); | ||
| List<String> internalIds = new ArrayList<String>(); | ||
| for(String resourceId : resourceIds){ | ||
| if(resourceId.matches("^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$")){ |
There was a problem hiding this comment.
What about simplifying this ?
You can use org.apache.commons.lang.math.NumberUtils.isDigits(String), to check if you have a long there, then you can convert it directly to Long, otherwise, you can check if it empty using org.apache.commons.lang3.StringUtils.isNotBlank(CharSequence), and if it is not blank, you can add in the UUID list.
There was a problem hiding this comment.
yes. But validating uuid also is possibly more foolproof
There was a problem hiding this comment.
Well, yes and no. I was thinking about it yesterday, do we need to validate it really?
If it is a string, we could simply use it, if it is invalid, we will not find anything.
I see validation useful when you display a message back to the user saying, "hey, the data you entered is not following the expected format, which should be ...."
That is not what is happening here.
b3308ba to
eec9aefCompareblueorangutan
commented
Nov 23, 2017
Trillian test result (tid-1685)
|
bwsw
commented
Nov 23, 2017
It looks overcomplicated, really, search over resourceId is a wrong logic. It should search only against uuds. We discussed the problem in a mail list with @rafaelweingartner. All marvin component and smoke are passing, and tag specific too: I believe, leaving internal id search is a wrong way to deal with it, because they can not and shouldn't appear in API. Also, the code I'm working on includes security fix for listing resource tags because right now the code from the PR doesn't even checks privileges for listing: Mine: So, basically the PR:
I need couple of days to finish the code because I want to add account tags feature and have to implement set of marvin tests for it. |
rafaelweingartner
commented
Nov 23, 2017
@bwsw I thought this was your PR, sorry for my lack of attention. So it seems someone else also ran into the same problem as you. Well, if you will still open another PR for this, we can check and discuss both solutions (pros and cons). |
bwsw
commented
Nov 23, 2017
@rafaelweingartner looks reasonable |
bwsw
commented
Dec 4, 2017
@rafaelweingartner@yvsubhash Take a look at |
840037f to
e1c5f26Comparee1c5f26 to
3693f23Compare…s support (#2350) This PR introduces several features and fixes some bugs: - account tags feature - fixed resource tags bugs which happened during tags search (found wrong entries because of mysql string to number translation - see #905, but this PR does more and fixes also resource access - vulnerability during list resource tags) - some marvin improvements (speed, sanity) Improved resource tags code: 1. Enhanced listTags security 2. Added support for account tags (account tags are required to support tags common for all users of an account) 3. Improved the tag management code (refactoring and cleanup) Marvin: 1. Fixed Marvin wait timeout between async pools. To decrease polling interval and improve CI speed. 2. Fixed /tmp/ to /tmp in zone configuration files. 3. Fixed + to os.path.join in log class. 4. Fixed + to os.path.join in deployDataCenter class. 5. Fixed typos in tag tests. 6. Modified Tags base class delete method. Deploy Datacenter script: 1. Improved deployDatacenter. Added option logdir to specify where script places results of evaluation. ConfigurationManagerImpl: 1. Added logging to ConfigurationManagerImpl to log when vlan is not found. Added test stubs for tags. Found accidental exception during simulator running after CI. tests_tags.py: 1. Fixed stale undeleted tags. 2. Changed region:India to scope:TestName.
yvsubhash
commented
Jan 9, 2018
closing this pr as #2350 takes care of this |
Co-authored-by: Pearl Dsilva <pearl.dsilva@shapeblue.com> Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
Root Cause: Due to wrong search string we are fetching a resource which does not belong to current account and domain. This results in permission exception.
Solution: Code changes has been done to fix the incorrect search string.