Allow privateips on console proxy by GaOrtiga · Pull Request #6808 · apache/cloudstack

GaOrtiga · 2022-10-05T12:01:36Z

Description

Currently the configuration consoleproxy.url.domain only accepts domain names as a valid value; however, some users have reported the desire to use private IPs as a value for this configuration. This is useful for CI implementation and local labs.

The main goal of this PR is to allow operators to use IPs as valid configurations as well.

The validations for the configuration values have also been refactored into new methods to facilitate unity tests and clean up the code.

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

Major
Minor

codecov · 2022-10-05T12:45:59Z

Codecov Report

Merging #6808 (ddfdae4) into main (95074f6) will increase coverage by 0.04%.
The diff coverage is 79.68%.

❗ Current head ddfdae4 differs from pull request most recent head 49aba8e. Consider uploading reports for the commit 49aba8e to get more accurate results

@@             Coverage Diff              @@
##               main    #6808      +/-   ##
============================================
+ Coverage     11.27%   11.31%   +0.04%     
- Complexity     7287     7336      +49     
============================================
  Files          2492     2494       +2     
  Lines        246805   246880      +75     
  Branches      38562    38567       +5     
============================================
+ Hits          27828    27940     +112     
+ Misses       215393   215344      -49     
- Partials       3584     3596      +12

Impacted Files	Coverage Δ
.../cloud/configuration/ConfigurationManagerImpl.java	`12.49% <79.03%> (+1.24%)`	⬆️
.../src/main/java/com/cloud/configuration/Config.java	`89.73% <100.00%> (+0.52%)`	⬆️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	`6.75% <0.00%> (-0.01%)`	⬇️
...in/java/com/cloud/server/ManagementServerImpl.java	`5.19% <0.00%> (-0.01%)`	⬇️
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java	`0.00% <0.00%> (ø)`
...va/com/cloud/deploy/DeploymentPlanningManager.java	`100.00% <0.00%> (ø)`
...k/engine/cloud/entity/api/VMEntityManagerImpl.java	`0.00% <0.00%> (ø)`
...stack/affinity/NonStrictHostAffinityProcessor.java	`71.79% <0.00%> (ø)`
...k/affinity/NonStrictHostAntiAffinityProcessor.java	`100.00% <0.00%> (ø)`
... and 8 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

DaanHoogland

small issues with the code so far.

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

DaanHoogland · 2022-10-11T08:59:48Z

@GaOrtiga dio you expect to have this finished/ready for review in the coming weeks?

GaOrtiga · 2022-10-13T17:06:58Z

@DaanHoogland Yes, I will probably have it ready by the next week.

sonarqubecloud · 2022-10-17T20:06:52Z

SonarCloud Quality Gate failed.

1 Bug
0 Vulnerabilities
0 Security Hotspots
270 Code Smells

34.6% Coverage
0.0% Duplication

DaanHoogland · 2022-12-13T09:11:10Z

SonarCloud Quality Gate failed.

1 Bug 0 Vulnerabilities 0 Security Hotspots 270 Code Smells

34.6% Coverage 0.0% Duplication

@GaOrtiga 270 code smells seems a bit daunting. Are you still working on this?

DaanHoogland · 2022-12-19T11:43:28Z

@GaOrtiga as this is still in draft, should it be pushed to a later milestone? i.e. 4.19?

GaOrtiga · 2022-12-19T14:59:15Z

@GaOrtiga as this is still in draft, should it be pushed to a later milestone? i.e. 4.19?

Sorry about the delay to finish this, while creating the tests I found the error that was fixed here: #6910
This error needed to be fixed in order for the tests to run, so now that the fix was merged I came back for this PR.
I believe I have finished the changes, it is working as expected so far.
Will just run some more tests and hopefully take out of draft this week.

sonarqubecloud · 2022-12-19T16:55:46Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
268 Code Smells

60.8% Coverage
0.0% Duplication

DaanHoogland · 2022-12-20T08:22:03Z

@blueorangutan package

blueorangutan · 2022-12-20T08:23:03Z

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

blueorangutan · 2022-12-20T09:29:00Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 5033

DaanHoogland · 2022-12-20T12:13:13Z

@blueorangutan test

blueorangutan · 2022-12-20T12:14:03Z

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

DaanHoogland

clgtm, but a license header is needed/missing

server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java

blueorangutan · 2022-12-20T23:50:44Z

Trillian test result (tid-5589)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 40424 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6808-t5589-kvm-centos7.zip
Smoke tests completed. 105 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

DaanHoogland

clgtm

DaanHoogland · 2022-12-21T08:36:51Z

@blueorangutan test keepEnv

blueorangutan · 2022-12-21T08:38:03Z

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

blueorangutan · 2022-12-21T20:35:27Z

Trillian test result (tid-5596)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 41787 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr6808-t5596-kvm-centos7.zip
Smoke tests completed. 105 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

JoaoJandre

LGTM

RodrigoDLopez

LGTM.
Tested locally and everything work like a charm

server/src/main/java/com/cloud/configuration/Config.java

DaanHoogland · 2022-12-22T12:58:51Z

@GaOrtiga,
Do you plan to document this? (in https://github.com/apache/cloudstack-documentation)?
And do you plan to apply @RodrigoDLopez 'extra space suggestion?

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Co-authored-by: Rodrigo D. Lopez <19981369+RodrigoDLopez@users.noreply.github.com> Co-authored-by: Stephan Krug <stekrug@icloud.com>

GaOrtiga · 2022-12-22T15:21:21Z

@DaanHoogland

Do you plan to document this?
Initially I did not plan on documenting since it is not a major change, but I can do it.

And do you plan to apply @RodrigoDLopez 'extra space suggestion?
I applied the suggestion, is there a concern regarding it?

DaanHoogland · 2022-12-22T15:43:05Z

@DaanHoogland

Do you plan to document this? Initially I did not plan on documenting since it is not a major change, but I can do it.

And do you plan to apply @RodrigoDLopez 'extra space suggestion? I applied the suggestion, is there a concern regarding it?

No concerns, I just wodered what to test/look for. I'm fine with merging as is but I think it will not be for those that do not know beforehand what to expect, so if you an find any documentation on the console proxy domain configuration it would be nice if you can add it.

This reverts commit 9164534.

GaOrtiga · 2022-12-22T19:08:58Z

@DaanHoogland
I applied one of the suggestions but forgot to update the tests with the current text. Now this test is broken, so, should we revert it or create a fix?

weizhouapache · 2022-12-22T19:11:27Z

@DaanHoogland I applied one of the suggestions but forgot to update the tests with the current text. Now this test is broken, so, should we revert it or create a fix?

@GaOrtiga
I just faced the same issue.
please create a pull request to fix the test asap

GaOrtiga · 2022-12-22T19:27:33Z

I created the PR #7018
Sorry for the mistake.

weizhouapache · 2022-12-22T20:24:25Z

no worries @GaOrtiga
pr has been mergd.

DaanHoogland reviewed Oct 11, 2022

View reviewed changes

DaanHoogland added this to the 4.18.0.0 milestone Oct 14, 2022

boring-cyborg bot added the component:api label Oct 17, 2022

GaOrtiga force-pushed the allow_privateips_on_Console_Proxy branch from c39f4e0 to d44ca46 Compare October 17, 2022 19:11

create parameter to determine whether roles are public or private

f218a9f

GaOrtiga force-pushed the allow_privateips_on_Console_Proxy branch from d44ca46 to f218a9f Compare December 19, 2022 15:02

error fix

2e34a97

GaOrtiga marked this pull request as ready for review December 19, 2022 15:42

DaanHoogland requested changes Dec 20, 2022

View reviewed changes

server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java Show resolved Hide resolved

add missing license

ddfdae4

DaanHoogland approved these changes Dec 21, 2022

View reviewed changes

JoaoJandre approved these changes Dec 22, 2022

View reviewed changes

RodrigoDLopez approved these changes Dec 22, 2022

View reviewed changes

server/src/main/java/com/cloud/configuration/Config.java Outdated Show resolved Hide resolved

stephankruggg approved these changes Dec 22, 2022

View reviewed changes

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Outdated Show resolved Hide resolved

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java Outdated Show resolved Hide resolved

Apply suggestions from code review

49aba8e

Co-authored-by: Rodrigo D. Lopez <19981369+RodrigoDLopez@users.noreply.github.com> Co-authored-by: Stephan Krug <stekrug@icloud.com>

DaanHoogland merged commit 9164534 into apache:main Dec 22, 2022

GaOrtiga added a commit to scclouds/cloudstack that referenced this pull request Dec 22, 2022

Revert "Allow privateips on console proxy (apache#6808)"

d986c66

This reverts commit 9164534.

Conversation

GaOrtiga commented Oct 5, 2022

Description

Types of changes

Feature/Enhancement Scale

Uh oh!

codecov bot commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Oct 11, 2022

Uh oh!

GaOrtiga commented Oct 13, 2022

Uh oh!

sonarqubecloud bot commented Oct 17, 2022

Uh oh!

DaanHoogland commented Dec 13, 2022

Uh oh!

DaanHoogland commented Dec 19, 2022

Uh oh!

GaOrtiga commented Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Dec 19, 2022

Uh oh!

DaanHoogland commented Dec 20, 2022

Uh oh!

blueorangutan commented Dec 20, 2022

Uh oh!

blueorangutan commented Dec 20, 2022

Uh oh!

DaanHoogland commented Dec 20, 2022

Uh oh!

blueorangutan commented Dec 20, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

blueorangutan commented Dec 20, 2022

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Dec 21, 2022

Uh oh!

blueorangutan commented Dec 21, 2022

Uh oh!

blueorangutan commented Dec 21, 2022

Uh oh!

JoaoJandre left a comment

Choose a reason for hiding this comment

Uh oh!

RodrigoDLopez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaanHoogland commented Dec 22, 2022

Uh oh!

Uh oh!

Uh oh!

GaOrtiga commented Dec 22, 2022

Uh oh!

DaanHoogland commented Dec 22, 2022

Uh oh!

GaOrtiga commented Dec 22, 2022

Uh oh!

weizhouapache commented Dec 22, 2022

Uh oh!

GaOrtiga commented Dec 22, 2022

Uh oh!

weizhouapache commented Dec 22, 2022

Uh oh!

codecov bot commented Oct 5, 2022 •

edited

Loading

GaOrtiga commented Dec 19, 2022 •

edited

Loading