Skip to content

Fix invalid consoleproxy url after upgrade - #2734

Merged
yadvr merged 1 commit into
apache:4.11from
resmo:fix/config-consoleproxy-url-domain
Jul 26, 2018
Merged

Fix invalid consoleproxy url after upgrade#2734
yadvr merged 1 commit into
apache:4.11from
resmo:fix/config-consoleproxy-url-domain

Conversation

@resmo

@resmo resmo commented Jul 6, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Description

Ensures we have a valid console proxy domain for protocol http even a * setting is used. See #2733

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

GitHub Issue/PRs

Fixes #2733

Screenshots (if appropriate):

after fix
selection_185

How Has This Been Tested?

Manually in simulator setup

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
    Testing
  • I have added tests to cover my changes.
  • All relevant new and existing integration tests have passed.
  • A full integration testsuite with all test that can run on my environment has passed.

rafaelweingartner
rafaelweingartner reviewed Jul 6, 2018
View reviewed changes
Comment thread core/src/main/java/com/cloud/info/ConsoleProxyInfo.java Outdated
// Domain in format *.example.com
if (consoleProxyUrlDomain.startsWith("*")) {
sb.append(proxyIpAddress);
for (int i = 0; i < proxyIpAddress.length(); i++)

@rafaelweingartner rafaelweingartner Jul 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can replace this block with a "replaceAll" method, right?

Comment thread core/src/main/java/com/cloud/info/ConsoleProxyInfo.java
proxyAddress = sb.toString();
proxyPort = port;
this.proxyUrlPort = proxyUrlPort;
StringBuffer sb = new StringBuffer();

@rafaelweingartner rafaelweingartner Jul 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would you mind extracting this block that creates the "proxyAddress" to a method? Then, we could have unit tests and documentation for it.

@resmo resmo changed the title Fix invalid consoleproxy url after upgrade [WIP] Fix invalid consoleproxy url after upgrade Jul 6, 2018
@resmo
resmo changed the base branch from master to 4.11 July 9, 2018 11:28
@resmo
resmo changed the base branch from 4.11 to master July 9, 2018 11:28
@resmo resmo changed the title [WIP] Fix invalid consoleproxy url after upgrade Fix invalid consoleproxy url after upgrade Jul 9, 2018
@yadvr yadvr added this to the 4.11.2.0 milestone Jul 10, 2018
@yadvr
yadvr changed the base branch from master to 4.11 July 10, 2018 06:21
@yadvr
yadvr changed the base branch from 4.11 to master July 10, 2018 06:21
@yadvr

yadvr commented Jul 10, 2018

Copy link
Copy Markdown
Member

@resmo can you rebase the PR against 4.11 and change the base branch to 4.11?

@resmo
resmo changed the base branch from master to 4.11 July 11, 2018 06:11
@resmo

resmo commented Jul 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member Author

@rhtyd done (force pushed again, to trigger build)

@yadvr

yadvr commented Jul 11, 2018

Copy link
Copy Markdown
Member

@blueorangutan package

@blueorangutan

blueorangutan commented Jul 11, 2018

Copy link
Copy Markdown

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan

blueorangutan commented Jul 11, 2018

Copy link
Copy Markdown

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2169

@yadvr

yadvr commented Jul 11, 2018

Copy link
Copy Markdown
Member

@blueorangutan test

@blueorangutan

blueorangutan commented Jul 11, 2018

Copy link
Copy Markdown

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

@blueorangutan

blueorangutan commented Jul 11, 2018

Copy link
Copy Markdown

Trillian test result (tid-2841)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30416 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2734-t2841-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup Error 0.00 test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup Failure 1228.73 test_privategw_acl.py
test_02_VPC_default_routes Failure 125.90 test_vpc_router_nics.py

@yadvr
yadvr requested review from DaanHoogland and yadvr July 13, 2018 11:14
@yadvr

yadvr commented Jul 13, 2018

Copy link
Copy Markdown
Member

Regression test LGTM, I did not test it manually though. @borisstoyanov if you've time next week, can you test and advise? Thanks.

rafaelweingartner
rafaelweingartner approved these changes Jul 13, 2018
View reviewed changes
Comment thread core/src/com/cloud/info/ConsoleProxyInfo.java
}
}

private String formatProxyAddress(String consoleProxyUrlDomain, String proxyIpAddress) {

@rafaelweingartner rafaelweingartner Jul 13, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@resmo you already did a lot! I would like to see a documentation (instead of inline comments) and unit tests for this method though.

@resmo

resmo commented Jul 18, 2018

Copy link
Copy Markdown
Member Author

will work on the unit test but +14 days and I assume forward merging will be tricky (file renames), suggesting to merge into 4.11 and add the test later on master for 4.12.

@rafaelweingartner

rafaelweingartner commented Jul 18, 2018

Copy link
Copy Markdown
Member

I think the forward merging is working (normally). Somehow git is managing the new folder structure that was applied between 4.11-4.12

borisstoyanov
borisstoyanov approved these changes Jul 19, 2018
View reviewed changes

@borisstoyanov borisstoyanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, based on manual verification and marvin test results

@yadvr
yadvr merged commit f88d606 into apache:4.11 Jul 26, 2018
@resmo
resmo deleted the fix/config-consoleproxy-url-domain branch July 28, 2018 10:43
@resmo resmo mentioned this pull request Aug 13, 2018
Merged
12 tasks
yadvr pushed a commit that referenced this pull request Aug 14, 2018
@resmo @yadvr
tests: add unit tests for console proxy url (#2800)
2930a60 
Follow up of #2734
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr approved these changes
@DaanHoogland DaanHoogland DaanHoogland approved these changes
@rafaelweingartner rafaelweingartner rafaelweingartner approved these changes
@borisstoyanov borisstoyanov borisstoyanov approved these changes

Assignees

No one assigned

Labels

component:console-proxy

Projects

None yet

Milestone

4.11.2.0

Development

Successfully merging this pull request may close these issues.

7 participants

@resmo @yadvr @blueorangutan @rafaelweingartner @DaanHoogland @borisstoyanov @GabrielBrascher