You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by kansal <gi...@git.apache.org> on 2015/11/26 10:23:06 UTC

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

GitHub user kansal opened a pull request:

    https://github.com/apache/cloudstack/pull/1125

    CLOUDSTACK-9086: ACS allows to create isolated networks with invalid …

    …gateway IP address - Fixed
    
    Problem: There was no check for the network and broadcast IP addresses in the case where we provide the gateway and netmask while creating the isolated network. As a result the provided IP gets assigned to the eth0 interface of the VR.
    
    Fix: Added the necessary checks.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/kansal/cloudstack CLOUDSTACK-9086

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/cloudstack/pull/1125.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1125
    
----
commit 27b40e761a398565b1e2a1947c6ba8634c497e40
Author: Kshitij Kansal <ka...@gmail.com>
Date:   2015-11-26T06:56:29Z

    CLOUDSTACK-9086: ACS allows to create isolated networks with invalid gateway IP address - Fixed

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by remibergsma <gi...@git.apache.org>.
Github user remibergsma commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-161919700
  
    LGTM based on these tests:
    
    ```
    nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=true \
    component/test_vpc_redundant.py \
    component/test_routers_iptables_default_policy.py \
    component/test_routers_network_ops.py \
    component/test_vpc_router_nics.py \
    smoke/test_loadbalance.py \
    smoke/test_internal_lb.py \
    smoke/test_ssvm.py \
    smoke/test_network.py
    
    ```
    
    Result:
    
    ```
    Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL | Status : SUCCESS ===
    ok
    Create a redundant VPC with two networks with two VMs in each network and check default routes ... === TestName: test_02_redundant_VPC_default_routes | Status : SUCCESS ===
    ok
    Create a redundant VPC with two networks with two VMs in each network ... === TestName: test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers | Status : SUCCESS ===
    ok
    Test iptables default INPUT/FORWARD policy on RouterVM ... === TestName: test_02_routervm_iptables_policies | Status : SUCCESS ===
    ok
    Test iptables default INPUT/FORWARD policies on VPC router ... === TestName: test_01_single_VPC_iptables_policies | Status : SUCCESS ===
    ok
    Test redundant router internals ... === TestName: test_01_isolate_network_FW_PF_default_routes_egress_true | Status : SUCCESS ===
    ok
    Test redundant router internals ... === TestName: test_02_isolate_network_FW_PF_default_routes_egress_false | Status : SUCCESS ===
    ok
    Test redundant router internals ... === TestName: test_01_RVR_Network_FW_PF_SSH_default_routes_egress_true | Status : SUCCESS ===
    ok
    Test redundant router internals ... === TestName: test_02_RVR_Network_FW_PF_SSH_default_routes_egress_false | Status : SUCCESS ===
    ok
    Create a VPC with two networks with one VM in each network and test nics after destroy ... === TestName: test_01_VPC_nics_after_destroy | Status : SUCCESS ===
    ok
    Create a VPC with two networks with one VM in each network and test default routes ... === TestName: test_02_VPC_default_routes | Status : SUCCESS ===
    ok
    Check the password file in the Router VM ... === TestName: test_isolate_network_password_server | Status : SUCCESS ===
    ok
    Check that the /etc/dhcphosts.txt doesn't contain duplicate IPs ... === TestName: test_router_dhcphosts | Status : SUCCESS ===
    ok
    Test to create Load balancing rule with source NAT ... === TestName: test_01_create_lb_rule_src_nat | Status : SUCCESS ===
    ok
    Test to create Load balancing rule with non source NAT ... === TestName: test_02_create_lb_rule_non_nat | Status : SUCCESS ===
    ok
    Test for assign & removing load balancing rule ... === TestName: test_assign_and_removal_lb | Status : SUCCESS ===
    ok
    Test to verify access to loadbalancer haproxy admin stats page ... === TestName: test02_internallb_haproxy_stats_on_all_interfaces | Status : SUCCESS ===
    ok
    Test create, assign, remove of an Internal LB with roundrobin http traffic to 3 vm's ... === TestName: test_01_internallb_roundrobin_1VPC_3VM_HTTP_port80 | Status : SUCCESS ===
    ok
    Test SSVM Internals ... === TestName: test_03_ssvm_internals | Status : SUCCESS ===
    ok
    Test CPVM Internals ... === TestName: test_04_cpvm_internals | Status : SUCCESS ===
    ok
    Test stop SSVM ... === TestName: test_05_stop_ssvm | Status : SUCCESS ===
    ok
    Test stop CPVM ... === TestName: test_06_stop_cpvm | Status : SUCCESS ===
    ok
    Test reboot SSVM ... === TestName: test_07_reboot_ssvm | Status : SUCCESS ===
    ok
    Test reboot CPVM ... === TestName: test_08_reboot_cpvm | Status : SUCCESS ===
    ok
    Test destroy SSVM ... === TestName: test_09_destroy_ssvm | Status : SUCCESS ===
    ok
    Test destroy CPVM ... === TestName: test_10_destroy_cpvm | Status : SUCCESS ===
    ok
    Test Remote Access VPN in VPC ... === TestName: test_vpc_remote_access_vpn | Status : SUCCESS ===
    ok
    Test VPN in VPC ... === TestName: test_vpc_site2site_vpn | Status : SUCCESS ===
    ok
    Test for port forwarding on source NAT ... === TestName: test_01_port_fwd_on_src_nat | Status : SUCCESS ===
    ok
    Test for port forwarding on non source NAT ... === TestName: test_02_port_fwd_on_non_src_nat | Status : SUCCESS ===
    ok
    Test for reboot router ... === TestName: test_reboot_router | Status : SUCCESS ===
    ok
    Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_1_static_nat_rule | Status : SUCCESS ===
    ok
    Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_2_nat_rule | Status : SUCCESS ===
    ok
    Test for Router rules for network rules on acquired public IP ... === TestName: test_network_rules_acquired_public_ip_3_Load_Balancer_Rule | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 34 tests in 16205.787s
    
    OK
    ```
    
    
    And:
    
    ```
    nosetests --with-marvin --marvin-config=${marvinCfg} -s -a tags=advanced,required_hardware=false \
    smoke/test_routers.py \
    smoke/test_network_acl.py \
    smoke/test_privategw_acl.py \
    smoke/test_reset_vm_on_reboot.py \
    smoke/test_vm_life_cycle.py \
    smoke/test_vpc_vpn.py \
    smoke/test_service_offerings.py \
    component/test_vpc_offerings.py \
    component/test_vpc_routers.py
    ```
    
    Result:
    
    ```
    Test router internal advanced zone ... === TestName: test_02_router_internal_adv | Status : SUCCESS ===
    ok
    Test restart network ... === TestName: test_03_restart_network_cleanup | Status : SUCCESS ===
    ok
    Test router basic setup ... === TestName: test_05_router_basic | Status : SUCCESS ===
    ok
    Test router advanced setup ... === TestName: test_06_router_advanced | Status : SUCCESS ===
    ok
    Test stop router ... === TestName: test_07_stop_router | Status : SUCCESS ===
    ok
    Test start router ... === TestName: test_08_start_router | Status : SUCCESS ===
    ok
    Test reboot router ... === TestName: test_09_reboot_router | Status : SUCCESS ===
    ok
    test_privategw_acl (integration.smoke.test_privategw_acl.TestPrivateGwACL) ... === TestName: test_privategw_acl | Status : SUCCESS ===
    ok
    Test reset virtual machine on reboot ... === TestName: test_01_reset_vm_on_reboot | Status : SUCCESS ===
    ok
    Test advanced zone virtual router ... === TestName: test_advZoneVirtualRouter | Status : SUCCESS ===
    ok
    Test Deploy Virtual Machine ... === TestName: test_deploy_vm | Status : SUCCESS ===
    ok
    Test Multiple Deploy Virtual Machine ... === TestName: test_deploy_vm_multiple | Status : SUCCESS ===
    ok
    Test Stop Virtual Machine ... === TestName: test_01_stop_vm | Status : SUCCESS ===
    ok
    Test Start Virtual Machine ... === TestName: test_02_start_vm | Status : SUCCESS ===
    ok
    Test Reboot Virtual Machine ... === TestName: test_03_reboot_vm | Status : SUCCESS ===
    ok
    Test destroy Virtual Machine ... === TestName: test_06_destroy_vm | Status : SUCCESS ===
    ok
    Test recover Virtual Machine ... === TestName: test_07_restore_vm | Status : SUCCESS ===
    ok
    Test migrate VM ... === TestName: test_08_migrate_vm | Status : SUCCESS ===
    ok
    Test destroy(expunge) Virtual Machine ... === TestName: test_09_expunge_vm | Status : SUCCESS ===
    ok
    Test to create service offering ... === TestName: test_01_create_service_offering | Status : SUCCESS ===
    ok
    Test to update existing service offering ... === TestName: test_02_edit_service_offering | Status : SUCCESS ===
    ok
    Test to delete service offering ... === TestName: test_03_delete_service_offering | Status : SUCCESS ===
    ok
    Test for delete account ... === TestName: test_delete_account | Status : SUCCESS ===
    ok
    Test for Associate/Disassociate public IP address for admin account ... === TestName: test_public_ip_admin_account | Status : SUCCESS ===
    ok
    Test for Associate/Disassociate public IP address for user account ... === TestName: test_public_ip_user_account | Status : SUCCESS ===
    ok
    Test for release public IP address ... === TestName: test_releaseIP | Status : SUCCESS ===
    ok
    Test create VPC offering ... === TestName: test_01_create_vpc_offering | Status : SUCCESS ===
    ok
    Test VPC offering without load balancing service ... === TestName: test_03_vpc_off_without_lb | Status : SUCCESS ===
    ok
    Test VPC offering without static NAT service ... === TestName: test_04_vpc_off_without_static_nat | Status : SUCCESS ===
    ok
    Test VPC offering without port forwarding service ... === TestName: test_05_vpc_off_without_pf | Status : SUCCESS ===
    ok
    Test VPC offering with invalid services ... === TestName: test_06_vpc_off_invalid_services | Status : SUCCESS ===
    ok
    Test update VPC offering ... === TestName: test_07_update_vpc_off | Status : SUCCESS ===
    ok
    Test list VPC offering ... === TestName: test_08_list_vpc_off | Status : SUCCESS ===
    test_09_create_redundant_vpc_offering (integration.component.test_vpc_offerings.TestVPCOffering) ... === TestName: test_09_create_redundant_vpc_offering | Status : SUCCESS ===
    ok
    Test start/stop of router after addition of one guest network ... === TestName: test_01_start_stop_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test reboot of router after addition of one guest network ... === TestName: test_02_reboot_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to change service offering of router after addition of one guest network ... === TestName: test_04_chg_srv_off_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test destroy of router after addition of one guest network ... === TestName: test_05_destroy_router_after_addition_of_one_guest_network | Status : SUCCESS ===
    ok
    Test to stop and start router after creation of VPC ... === TestName: test_01_stop_start_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Test to reboot the router after creating a VPC ... === TestName: test_02_reboot_router_after_creating_vpc | Status : SUCCESS ===
    ok
    Tests to change service offering of the Router after ... === TestName: test_04_change_service_offerring_vpc | Status : SUCCESS ===
    ok
    Test to destroy the router after creating a VPC ... === TestName: test_05_destroy_router_after_creating_vpc | Status : SUCCESS ===
    ok
    
    ----------------------------------------------------------------------
    Ran 42 tests in 8062.366s
    
    OK
    ```
    
    Haven't tested the actual functionality, this just shows you didn't break anything else.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by bhaisaab <gi...@git.apache.org>.
Github user bhaisaab commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162415947
  
    @kansal can you address some of the issues Daan has mentioned, if you can fix them today I help can review and merge this. Thanks.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162480371
  
    Not sure if it is related but
    
    ```
    Running com.cloud.utils.net.NetUtilsTest
    2015-12-07 11:33:08,471 INFO  [utils.net.NetUtils] (main:) Invalid value of cidr 10.3.6.5/50
    2015-12-07 11:33:08,480 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::1
    2015-12-07 11:33:08,480 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::2
    2015-12-07 11:33:08,483 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::1
    2015-12-07 11:33:08,488 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::1
    2015-12-07 11:33:08,490 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::1
    2015-12-07 11:33:08,492 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::99d6:578:c6d0:4d1e
    2015-12-07 11:33:08,497 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::c2ad:5357:a636:5465
    2015-12-07 11:33:08,500 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::f54c:4cd1:fafc:6d08
    2015-12-07 11:33:08,509 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::54d1:5897:eba:aced
    2015-12-07 11:33:08,511 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::a924:8a89:5af3:2039
    2015-12-07 11:33:08,512 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::1442:6ab4:8a7f:1495
    2015-12-07 11:33:08,518 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::e319:8c5f:b31c:eda1
    2015-12-07 11:33:08,520 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::e380:62c1:5705:769f
    2015-12-07 11:33:08,521 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::a764:8dd3:1f33:bc29
    2015-12-07 11:33:08,522 INFO  [utils.net.NetUtilsTest] (main:) IP is 1234:5678::9b2d:3bd9:7c1c:3df1
    2015-12-07 11:33:08,564 ERROR [utils.net.NetUtils] (main:) Failed to convert a string to an IPv6 address
    java.lang.IllegalArgumentException: can not parse []
    	at com.googlecode.ipv6.IPv6Address.tryParseStringArrayIntoLongArray(IPv6Address.java:94)
    	at com.googlecode.ipv6.IPv6Address.fromString(IPv6Address.java:80)
    	at com.cloud.utils.net.NetUtils.countIp6InRange(NetUtils.java:1317)
    	at com.cloud.utils.net.NetUtilsTest.testCountIp6InRangeWithNullStart(NetUtilsTest.java:152)
    	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    	at java.lang.reflect.Method.invoke(Method.java:497)
    	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:47)
    	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:44)
    	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:271)
    	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:70)
    	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:50)
    	at org.junit.runners.ParentRunner$3.run(ParentRunner.java:238)
    	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:63)
    	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:236)
    	at org.junit.runners.ParentRunner.access$000(ParentRunner.java:53)
    	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:229)
    	at org.junit.runners.ParentRunner.run(ParentRunner.java:309)
    	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:283)
    	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:173)
    	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:153)
    	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:128)
    	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:203)
    	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:155)
    	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:103)
    Tests run: 40, Failures: 3, Errors: 0, Skipped: 0, Time elapsed: 0.143 sec <<< FAILURE! - in com.cloud.utils.net.NetUtilsTest
    test31BitPrefixEnd(com.cloud.utils.net.NetUtilsTest)  Time elapsed: 0.019 sec  <<< FAILURE!
    java.lang.AssertionError: Check if the subnetUtils.setInclusiveHostCount(true) has been called.
    	at org.junit.Assert.fail(Assert.java:88)
    	at org.junit.Assert.assertTrue(Assert.java:41)
    	at com.cloud.utils.net.NetUtilsTest.test31BitPrefixEnd(NetUtilsTest.java:362)
    
    test31BitPrefixFail(com.cloud.utils.net.NetUtilsTest)  Time elapsed: 0.001 sec  <<< FAILURE!
    java.lang.AssertionError: Out of the range. Why did it return true?
    	at org.junit.Assert.fail(Assert.java:88)
    	at org.junit.Assert.assertTrue(Assert.java:41)
    	at org.junit.Assert.assertFalse(Assert.java:64)
    	at com.cloud.utils.net.NetUtilsTest.test31BitPrefixFail(NetUtilsTest.java:372)
    
    test31BitPrefixStart(com.cloud.utils.net.NetUtilsTest)  Time elapsed: 0.001 sec  <<< FAILURE!
    java.lang.AssertionError: Check if the subnetUtils.setInclusiveHostCount(true) has been called.
    	at org.junit.Assert.fail(Assert.java:88)
    	at org.junit.Assert.assertTrue(Assert.java:41)
    	at com.cloud.utils.net.NetUtilsTest.test31BitPrefixStart(NetUtilsTest.java:352)
    ```
    
    ```
    Results :
    
    Failed tests:
      NetUtilsTest.test31BitPrefixEnd:362 Check if the subnetUtils.setInclusiveHostCount(true) has been called.
      NetUtilsTest.test31BitPrefixFail:372 Out of the range. Why did it return true?
      NetUtilsTest.test31BitPrefixStart:352 Check if the subnetUtils.setInclusiveHostCount(true) has been called.
    ```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162471051
  
    cc @bhaisaab @DaanHoogland . Added test cases and made some minor changes to the function. Please have a look.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162490625
  
    @DaanHoogland I see a test case in the NetUtilsTest.java class' testIsSameIpRange() function. 
     assertFalse(NetUtils.isSameIpRange(cidrFirst, "10.3.6.5/50"));
    
    I think this is the one causing the problem? 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by kansal <gi...@git.apache.org>.
Github user kansal closed the pull request at:

    https://github.com/apache/cloudstack/pull/1125


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46672870
  
    --- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
    @@ -1526,6 +1527,14 @@ public static boolean isIpWithtInCidrRange(final String ipAddress, final String
             return isInRange;
         }
     
    +    public static boolean isNetworkorBroadcastIP(String ip, String netmask){
    --- End diff --
    
    Can you add unit tests for this function?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46672603
  
    --- Diff: server/src/com/cloud/network/NetworkServiceImpl.java ---
    @@ -1197,6 +1197,10 @@ public Network createGuestNetwork(CreateNetworkCmd cmd) throws InsufficientCapac
                 }
     
                 if (gateway != null && netmask != null) {
    +                if(!NetUtils.isNetworkorBroadcastIP(gateway,netmask)) {
    +                    s_logger.debug("The gateway IP provided is "+ gateway + " and netmask is "+ netmask + ". The IP is either broadcast or network IP.");
    --- End diff --
    
    please, check for isDebugEnabled on s_logger


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162497602
  
    it is the NetUtilsTest.test31BitPrefix...() family of tests that are failing


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:

    https://github.com/apache/cloudstack/pull/1125#issuecomment-162474888
  
    LGTM
    I will run a test build on this
    also github reports conflicts on this branch, did you fork this of master? and when? It may need a rebase to merge with adjacent changes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46805977
  
    --- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
    @@ -1523,7 +1524,15 @@ public static boolean isIpWithtInCidrRange(final String ipAddress, final String
     
             final boolean isInRange = subnetUtils.getInfo().isInRange(ipAddress);
     
    -        return isInRange;
    +        return !isInRange;
    --- End diff --
    
    why did you reverse this return value @kansal ? I missed it in the first itteration


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46672736
  
    --- Diff: server/src/com/cloud/network/NetworkServiceImpl.java ---
    @@ -1197,6 +1197,10 @@ public Network createGuestNetwork(CreateNetworkCmd cmd) throws InsufficientCapac
                 }
     
                 if (gateway != null && netmask != null) {
    +                if(!NetUtils.isNetworkorBroadcastIP(gateway,netmask)) {
    +                    s_logger.debug("The gateway IP provided is "+ gateway + " and netmask is "+ netmask + ". The IP is either broadcast or network IP.");
    --- End diff --
    
    maybe you can change the name of the static object to LOGGER while you are at it?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46673005
  
    --- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
    @@ -1526,6 +1527,14 @@ public static boolean isIpWithtInCidrRange(final String ipAddress, final String
             return isInRange;
         }
     
    +    public static boolean isNetworkorBroadcastIP(String ip, String netmask){
    --- End diff --
    
    @DaanHoogland Sure I can do that. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] cloudstack pull request: CLOUDSTACK-9086: ACS allows to create iso...

Posted by kansal <gi...@git.apache.org>.
Github user kansal commented on a diff in the pull request:

    https://github.com/apache/cloudstack/pull/1125#discussion_r46809694
  
    --- Diff: utils/src/main/java/com/cloud/utils/net/NetUtils.java ---
    @@ -1523,7 +1524,15 @@ public static boolean isIpWithtInCidrRange(final String ipAddress, final String
     
             final boolean isInRange = subnetUtils.getInfo().isInRange(ipAddress);
     
    -        return isInRange;
    +        return !isInRange;
    --- End diff --
    
    @DaanHoogland Nothing changes functionality wise. I reversed it so as it makes sense with the name of the function isNetworkorBroadCastIP() should return true if its a network or broadcast IP. 


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---