Bug fix in displaying public IP address of shared networks by ravening · Pull Request #4675 · apache/cloudstack

ravening · 2021-02-10T10:36:26Z

Description

Public IP address dedicated to one domain should not be accessed
by other domains. Also root admin should be able to display all
public ip address in system.

Currently following issues exist

Public IP address assigned to one domain can be accessed by
other sibling domains

If use.system.public.ip is false then child domains should not
see public ip of ROOT domain

Before fix

(test1) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress,
{
  "count": 59,
  "publicipaddress": [

After fix

(test) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress,
{
  "count": 10,

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

shwstppr · 2021-02-15T08:24:07Z

@ravening conflict here. Should this go in 4.15.1.0 as it is a bug fix?

DaanHoogland · 2021-02-16T14:38:08Z

@blueorangutan package

blueorangutan · 2021-02-16T14:39:32Z

@DaanHoogland a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-02-16T15:16:57Z

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2717

DaanHoogland

some remarks, haven't had time to review it fully yet

engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java

server/src/main/java/com/cloud/api/ApiResponseHelper.java

DaanHoogland · 2021-02-16T18:02:32Z

@blueorangutan test

blueorangutan · 2021-02-16T18:03:38Z

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-02-17T04:11:20Z

Trillian test result (tid-3556)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 34467 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t3556-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_network.py
Smoke tests completed. 85 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_public_ip_admin_account	`Failure`	32.31	test_network.py
test_public_ip_user_account	`Failure`	2.29	test_network.py
test_releaseIP	`Failure`	663.16	test_network.py

DaanHoogland · 2021-02-18T10:28:53Z

@ravening can you look at those failures ^^?

ravening · 2021-02-18T10:30:12Z

@ravening can you look at those failures ^^?

@DaanHoogland will look into it

yadvr · 2021-03-01T13:26:52Z

There's also a conflict @ravening

yadvr · 2021-03-11T08:30:42Z

@shwstppr can you help resolve conflict and then kick tests. Thanks.

shwstppr · 2021-03-11T08:53:29Z

@rhtyd I've fixed the conflicts but test failures from previous run doesn't seem intermittent and would need investigation/changes cc @ravening

2021-02-16 21:39:22,806 - CRITICAL - FAILED: test_public_ip_admin_account: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 236, in test_public_ip_admin_account\n    self.fail("list public ip response is not empty")\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 450, in fail\n    raise self.failureException(msg)\n', 'AssertionError: list public ip response is not empty\n']
2021-02-16 21:39:25,098 - CRITICAL - FAILED: test_public_ip_user_account: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 285, in test_public_ip_user_account\n    "Check if disassociated IP Address is no longer available"\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 553, in assertEqual\n    assertion_func(first, second, msg=msg)\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 546, in _baseAssertEqual\n    raise self.failureException(msg)\n', 'AssertionError: Check if disassociated IP Address is no longer available\n']
2021-02-16 21:56:14,340 - CRITICAL - FAILED: test_releaseIP: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 894, in test_releaseIP\n    "Failed to disassociate IP address")\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 462, in assertTrue\n    raise self.failureException(msg)\n', 'AssertionError: Failed to disassociate IP address\n']

@blueorangutan package

blueorangutan · 2021-03-11T08:54:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]

Public IP address dedicated to one domain should not be accessed by other domains. Also root admin should be able to display all public ip address in system. Currently following issues exist 1. Public IP address assigned to one domain can be accessed by other sibling domains If use.system.public.ip is false then child domains should not see public ip of ROOT domain Before fix ``` (test1) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress, { "count": 59, "publicipaddress": [ ``` After fix ``` (test) mgt01 > list publicipaddresses listall=true fordisplay=true allocatedonly=false forvirtualnetwork=true filter=ipaddress, { "count": 10, ```

blueorangutan · 2021-03-11T09:22:45Z

Packaging result: ✔️ centos7 ✖️ centos8 ✔️ debian. SL-JID 87

ravening · 2021-03-11T10:42:02Z

@rhtyd I've fixed the conflicts but test failures from previous run doesn't seem intermittent and would need investigation/changes cc @ravening

2021-02-16 21:39:22,806 - CRITICAL - FAILED: test_public_ip_admin_account: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 236, in test_public_ip_admin_account\n    self.fail("list public ip response is not empty")\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 450, in fail\n    raise self.failureException(msg)\n', 'AssertionError: list public ip response is not empty\n']
2021-02-16 21:39:25,098 - CRITICAL - FAILED: test_public_ip_user_account: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 285, in test_public_ip_user_account\n    "Check if disassociated IP Address is no longer available"\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 553, in assertEqual\n    assertion_func(first, second, msg=msg)\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 546, in _baseAssertEqual\n    raise self.failureException(msg)\n', 'AssertionError: Check if disassociated IP Address is no longer available\n']
2021-02-16 21:56:14,340 - CRITICAL - FAILED: test_releaseIP: ['Traceback (most recent call last):\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 369, in run\n    testMethod()\n', '  File "/marvin/tests/smoke/test_network.py", line 894, in test_releaseIP\n    "Failed to disassociate IP address")\n', '  File "/usr/lib64/python2.7/unittest/case.py", line 462, in assertTrue\n    raise self.failureException(msg)\n', 'AssertionError: Failed to disassociate IP address\n']

@shwstppr I have pushed a commit. can you retrigger the tests to see if that fixes?

shwstppr · 2021-03-11T10:57:40Z

okay @ravening
@blueorangutan package

blueorangutan · 2021-03-11T10:58:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]

yadvr · 2021-03-12T08:35:07Z

@Spaceman1984 can you help manually test/verify this? cc @shwstppr Thanks.

shwstppr · 2021-03-12T09:27:33Z

@ravening still some issue

Spaceman1984 · 2021-03-12T09:42:54Z

@Spaceman1984 can you help manually test/verify this? cc @shwstppr Thanks.

I'll test @rhtyd

ravening · 2021-03-12T09:54:23Z

@ravening still some issue

@shwstppr found the issue.. without the change, if ip address is in free state, its not displayed by list publicipaddress command but with this change, it will be displayed. so need to pass an extra param state=Allocated. Then its not displayed

shwstppr · 2021-03-18T06:47:27Z

@blueorangutan package

blueorangutan · 2021-03-18T06:48:03Z

@shwstppr a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress. [S]

blueorangutan · 2021-03-18T07:06:55Z

Packaging result: ✖️ centos7 ✔️ centos8 ✔️ debian. SL-JID 168

server/src/main/java/com/cloud/server/ManagementServerImpl.java

yadvr · 2021-03-24T07:05:21Z

@blueorangutan package

blueorangutan · 2021-03-24T07:06:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-03-24T07:20:40Z

Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 222

yadvr · 2021-03-24T07:26:12Z

@blueorangutan package

blueorangutan · 2021-03-24T07:27:08Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-03-24T07:37:07Z

Packaging result: ✖️ centos7 ✖️ centos8 ✖️ debian. SL-JID 230

yadvr · 2021-03-24T11:09:06Z

@blueorangutan package

blueorangutan · 2021-03-24T11:10:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-03-24T11:38:37Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 233

shwstppr · 2021-03-25T05:44:49Z

@blueorangutan test

blueorangutan · 2021-03-25T05:46:02Z

@shwstppr a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-03-25T06:48:32Z

Trillian test result (tid-253)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40948 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t253-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermittent failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_deploy_kubernetes_cluster	`Failure`	3602.73	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	71.83	test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume	`Error`	67.20	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	51.12	test_vm_life_cycle.py

blueorangutan · 2021-03-25T16:05:25Z

Trillian test result (tid-259)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 35585 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4675-t259-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Smoke tests completed. 84 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	70.88	test_kubernetes_clusters.py
test_01_migrate_VM_and_root_volume	`Error`	67.17	test_vm_life_cycle.py
test_02_migrate_VM_with_two_data_disks	`Error`	50.06	test_vm_life_cycle.py

yadvr · 2021-03-29T10:47:51Z

Ping @ravening @shwstppr @Spaceman1984 what's the update on this? Thanks

shwstppr · 2021-03-31T03:43:57Z

Manually tested, fixes #4723

shwstppr added this to the 4.15.1.0 milestone Feb 15, 2021

ravening force-pushed the public-ip-range branch from f936db9 to 2330cb2 Compare February 16, 2021 12:05

ravening changed the base branch from master to 4.15 February 16, 2021 12:05

DaanHoogland requested review from GabrielBrascher, weizhouapache and wido February 16, 2021 15:00

DaanHoogland added the component:networking label Feb 16, 2021

DaanHoogland reviewed Feb 16, 2021

View reviewed changes

engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java Show resolved Hide resolved

server/src/main/java/com/cloud/api/ApiResponseHelper.java Show resolved Hide resolved

server/src/main/java/com/cloud/api/ApiResponseHelper.java Outdated Show resolved Hide resolved

shwstppr added the status:needs-testing label Feb 22, 2021

weizhouapache mentioned this pull request Mar 1, 2021

listPublicIpAddresses API lists the (unallocated) reserved public IPs of other account, and resulting in UI to show other account's public IPs while acquire new IPs in the network #4723

Closed

ravening force-pushed the public-ip-range branch from 532a7e4 to a04080d Compare March 11, 2021 08:59

fix test failures

b059220

fix test failures

0c98634

shwstppr reviewed Mar 18, 2021

View reviewed changes

server/src/main/java/com/cloud/server/ManagementServerImpl.java Show resolved Hide resolved

weizhouapache mentioned this pull request Mar 23, 2021

API command listPublicIpAddresses lists IPs only with forvirtualnetwork=true #4853

Closed

yadvr assigned shwstppr Mar 24, 2021

shwstppr approved these changes Mar 31, 2021

View reviewed changes

yadvr approved these changes Apr 1, 2021

View reviewed changes

yadvr merged commit 76ba5c6 into apache:4.15 Apr 1, 2021

DaanHoogland removed the status:needs-testing label Jan 28, 2026

Conversation

ravening commented Feb 10, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

shwstppr commented Feb 15, 2021

Uh oh!

DaanHoogland commented Feb 16, 2021

Uh oh!

blueorangutan commented Feb 16, 2021

Uh oh!

blueorangutan commented Feb 16, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Feb 16, 2021

Uh oh!

blueorangutan commented Feb 16, 2021

Uh oh!

blueorangutan commented Feb 17, 2021

Uh oh!

DaanHoogland commented Feb 18, 2021

Uh oh!

ravening commented Feb 18, 2021

Uh oh!

yadvr commented Mar 1, 2021

Uh oh!

yadvr commented Mar 11, 2021

Uh oh!

shwstppr commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

ravening commented Mar 11, 2021

Uh oh!

shwstppr commented Mar 11, 2021

Uh oh!

blueorangutan commented Mar 11, 2021

Uh oh!

yadvr commented Mar 12, 2021

Uh oh!

shwstppr commented Mar 12, 2021

Uh oh!

Spaceman1984 commented Mar 12, 2021

Uh oh!

ravening commented Mar 12, 2021

Uh oh!

shwstppr commented Mar 18, 2021

Uh oh!

blueorangutan commented Mar 18, 2021

Uh oh!

blueorangutan commented Mar 18, 2021

Uh oh!

Uh oh!

yadvr commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

yadvr commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021

Uh oh!

yadvr commented Mar 24, 2021

Uh oh!

blueorangutan commented Mar 24, 2021