Skip to content
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.

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

Allow privateips on console proxy #6808

Merged
merged 4 commits into from Dec 22, 2022

Conversation

GaOrtiga
Copy link
Contributor

@GaOrtiga GaOrtiga commented Oct 5, 2022

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
Copy link

codecov bot commented Oct 5, 2022

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

small issues with the code so far.

@DaanHoogland
Copy link
Contributor

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

@GaOrtiga
Copy link
Contributor Author

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

@DaanHoogland DaanHoogland added this to the 4.18.0.0 milestone Oct 14, 2022
@GaOrtiga GaOrtiga force-pushed the allow_privateips_on_Console_Proxy branch from c39f4e0 to d44ca46 Compare October 17, 2022 19:11
@sonarcloud
Copy link

sonarcloud bot commented Oct 17, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug E 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell D 270 Code Smells

34.6% 34.6% Coverage
0.0% 0.0% Duplication

@DaanHoogland
Copy link
Contributor

SonarCloud Quality Gate failed. Quality Gate failed

Bug E 1 Bug Vulnerability A 0 Vulnerabilities Security Hotspot A 0 Security Hotspots Code Smell D 270 Code Smells

34.6% 34.6% Coverage 0.0% 0.0% Duplication

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

@DaanHoogland
Copy link
Contributor

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

@GaOrtiga
Copy link
Contributor Author

GaOrtiga commented Dec 19, 2022

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

@GaOrtiga GaOrtiga force-pushed the allow_privateips_on_Console_Proxy branch from d44ca46 to f218a9f Compare December 19, 2022 15:02
@GaOrtiga GaOrtiga marked this pull request as ready for review December 19, 2022 15:42
@sonarcloud
Copy link

sonarcloud bot commented Dec 19, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell E 268 Code Smells

60.8% 60.8% Coverage
0.0% 0.0% Duplication

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@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
Copy link

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

@DaanHoogland
Copy link
Contributor

@blueorangutan test

@blueorangutan
Copy link

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm, but a license header is needed/missing

@blueorangutan
Copy link

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

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm

@DaanHoogland
Copy link
Contributor

@blueorangutan test keepEnv

@blueorangutan
Copy link

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

@blueorangutan
Copy link

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

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@RodrigoDLopez RodrigoDLopez left a comment

Choose a reason for hiding this comment

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

LGTM.
Tested locally and everything work like a charm

server/src/main/java/com/cloud/configuration/Config.java Outdated Show resolved Hide resolved
@DaanHoogland
Copy link
Contributor

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

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

@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
Copy link
Contributor

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

@DaanHoogland 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
@GaOrtiga
Copy link
Contributor Author

@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
Copy link
Member

@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
Copy link
Contributor Author

I created the PR #7018
Sorry for the mistake.

@weizhouapache
Copy link
Member

no worries @GaOrtiga
pr has been mergd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants