-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix API deleteTrafficType not filtering physical network #6510
Fix API deleteTrafficType not filtering physical network #6510
Conversation
engine/schema/src/test/java/com/cloud/network/dao/NetworkDaoImplTest.java
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! |
@@ -572,7 +572,7 @@ public List<NetworkVO> listByPhysicalNetwork(final long physicalNetworkId) { | |||
public List<NetworkVO> listByPhysicalNetworkTrafficType(final long physicalNetworkId, final TrafficType trafficType) { | |||
final SearchCriteria<NetworkVO> sc = AllFieldsSearch.create(); | |||
sc.setParameters("trafficType", trafficType); | |||
sc.setParameters("physicalNetworkId", physicalNetworkId); | |||
sc.setParameters("physicalNetwork", physicalNetworkId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is strange, the field in both NetworkVO
and in PhysicalNetworkTrafficType
is called physicalNetworkId
. I would say the new value is the typo. Do you know why this should work this way @GutoVeronezi ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaanHoogland, SearchCriteria
works according the declaration of the field while creating the SearchBuilder
. The field representing the physical network ID was declared as physicalNetwork
:
cloudstack/engine/schema/src/main/java/com/cloud/network/dao/NetworkDaoImpl.java
Lines 122 to 141 in ea9124e
AllFieldsSearch = createSearchBuilder(); | |
AllFieldsSearch.and("name", AllFieldsSearch.entity().getName(), Op.EQ); | |
AllFieldsSearch.and("trafficType", AllFieldsSearch.entity().getTrafficType(), Op.EQ); | |
AllFieldsSearch.and("cidr", AllFieldsSearch.entity().getCidr(), Op.EQ); | |
AllFieldsSearch.and("broadcastType", AllFieldsSearch.entity().getBroadcastDomainType(), Op.EQ); | |
AllFieldsSearch.and("offering", AllFieldsSearch.entity().getNetworkOfferingId(), Op.EQ); | |
AllFieldsSearch.and("datacenter", AllFieldsSearch.entity().getDataCenterId(), Op.EQ); | |
AllFieldsSearch.and("account", AllFieldsSearch.entity().getAccountId(), Op.EQ); | |
AllFieldsSearch.and("related", AllFieldsSearch.entity().getRelated(), Op.EQ); | |
AllFieldsSearch.and("guestType", AllFieldsSearch.entity().getGuestType(), Op.EQ); | |
AllFieldsSearch.and("physicalNetwork", AllFieldsSearch.entity().getPhysicalNetworkId(), Op.EQ); | |
AllFieldsSearch.and("broadcastUri", AllFieldsSearch.entity().getBroadcastUri(), Op.EQ); | |
AllFieldsSearch.and("vpcId", AllFieldsSearch.entity().getVpcId(), Op.EQ); | |
AllFieldsSearch.and("aclId", AllFieldsSearch.entity().getNetworkACLId(), Op.EQ); | |
AllFieldsSearch.and("redundant", AllFieldsSearch.entity().isRedundant(), Op.EQ); | |
final SearchBuilder<NetworkOfferingVO> join1 = _ntwkOffDao.createSearchBuilder(); | |
join1.and("isSystem", join1.entity().isSystemOnly(), Op.EQ); | |
join1.and("isRedundant", join1.entity().isRedundantRouter(), Op.EQ); | |
AllFieldsSearch.join("offerings", join1, AllFieldsSearch.entity().getNetworkOfferingId(), join1.entity().getId(), JoinBuilder.JoinType.INNER); | |
AllFieldsSearch.done(); |
Therefore, we need to use physicalNetwork
in the SearchCriteria
in order to add the validation to the where clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right, very confusing, but I shouldn´t rely on naming conventions ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code LGTM
@blueorangutan package |
@sureshanaparti a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result: ✖️ el7 ✔️ el8 ✔️ debian ✖️ suse15. SL-JID 3670 |
@blueorangutan package |
@sureshanaparti a Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 3673 |
@blueorangutan test |
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
Trillian test result (tid-4402)
|
Description
While deleting a traffic type, ACS validates if there is any VM related to it. However, if we have several physical networks containing a traffic type, ACS does not filter the physical network to do the validation. For instance, if we have two (2) physical networks containing the traffic type Guest, the first one having VMs related, and the second not having VMs related, if we try to remove the second traffic type, ACS give us the message
The Traffic Type is not deletable because there are existing networks with this traffic type:Guest
.The API
deleteTrafficType
was designed to filter the physical network where the traffic type is, however, due to a typo this filtering was not been applied correctly. This PR intends to fix this typo to honor the API behavior.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
In an advanced zone I created 4 physical networks, one for each traffic type (Public, Guest, Management, Storage). I instantiated some VMs so they get guest IPs. In the Public physical network I added a Guest traffic type. I tried to remove the new Guest traffic type from Public physical network, which did not have any VMs related to it, and, before the changes, I was getting the message
The Traffic Type is not deletable because there are existing networks with this traffic type:Guest
. After the changes, I could remove successfully the new Guest traffic type via API deleteTrafficType. I also tried to remove the Guest traffic type which had VMs related to it, however, as expected, I received theThe Traffic Type is not deletable...
message.I also created a unit test to validate the data retrieving.