Skip to content

server: allow user to list available IPs on shared networks #7898

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.

Sign up for GitHub

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

Merged
yadvr merged 1 commit into from
Aug 24, 2023
Merged

server: allow user to list available IPs on shared networks #7898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ public interface IpAddressManager {
"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",
Copy link
Copy Markdown
Member

@yadvr yadvr Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@yadvr yadvr Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also worth testing in the UI -> that users can select shared network IP when adding NIC for example, or deploy a VM? cc @kiranchavala

Copy link
Copy Markdown
Member Author

@weizhouapache weizhouapache Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

"Determines whether users can list available IPs on shared networks",
true, ConfigKey.Scope.Global);

/**
* Assigns a new public ip address.
*
Expand Down
2 changes: 1 addition & 1 deletion server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2342,7 +2342,7 @@ public String getConfigComponentName() {

@Override
public ConfigKey<?>[] getConfigKeys() {
return new ConfigKey<?>[] {UseSystemPublicIps, RulesContinueOnError, SystemVmPublicIpReservationModeStrictness, VrouterRedundantTiersPlacement};
return new ConfigKey<?>[] {UseSystemPublicIps, RulesContinueOnError, SystemVmPublicIpReservationModeStrictness, VrouterRedundantTiersPlacement, AllowUserListAvailableIpsOnSharedNetwork};
}

/**
Expand Down
16 changes: 15 additions & 1 deletion server/src/main/java/com/cloud/server/ManagementServerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2323,6 +2323,7 @@ public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListP
isAllocated = Boolean.TRUE;
}
}
boolean isAllocatedTemp = isAllocated;

VlanType vlanType = null;
if (forVirtualNetwork != null) {
Expand All @@ -2333,6 +2334,7 @@ public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListP

final Account caller = getCaller();
List<IPAddressVO> addrs = new ArrayList<>();
NetworkVO network = null; // shared network

if (vlanType == VlanType.DirectAttached && networkId == null && ipId == null) { // only root admin can list public ips in all shared networks
if (caller.getType() != Account.Type.ADMIN) {
Expand All @@ -2341,7 +2343,6 @@ public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListP
} else if (vlanType == VlanType.DirectAttached) {
// list public ip address on shared network
// access control. admin: all Ips, domain admin/user: all Ips in shared network in the domain/sub-domain/user
NetworkVO network = null;
if (networkId == null) {
IPAddressVO ip = _publicIpAddressDao.findById(ipId);
if (ip == null) {
Expand Down Expand Up @@ -2475,7 +2476,20 @@ public Pair<List<? extends IpAddress>, Integer> searchForIPAddresses(final ListP
for (IPAddressVO addr: freeAddrs) {
freeAddrIds.add(addr.getId());
}
} 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
}
Comment on lines +2479 to +2490
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we move this to a separate method, and the inner else-clause to the Dao?

Copy link
Copy Markdown
Member Author

@weizhouapache weizhouapache Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DaanHoogland
moving to Dao seems difficult, as this code block calls two methods to build and set search parameters: buildParameters and setParameters

Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland Aug 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all done in searchForIpAddresses. this is out off scope but definitely needs factoring.

}

if (freeAddrIds.size() > 0) {
final SearchBuilder<IPAddressVO> sb2 = _publicIpAddressDao.createSearchBuilder();
buildParameters(sb2, cmd, false);
Expand Down