cloudstack
cloudstack copied to clipboard
apache
Possibility to choose the initial IP address on a isolated network or VPC
Description
This PR intends to allow users to choose an IP address for the VR of their network from the pool of available addresses.
Types of changes
- [x] 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)
- [x] 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
- [x] Major
- [ ] Minor
Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [x] Minor
- [ ] Trivial
Screenshots (if appropriate):
How Has This Been Tested?
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1696)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1925)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1927)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1928)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1937)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1940)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1951)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1959)
@acs-robot a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.
UI build: :heavy_check_mark: Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/6442 (SL-JID-1960)
SonarCloud Quality Gate failed. 
0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell
0.0% Coverage
0.0% Duplication
![sonarqubecloud[bot] avatar](https://avatars.githubusercontent.com/in/12526?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Codecov Report
Merging #6442 (6dbcee2) into main (941cc83) will increase coverage by
0.32%. The diff coverage is32.46%.
@@ Coverage Diff @@
## main #6442 +/- ##
============================================
+ Coverage 12.69% 13.01% +0.32%
- Complexity 8674 9172 +498
============================================
Files 2729 2728 -1
Lines 256579 262916 +6337
Branches 39987 42677 +2690
============================================
+ Hits 32575 34222 +1647
- Misses 219859 224397 +4538
- Partials 4145 4297 +152
| Impacted Files | Coverage Δ | |
|---|---|---|
| .../main/java/com/cloud/network/IpAddressManager.java | 100.00% <ø> (ø) |
|
| ...n/java/com/cloud/network/dao/IPAddressDaoImpl.java | 31.22% <0.00%> (ø) |
|
| ...src/main/java/com/cloud/api/ApiResponseHelper.java | 4.96% <0.00%> (+1.13%) |
:arrow_up: |
| ...com/cloud/network/NetworkMigrationManagerImpl.java | 0.28% <ø> (ø) |
|
| .../cloud/configuration/ConfigurationManagerImpl.java | 16.10% <5.55%> (+0.85%) |
:arrow_up: |
| ...ain/java/com/cloud/network/vpc/VpcManagerImpl.java | 9.37% <25.00%> (+0.70%) |
:arrow_up: |
| ...ain/java/com/cloud/network/NetworkServiceImpl.java | 12.91% <38.21%> (+1.50%) |
:arrow_up: |
| ...n/java/com/cloud/network/IpAddressManagerImpl.java | 2.90% <100.00%> (+0.77%) |
:arrow_up: |
... and 144 files with indirect coverage changes
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
@DaanHoogland is this ready for review ?
I need to do some functional testing and discuss with @alexandremattioli if this is a good intermediat state to work from.
@DaanHoogland should this be in 4.18.0.0 milestone ?
I will try to get it in, it doesn´t have high prio though, so only if it is proven correct before RC1
@DaanHoogland Do you have time to update this PR before 4.18.0.0 cut ?
@DaanHoogland Do you have time to update this PR before 4.18.0.0 cut ?
@weizhouapache , I am investigating similar errors as the tungsten PR. that one has priority of course. I do not think there is anything to update anymore, but if so, I will try.
Just FYI - there is a UI-only workaround that may even be implemented (so just a UI feature). When creating a network in the UI, ask in the same form or as a next step the IP that the user wants, and acquire that public IP. The first IP acquired in an isolated network for ex. becomes the SNAT IP :) I've been using this approach for years ;)
+1
Just FYI - there is a UI-only workaround that may even be implemented (so just a UI feature). When creating a network in the UI, ask in the same form or as a next step the IP that the user wants, and acquire that public IP. The first IP acquired in an isolated network for ex. becomes the SNAT IP :) I've been using this approach for years ;)
Just FYI - there is a UI-only workaround that may even be implemented (so just a UI feature). When creating a network in the UI, ask in the same form or as a next step the IP that the user wants, and acquire that public IP. The first IP acquired in an isolated network for ex. becomes the SNAT IP :) I've been using this approach for years ;)
It is good to have a workaround right away; however, I think we still should provide a clear and solid method for operators to define the source NAT of the network/VPC, as the workaround is only possible for not persistent networks and if we create VPCs without starting it.
I think that a better solution here would be to behave as the "add VM to network process": if no IP is provided, one is selected at random; if the IP is provided, validations are executed, and if everything is fine, the IP is used. We could receive the parameter via the createVPC/createNetwork APIs and acquire the IP for the VPC/network (if it is available) directly in the code; in the UI, we could implement the IP selection for the Create VPC and Create Network forms.
@DaanHoogland, is there any reason to consider selecting the source NAT IP as a capacity of the source NAT service? In my opinion, it is not related to the service supporting the functionality itself, but to the offering supporting the service; if the offering supports the source NAT service, then the source NAT IP can be informed, otherwise it cannot. For instance, with the current design, one could define a VPC offering without the select the source NAT IP capability, create the VPC without starting it, and still select the source NAT IP through the workaround @rohityadavcloud mentioned.
@GutoVeronezi ,
@DaanHoogland, is there any reason to consider
selecting the source NAT IPas a capacity of thesource NATservice?
In short because the offering having such a attribute when source NAT is not enabled makes no sense. Note also that this was already implemented for shared networks and the mechs are inspired on that.
In my opinion, it is not related to the service supporting the functionality itself, but to the offering supporting the service; if the offering supports the
source NATservice, then the source NAT IP can be informed, otherwise it cannot. For instance, with the current design, one could define a VPC offering without theselect the source NAT IPcapability, create the VPC without starting it, and still select the source NAT IP through the workaround @rohityadavcloud mentioned.
Yes, one could. So do you want to prevent this, and how? delecting IPs must happen and the first one will always be the source NAT one, whether it is chosen or randomly assigned. I a, still trying to define the end functionality, but I think this is a good intermediate. Does that make sense?
@GutoVeronezi ,
@DaanHoogland, is there any reason to consider
selecting the source NAT IPas a capacity of thesource NATservice?In short because the offering having such a attribute when source NAT is not enabled makes no sense.
In my opinion, it is not related to the service supporting the functionality itself, but to the offering supporting the service; if the offering supports the
source NATservice, then the source NAT IP can be informed, otherwise it cannot. For instance, with the current design, one could define a VPC offering without theselect the source NAT IPcapability, create the VPC without starting it, and still select the source NAT IP through the workaround @rohityadavcloud mentioned.Yes, one could. So do you want to prevent this, and how? delecting IPs must happen and the first one will always be the source NAT one, whether it is chosen or randomly assigned. I a, still trying to define the end functionality, but I think this is a good intermediate. Does that make sense?
@DaanHoogland, my point was that we would not need an attribute like selecting the source NAT IP in the offering; we could just validate if the offering supports the SourceNat service; if the offering supports the SourceNat service, then the source NAT IP could be informed while creating the network/VPC, otherwise, it could not. In my opinion, it is a simplier design to implement and the operators would not need to create new offerings to use the feature. What do you think?
@GutoVeronezi ,
@DaanHoogland, is there any reason to consider
selecting the source NAT IPas a capacity of thesource NATservice?In short because the offering having such a attribute when source NAT is not enabled makes no sense.
In my opinion, it is not related to the service supporting the functionality itself, but to the offering supporting the service; if the offering supports the
source NATservice, then the source NAT IP can be informed, otherwise it cannot. For instance, with the current design, one could define a VPC offering without theselect the source NAT IPcapability, create the VPC without starting it, and still select the source NAT IP through the workaround @rohityadavcloud mentioned.Yes, one could. So do you want to prevent this, and how? delecting IPs must happen and the first one will always be the source NAT one, whether it is chosen or randomly assigned. I a, still trying to define the end functionality, but I think this is a good intermediate. Does that make sense?
@DaanHoogland, my point was that we would not need an attribute like
selecting the source NAT IPin the offering; we could just validate if the offering supports theSourceNatservice; if the offering supports theSourceNatservice, then the source NAT IP could be informed while creating the network/VPC, otherwise, it could not. In my opinion, it is a simplier design to implement and the operators would not need to create new offerings to use the feature. What do you think?
I agree with @GutoVeronezi it may be unnecessary to add a capability to enable/disable SourceNat IP selection.
As @rohityadavcloud said, currently users can do it by
- create network or VPC (but not start it)
- acquire a specific public IP
- create vm The acquired Public IP will be the Source NAT IP.
+1 on what @GutoVeronezi proposed. We just need to check if the offering provides SourceNAT as a service.
@weizhouapache that's an interesting workaround, but seems quite limited. Main use case I see for this is when an Operator needs to decommision a "public" IP range (I had to do this in the past and it was a nightmare of database hacks).
So what you are saying, @weizhouapache @GutoVeronezi , is that if source NAT is enabled on an offering, the user automatically has the possibility to choose a source NAT IP?
Exactly that @DaanHoogland If the SourceNAT service is enabled, then the end user should be able to change the IP.
@DaanHoogland, my point was that we would not need an attribute like
selecting the source NAT IPin the offering; we could just validate if the offering supports theSourceNatservice; if the offering supports theSourceNatservice, then the source NAT IP could be informed while creating the network/VPC, otherwise, it could not. In my opinion, it is a simplier design to implement and the operators would not need to create new offerings to use the feature. What do you think?
makes sense, but I would not make it obligatory, if that is what you mean, just make it an optional parameter that is accepted and validated if source NAT is enabled.
@DaanHoogland, my point was that we would not need an attribute like
selecting the source NAT IPin the offering; we could just validate if the offering supports theSourceNatservice; if the offering supports theSourceNatservice, then the source NAT IP could be informed while creating the network/VPC, otherwise, it could not. In my opinion, it is a simplier design to implement and the operators would not need to create new offerings to use the feature. What do you think?makes sense, but I would not make it obligatory, if that is what you mean, just make it an optional parameter that is accepted and validated if source NAT is enabled.
Yes, it could behave as the "add VM to network process": if no IP is provided, one is selected at random (as the current behavior); if the IP is provided, validations are executed, and if everything is fine, the IP is used.
I am taking ipv6 addresses out of scope for this. Also I am slightly refactorring the interface tto allow for a UUID, though i think a UUID for an IP address makes no sense as the IP itself is already a unique identifyer. #legacy
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
@DaanHoogland 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: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 5856
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.
@DaanHoogland 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.