Merge branch Feature/auto-vic-ui-plugin #1979

wjun · 2018-08-27T09:56:25Z

We switch ui plugin registration from running manual commands of install.sh or upgrade.sh to automation. A set of rest apis are hosted on the VIC appliance fileserver. During the appliance initialization, index.html page will send a post request to the plugin install api, which will prepare the plugin and register the plugin to vCenter.

A thumbprint verification page is shown to enhance security, e.g. a wrong vCenter or a man-in-the-middle attack.

In case there is an old ui plugin in vCenter already, the new VIC appliance will overwrite the old ui plugin and register a new one.

UI plugin registration is also integrated into upgrade.sh, so the UI plugin can be upgraded as well when running upgrade.sh for VIC appliance upgrade.

With UI plugin auto registration, we do not need the manual ui install/upgrade.sh scripts and vic-ui-* commands anymore.

UI plugin registration is set to be installed by default during initialization and upgrade, but users can also disable the installation by uncheck the installation option.

Towards: vmware/vic-product#1432 vmware/vic-product#1952

zjs

Add ui plugin installation as an option c33d39d

This should be posted for review separately, as a PR targeting the feature branch.

Edit: As discussed out-of-band, we will violate the process this time in order to expedite merge of the feature branch into master to meet the RC1 deadline.

zjs

I did not review 5cdbba4 (Update vendored vmware/vic to v1.4.0).

installer/fileserver/main.go

installer/build/scripts/upgrade/upgrade.sh

zjs · 2018-08-28T16:38:01Z

installer/build/scripts/upgrade/upgrade.sh

@@ -658,7 +665,11 @@ function finish() {
  if [ "$rc" -eq 0 ]; then
    log ""
    log "-------------------------"
-    log "Upgrade completed successfully. Exiting."
+    if [ "$UPGRADE_UI_PLUGIN" == "y" ]; then
+      log "Upgrade completed successfully. Exiting. Please logout and login vCenter Server twice to view the upgraded ui plugin."


Please logout and login vCenter Server twice to view the upgraded ui plugin.

Where does this instruction come from? It does not seem consistent with our documentation.

This question should be answered and any resulting discussion should be resolved before merging the feature branch to master.

To clarify this question: why does the user need to log out and log in twice?

The previous instructions only instructed the user to log out and log once:

... log out and log back in again ...

zjs · 2018-08-28T16:41:15Z

installer/fileserver/routes/index.go

@@ -60,6 +60,8 @@ func (i *IndexHTMLRenderer) IndexHandler(resp http.ResponseWriter, req *http.Req
 		if err := indexFormHandler(op, req, html); err != nil {
 			op.Errorf("Install failed: %s", err.Error())
 			html.InitErrorFeedback = fmt.Sprintf("Installation failed: %s", err.Error())
+		} else if req.FormValue("needuiplugin") == "true" {
+			html.InitSuccessFeedback = "Installation successful. Refer to the Post-install and Deployment tasks below. Please logout and login vCenter Server twice to view the latest ui plugin."


Please logout and login vCenter Server twice to view the latest ui plugin.

As above: Where does this instruction come from? It does not seem consistent with our documentation.

This question should be answered and any resulting discussion should be resolved before merging the feature branch to master.

This text change should be reviewed by @lweitzman (UX) or @stuclem (IX) before merging to master. I don't believe "latest ui plugin" is the way we refer to the plugin in user-facing messages. Also, we seem to use "log out" and "log in" as verbs (vs. "logout" and "login").

To clarify this question: why does the user need to log out and log in twice?

The previous instructions only instructed the user to log out and log once:

... log out and log back in again ...

installer/fileserver/html/.gitignore

installer/fileserver/html/js/fixtures/index.html

installer/fileserver/html/js/index-handler.js

installer/build/scripts/upgrade/upgrade.sh

installer/lib/login.go

ghost

@zjs @wjun addressed the thumbprint concerns. The diff crashed my browser so I did not look at anything else. Nice job! :)

installer/lib/login.go

installer/fileserver/main.go

installer/build/scripts/upgrade/upgrade.sh

installer/scripts/ci-build.sh

installer/build/build-ova.sh

installer/build/bootable/build-main.sh

installer/lib/login.go

zjs

This looks much better! Here's an attempt to summarize the open feedback items in a single review.

I've attempted to note the items that I think still need to be resolved before merging to master.

zjs · 2018-08-30T07:15:15Z

installer/build/scripts/upgrade/upgrade.sh

+  local vc='{"target":"'"${VCENTER_TARGET}"'","user":"'"${VCENTER_USERNAME}"'","password":"'"${VCENTER_PASSWORD}"'","thumbprint":"'"${VCENTER_FINGERPRINT}"'"}'
+  local plugin='{"preset":"'"${preset}"'","force":true}'
+  local payload='{"vc":"${vc}","plugin":"${plugin}"}'
+  echo "register payload - ${payload}" | sed -e 's/'${VCENTER_PASSWORD}'/***/g' >> $upgrade_log_file 2>&1


It seems like this has the previously-mentioned issue where some choices of password can cause a variety of very strange effects here.

As this is a potential security issue, I think we should address this before merging to master.

zjs · 2018-08-30T07:15:34Z

installer/build/scripts/upgrade/upgrade.sh

 function callRegisterEndpoint {
+  local payload='{"target":"'"${VCENTER_TARGET}"'","user":"'"${VCENTER_USERNAME}"'","password":"'"${VCENTER_PASSWORD}"'","thumbprint":"'"${VCENTER_FINGERPRINT}"'","externalpsc":"'"${EXTERNAL_PSC}"'","pscdomain":"'"${PSC_DOMAIN}"'"}'
+  echo "register payload - ${payload}" | sed -e 's/'${VCENTER_PASSWORD}'/***/g' >> $upgrade_log_file 2>&1


zjs · 2018-08-30T07:21:05Z

tests/test-cases/Group2-Getting-Started/2-01-Getting-Started.robot

+    Verify Getting Started Page Title
+    #Log In And Complete OVA Installation


I don't think we should merge to master without having some very basic automated integration testing. With this line commented out, it seems like our only testing is via scenario tests.

(Previous discussion: #1979 (comment))

zjs · 2018-08-30T07:22:34Z

installer/fileserver/html/js/fixtures/index.html

@@ -0,0 +1,96 @@
+<div id="main-container">


This duplication should be addressed for 1.4.3, but I think this can happen after we merge to master.

(Previous discussion: bbfe684#r213637735)

#1996 has been filed to track, but I would suggest to review and prioritize to see if this can be addressed in release 1.4.3.

zjs · 2018-08-30T07:24:31Z

installer/build/bootable/build-main.sh

@@ -86,6 +86,11 @@ function build_app {
                SOURCE=$(jq '.['$LINE_NUM'] | .source' "${MANIFEST}" | tr -d '"')
                DESTINATION=$(echo "${ROOT}/$(cat "${MANIFEST}" | jq '.['$LINE_NUM'] | .destination')" | tr -d '"' )
                mkdir -p "$(dirname "$DESTINATION")" && cp -R $SOURCE "$DESTINATION"
+                if [[ "$DESTINATION" == *"fileserver/html"* ]]; then


I think a better short-term hack would be to update the manifest to explicitly whitelist files/directories under fileserver/html that we wish to include, instead of deleting files here.

(Previous discussion: bbfe684#r213761707)

There are quite a few files and directories under html, compared with the ones we want to delete. The whitelist will always ask UI developers to maintain the changes to whitelist which is a burden to them, while deletion with * will not introduce any extra work.

installer/build/scripts/provisioners/provision_fileserver.sh

installer/scripts/ci-build.sh

zjs · 2018-08-30T07:32:47Z

installer/fileserver/routes/index.go

@@ -60,6 +60,8 @@ func (i *IndexHTMLRenderer) IndexHandler(resp http.ResponseWriter, req *http.Req
 		if err := indexFormHandler(op, req, html); err != nil {
 			op.Errorf("Install failed: %s", err.Error())
 			html.InitErrorFeedback = fmt.Sprintf("Installation failed: %s", err.Error())
+		} else if req.FormValue("needuiplugin") == "true" {
+			html.InitSuccessFeedback = "Installation successful. Refer to the Post-install and Deployment tasks below. All vSphere Client users must log out and log back in again twice to see the vSphere Integrated Containers plug-in."


We should file an issue to investigate further; logging out twice should not be required.

(Previous discussion: #1979 (comment))

zjs · 2018-08-30T07:35:15Z

Rebasing again on master should fix some of the CI failures.

Updates the vendored vmware/vic repo to version v1.4.0. Also adjusts dep configuration to prune unused packages. Progress to #1433.

zjs · 2018-08-30T19:17:25Z

installer/build/scripts/upgrade/upgrade.sh

+  local vc='{"target":"'"${VCENTER_TARGET}"'","user":"'"${VCENTER_USERNAME}"'","password":"'"${VCENTER_PASSWORD}"'","thumbprint":"'"${VCENTER_FINGERPRINT}"'"}'
+  local plugin='{"preset":"'"${preset}"'","force":true}'
+  local payload='{"vc":"${vc}","plugin":"${plugin}"}'
+  echo "register payload - ${payload}" | sed -e 's/'"${VCENTER_PASSWORD}"'/***/g' >> $upgrade_log_file 2>&1


sed -e 's/'"${VCENTER_PASSWORD}"'/***/g'

This is better, but I still think there are some issues with it. As a silly example, a password of register.* will cause the whole line to be replace with ***.

One solution might be to use something like sed -e 's/[]\/$*.^[]/\\&/g' to escape any special characters in ${VCENTER_PASSWORD} before passing it to sed.

This would give us something like:

sed -e 's/"password":"'"$(echo "${VCENTER_PASSWORD}" | sed -e 's/[]\/$*.^[]/\\&/g')"'/***/g'

Another solution might be to use a different command to perform the substitution; sed may be overkill for this.

A third solution would be to refactor the code to avoid the need for the replacement entirely; we put the password into the string right before working hard to remove it.

According to vCenter password policy, register.* is an invalid password.

Removed sed or any replace approach, and use string without password variable instead for log only now.

Introduces caching of the vic-ui plugin artifacts as a component of the ova. Updates the fileserver provisioner to package vic-ui with the correct version numbers. Creates the Fileserver Thumbprint API to return the thumbprint of a given target ip address or fqdn. Also refactors the fileserver main process into main.go, and creates the routes/ and tasks/ sub-packages. Moves vic-ui plugin dependencies to fileserver. This includes: - vic/lib/install/ova - vic/lib/install/plugin - vic/tagvm/ Additionally, refactors ovatools/vic-ui/ui/ui.go to plugin.go that doesn't include any CLI flags. Automates the plugin install process and improves logging in the fileserver: - Add trace.Op logging to fileserver tasks - Require vcenter thumprint on psc registration - Install VIC UI plugins in the initialization process - Attaches a managed tag to the Appliance VM - Performs a plugin install during the appliance upgrade process using the /plugin API. Fixes #1433, #637, #1720, #1702 and #1789.

Adds a new modal for thumbprint validation to the Getting Started Page in the Fileserver. Fixes vmware/vic-ui#465

Refactor register tests to use thumbprint Refacotr UI plugin registration test cases

When multiple vic appliances are installed, we need to make ui plugin installation optional on individual appliance,so users can decide which ui plugin should be installed or upgraded for later use.

Comments have been addressed

We switch ui plugin registration from running manual commands of install.sh or upgrade.sh to automation. A set of rest apis are hosted on the VIC appliance fileserver. During the appliance initialization, index.html page will send a post request to the plugin install api, which will prepare the plugin and register the plugin to vCenter. A thumbprint verification page is shown to enhance security, e.g. a wrong vCenter or a man-in-the-middle attack. In case there is an old ui plugin in vCenter already, the new VIC appliance will overwrite the old ui plugin and register a new one. UI plugin registration is also integrated into upgrade.sh, so the UI plugin can be upgraded as well when running upgrade.sh for VIC appliance upgrade. With UI plugin auto registration, we do not need the manual ui install/upgrade.sh scripts and vic-ui-* commands anymore. UI plugin registration is set to be installed by default during initialization and upgrade, but users can also disable the installation by uncheck the installation option. (cherry picked from commit 0220fd5)

vmwclabot added the cla-not-required label Aug 27, 2018

wjun requested a review from zjs August 27, 2018 11:13

wjun mentioned this pull request Aug 27, 2018

Merge branch Feature/auto-vic-ui-plugin #1947

Closed

renmaosheng approved these changes Aug 27, 2018

View reviewed changes

zjs requested changes Aug 27, 2018

View reviewed changes

zjs requested changes Aug 28, 2018

View reviewed changes

zjs assigned wjun Aug 28, 2018

wjun force-pushed the feature/auto-vic-ui-plugin branch from ce57678 to 6ec823c Compare August 29, 2018 13:15

ghost suggested changes Aug 29, 2018

View reviewed changes

installer/lib/login.go Show resolved Hide resolved

wjun force-pushed the feature/auto-vic-ui-plugin branch from 6ec823c to 52430cd Compare August 29, 2018 15:20

zjs reviewed Aug 29, 2018

View reviewed changes

installer/fileserver/main.go Outdated Show resolved Hide resolved

installer/build/scripts/upgrade/upgrade.sh Show resolved Hide resolved

zjs reviewed Aug 29, 2018

View reviewed changes

installer/build/scripts/upgrade/upgrade.sh Outdated Show resolved Hide resolved

installer/build/scripts/upgrade/upgrade.sh Show resolved Hide resolved

zjs reviewed Aug 29, 2018

View reviewed changes

installer/scripts/ci-build.sh Outdated Show resolved Hide resolved

installer/build/build-ova.sh Show resolved Hide resolved

zjs reviewed Aug 29, 2018

View reviewed changes

installer/build/bootable/build-main.sh Show resolved Hide resolved

installer/lib/login.go Show resolved Hide resolved

wjun force-pushed the feature/auto-vic-ui-plugin branch from 52430cd to 47b7e46 Compare August 30, 2018 06:49

zjs previously requested changes Aug 30, 2018

View reviewed changes

wjun force-pushed the feature/auto-vic-ui-plugin branch 2 times, most recently from 35238d0 to bd45caf Compare August 30, 2018 09:03

Update vendored vmware/vic to v1.4.0 (#1757)

cf21f90

Updates the vendored vmware/vic repo to version v1.4.0. Also adjusts dep configuration to prune unused packages. Progress to #1433.

wjun force-pushed the feature/auto-vic-ui-plugin branch 2 times, most recently from 67cc786 to bbbe1a8 Compare August 30, 2018 14:29

zjs reviewed Aug 30, 2018

View reviewed changes

wjun force-pushed the feature/auto-vic-ui-plugin branch from bbbe1a8 to 8e7269a Compare August 31, 2018 02:25

Jason Morris and others added 4 commits August 31, 2018 13:24

Implement fileserver thumbprint verification (#1790)

794661c

Adds a new modal for thumbprint validation to the Getting Started Page in the Fileserver. Fixes vmware/vic-ui#465

vic-ui robot tests (#1911)

b8aef6a

Refactor register tests to use thumbprint Refacotr UI plugin registration test cases

Make ui plugin installation optional

ab536c8

When multiple vic appliances are installed, we need to make ui plugin installation optional on individual appliance,so users can decide which ui plugin should be installed or upgraded for later use.

wjun force-pushed the feature/auto-vic-ui-plugin branch from 8e7269a to ab536c8 Compare August 31, 2018 05:26

wjun merged commit 0220fd5 into master Aug 31, 2018

wjun mentioned this pull request Aug 31, 2018

Cherry-pick: Merge branch Feature/auto-vic-ui-plugin (#1979) #1998

Merged

6 tasks

wjun mentioned this pull request Sep 7, 2018

Automated VIC UI Plugin - Nightly/Integration Tests #1770

Closed

2 tasks

zjs mentioned this pull request Sep 11, 2018

Upgrade from VIC 1.3.1 to 1.4.3 failed because h5 and flex plugin cannot be upgraded #2050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge branch Feature/auto-vic-ui-plugin #1979

Merge branch Feature/auto-vic-ui-plugin #1979

wjun commented Aug 27, 2018 •

edited

Loading

zjs left a comment •

edited

Loading

zjs left a comment

zjs Aug 28, 2018

zjs Aug 29, 2018

zjs Aug 28, 2018 •

edited

Loading

zjs Aug 29, 2018

ghost left a comment

zjs left a comment

zjs Aug 30, 2018

wjun Aug 30, 2018

zjs Aug 30, 2018

wjun Aug 30, 2018

zjs Aug 30, 2018

zjs Aug 30, 2018

wjun Aug 31, 2018

zjs Aug 30, 2018

wjun Aug 31, 2018

zjs Aug 30, 2018

zjs commented Aug 30, 2018

zjs Aug 30, 2018

wjun Aug 31, 2018

wjun Aug 31, 2018

		Verify Getting Started Page Title
		#Log In And Complete OVA Installation

Merge branch Feature/auto-vic-ui-plugin #1979

Merge branch Feature/auto-vic-ui-plugin #1979

Conversation

wjun commented Aug 27, 2018 • edited Loading

zjs left a comment • edited Loading

Choose a reason for hiding this comment

zjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjs Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

zjs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zjs commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wjun commented Aug 27, 2018 •

edited

Loading

zjs left a comment •

edited

Loading

zjs Aug 28, 2018 •

edited

Loading