Skip to content

Conversation

@utchoang
Copy link

@utchoang utchoang commented Apr 1, 2021

Description

Related apache/cloudstack-primate#898
This PR for Add multiple management server support

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 or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

image

How Has This Been Tested?

@utchoang utchoang changed the title add multiple management server support UI: Add multiple management server support Apr 1, 2021
@rohityadavcloud rohityadavcloud added this to the 4.16.0.0 milestone Apr 4, 2021
@davidjumani
Copy link
Contributor

davidjumani commented Jun 1, 2021

Think it would be good to show the management server to which the user is connected listed in the header next to his name. Thoughts @utchoang @rhtyd

Screenshot from 2021-06-01 14-04-57

@rohityadavcloud
Copy link
Member

@blueorangutan ui

@blueorangutan
Copy link

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

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/4885 (SL-JID-229)

@davidjumani
Copy link
Contributor

@utchoang Did you face any CORS issue ?

@utchoang
Copy link
Author

@davidjumani In the previous discussion at apache/cloudstack-primate#898 there was a discussion about the CORS issue, and I think it will be solved by Nginx proxy. I think so.

@utchoang
Copy link
Author

@blueorangutan ui

@blueorangutan
Copy link

@utchoang a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/4885 (SL-JID-273)

@wido
Copy link
Contributor

wido commented Jun 22, 2021

@davidjumani In the previous discussion at apache/cloudstack-primate#898 there was a discussion about the CORS issue, and I think it will be solved by Nginx proxy. I think so.

Indeed. As long as a proxy in between is used to use URLs and not different domains for management servers CORS is not an issue, for example:

  • /proxy/management-server-1
  • /proxy/management-server-2
  • /proxy/management-server-3

These would be the relative URLs to reach the different management servers.

This means that you do not set apiHost, but only set apiBase for each Management Server.

Otherwise you need to deal with CORS indeed and thus add headers which can be done by Nginx/Apache.

Copy link
Contributor

@wido wido left a comment

Choose a reason for hiding this comment

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

LGTM

@weizhouapache
Copy link
Member

I have tested apache/cloudstack-primate#898, which worked.
I used nginx or pfsense/haproxy to solve CORS issue.

@utchoang are these two prs same ?

@utchoang
Copy link
Author

@weizhouapache Yes. Two PRs same.

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

@utchoang great. lgtm

@davidjumani
Copy link
Contributor

@davidjumani In the previous discussion at apache/cloudstack-primate#898 there was a discussion about the CORS issue, and I think it will be solved by Nginx proxy. I think so.

Indeed. As long as a proxy in between is used to use URLs and not different domains for management servers CORS is not an issue, for example:

* /proxy/management-server-1

* /proxy/management-server-2

* /proxy/management-server-3

These would be the relative URLs to reach the different management servers.

This means that you do not set apiHost, but only set apiBase for each Management Server.

Otherwise you need to deal with CORS indeed and thus add headers which can be done by Nginx/Apache.

Thanks, was playing around in dev mode but got it working

Copy link
Member

@GabrielBrascher GabrielBrascher left a comment

Choose a reason for hiding this comment

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

LGTM

@nvazquez
Copy link
Contributor

LGTM
@utchoang there is only one open comment, can you please check?

@DaanHoogland
Copy link
Contributor

enough lgtm, but has this been tested? (@davidjumani ?)

@rohityadavcloud
Copy link
Member

@utchoang can you send some doc PR on how users will use it; for example how to configure the reverse proxy/server etc and will the dropdown be hidden for most users, say who don't want to specify or use this feature?

@davidjumani
Copy link
Contributor

@DaanHoogland This has been tested and working. For anyone who needs to test it locally,

Update ui/public/config.json

  "servers": [
    {
      "name": "QA-Server",
      "apiHost": "",
      "apiBase": "/qa/api"
    },
    {
      "name": "Localhost",
      "apiHost": "",
      "apiBase": "/client/api"
    }
  ],

Update ui/vue.config.js

proxy: {
      '/qa': {
        target: 'http://qa.cloudstack.cloud:8080/',
        secure: false,
        ws: false,
        pathRewrite: {
          '/qa/api': '/client/api',
          '/qa/console': '/console'
        },
        cookiePathRewrite: {
          '/qa/api': '/client/api',
          '*': '/'
        },
        changeOrigin: true,
        proxyTimeout: 10 * 60 * 1000 // 10 minutes
      },
      '/client': {
        target: process.env.CS_URL || 'http://localhost:8080',
        secure: false,
        ws: false,
        changeOrigin: true,
        proxyTimeout: 10 * 60 * 1000 // 10 minutes
      }
    }

@rohityadavcloud
Copy link
Member

@blueorangutan ui

@blueorangutan
Copy link

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

@@ -1,5 +1,12 @@
{
"apiBase": "/client/api",
"servers": [
Copy link
Member

Choose a reason for hiding this comment

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

I think this feature shouldn't be enabled by default, wouldn't it confuse users/admins? @utchoang @davidjumani @wido @nvazquez @sureshanaparti @weizhouapache @Pearl1594 @shwstppr ?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the list only shows one server at the time, what would it confuse them? There is only one option to select

Copy link
Author

Choose a reason for hiding this comment

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

@rhtyd Yes. I will set it to default hidden mode. If the user/admin wants to enable it, the configuration can be changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good with @utchoang's change, disabled by default. +1 for getting this in asap

Copy link
Contributor

Choose a reason for hiding this comment

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

+1
thanks @utchoang

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/4885 (SL-JID-495)

@davidjumani
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

@davidjumani a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: http://qa.cloudstack.cloud:8080/client/pr/4885 (SL-JID-500)

Copy link
Contributor

@nvazquez nvazquez left a comment

Choose a reason for hiding this comment

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

LGTM

@nvazquez
Copy link
Contributor

Merging based on approvals

@nvazquez nvazquez merged commit 1182051 into apache:4.15 Aug 11, 2021
@utchoang utchoang deleted the feature/multiple-server branch August 16, 2021 00:56
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.

10 participants