server: allow user to list available IPs on shared networks#7898
server: allow user to list available IPs on shared networks#7898yadvr merged 1 commit intoapache:4.18from
Conversation
| "Set placement of vrouter ips in redundant mode in vpc tiers, this can be 3 value: `first` to use first ips in tiers, `last` to use last ips in tiers and `random` to take random ips in tiers.", | ||
| true, ConfigKey.Scope.Account, null, null, null, null, null, ConfigKey.Kind.Select, "first,last,random"); | ||
|
|
||
| ConfigKey<Boolean> AllowUserListAvailableIpsOnSharedNetwork = new ConfigKey<Boolean>("Advanced", Boolean.class, "allow.user.list.available.ips.on.shared.network", "false", |
There was a problem hiding this comment.
Could this be setup to true by default; it's because users can already see public IPs say in an isolated network or VPC. Only stricter env can disable it.
There was a problem hiding this comment.
Also worth testing in the UI -> that users can select shared network IP when adding NIC for example, or deploy a VM? cc @kiranchavala
There was a problem hiding this comment.
@rohityadavcloud
some cloud providers might dislike it (as true) , for security reasons.
the UI changes requires some other changes, as normal users cannot list the global setting . we need to add it as a capability of the platform.
Codecov Report
@@ Coverage Diff @@
## 4.18 #7898 +/- ##
============================================
- Coverage 13.06% 13.05% -0.01%
Complexity 9083 9083
============================================
Files 2720 2720
Lines 257289 257381 +92
Branches 40116 40126 +10
============================================
+ Hits 33610 33613 +3
- Misses 219458 219547 +89
Partials 4221 4221
... and 6 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@blueorangutan package |
|
@weizhouapache a [SF] 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. |
|
@weizhouapache have you tested, if say once a user lists and find a free IP address - does deployVM and add nic to network APIs honour user-supplied public IP address? |
@rohityadavcloud |
|
@blueorangutan test keepEnv |
|
@DaanHoogland a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-7526)
|
@rohityadavcloud
|
|
Excellent thanks @weizhouapache let's go ahead and merge this once it meets enought reviews; cc @DaanHoogland |
| } else if (vlanType == VlanType.DirectAttached && network != null && !isAllocatedTemp && isAllocated) { | ||
| if (caller.getType() != Account.Type.ADMIN && !IpAddressManager.AllowUserListAvailableIpsOnSharedNetwork.value()) { | ||
| s_logger.debug("Non-admin users are not allowed to list available IPs on shared networks"); | ||
| } else { | ||
| final SearchBuilder<IPAddressVO> searchBuilder = _publicIpAddressDao.createSearchBuilder(); | ||
| buildParameters(searchBuilder, cmd, false); | ||
|
|
||
| SearchCriteria<IPAddressVO> searchCriteria = searchBuilder.create(); | ||
| setParameters(searchCriteria, cmd, vlanType, false); | ||
| searchCriteria.setParameters("state", IpAddress.State.Free.name()); | ||
| addrs.addAll(_publicIpAddressDao.search(searchCriteria, searchFilter)); // Free IPs on shared network | ||
| } |
There was a problem hiding this comment.
can we move this to a separate method, and the inner else-clause to the Dao?
There was a problem hiding this comment.
@DaanHoogland
moving to Dao seems difficult, as this code block calls two methods to build and set search parameters: buildParameters and setParameters
There was a problem hiding this comment.
all done in searchForIpAddresses. this is out off scope but definitely needs factoring.
Description
This fixes #7817
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?