Skip to content

Windows installer now ensures that legacy osquery installations gets removed during clean install - #9048

Merged
zwass merged 4 commits into
mainfrom
8891-installer-custom-actions
Dec 20, 2022
Merged

Windows installer now ensures that legacy osquery installations gets removed during clean install#9048
zwass merged 4 commits into
mainfrom
8891-installer-custom-actions

Conversation

@marcosd4h

Copy link
Copy Markdown
Contributor

This relates to #8891

This PR introduces Wix custom actions usage

Checklist for submitter

If some of the following don't apply, delete the relevant line.

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
  • Manual QA for all new/changed functionality

@marcosd4h
marcosd4h requested a review from zwass December 16, 2022 16:06
@marcosd4h
marcosd4h requested a review from a team as a code owner December 16, 2022 16:06
@codecov-commenter

codecov-commenter commented Dec 16, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.25%. Comparing base (f5b60b9) to head (d90e07a).
⚠️ Report is 16884 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #9048    +/-   ##
========================================
  Coverage   60.24%   60.25%            
========================================
  Files         472      473     +1     
  Lines       45759    46063   +304     
========================================
+ Hits        27566    27753   +187     
- Misses      15623    15706    +83     
- Partials     2570     2604    +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment thread orbit/pkg/packaging/windows.go Outdated
}

if err := ioutil.WriteFile(path, contents.Bytes(), constant.DefaultFileMode); err != nil {
return fmt.Errorf("epowershell installer utils file write: %w", err)

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.

nit: spelling

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, just fixed this

@@ -0,0 +1 @@
* Windows installer now ensures that installed osquery versio get removed before installing Orbit

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.

nit: spelling

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@marcosd4h
marcosd4h temporarily deployed to Docker Hub December 16, 2022 17:54 — with GitHub Actions Inactive

@zwass zwass left a comment

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.

I'll give the code a more thorough review (only skimmed it so far). Can you please document in the PR description what manual QA steps you went through? For these kinds of complex agent PRs it's really helpful for me to get confidence that we've gone through a thorough set of tests.

@marcosd4h

marcosd4h commented Dec 16, 2022

Copy link
Copy Markdown
Contributor Author

I'll give the code a more thorough review (only skimmed it so far). Can you please document in the PR description what manual QA steps you went through? For these kinds of complex agent PRs it's really helpful for me to get confidence that we've gone through a thorough set of tests.

Hey @zwass, sure, that's a good point!

I've manually tested the following scenarios. The scenarios3 and scenario4 are the ones that caused issues on the field

Scenario 1 - Clean Orbit installation

  1. Run msiexec /lv loginstall.txt /i <package.msi> to install the package
  2. Manual check that everything is working

Scenario 2 - Clean Orbit uninstallation

  1. Run msiexec /lv loguninstall.txt /x <package.msi> to uninstall the package
  2. Manual check that everything got uninstalled and removed

Scenario 3 - Problematic Scenario - Multiple logged-in users with legacy osquery installation

  1. Create two local users accounts on a Windows machine
  2. Install osquery using one of the users (no previous osquery/orbit installations)
  3. Then login as a different user and try installing Orbit. Without the fix, orbit installation will not succeed. The fix will ensure cleaning up legacy osquery installations and installing Orbit.
  4. Manual check that everything is working

Scenario 4 - Problematic Scenario - Multiple users with legacy osquery installation - Only one user is logged in

  1. Create two local users accounts on a Windows machine
  2. Install osquery using one of the users (no previous osquery/orbit installations)
  3. Reboot
  4. Once the machine is back, login as a different user and try installing Orbit. Without the fix, orbit installation will not succeed. The fix will ensure cleaning up legacy osquery installations and installing Orbit.
  5. Manual check that everything is working

…oving installer_utils.ps1 deployment by removing duplicate
@marcosd4h
marcosd4h requested a review from a team December 16, 2022 18:55
@marcosd4h
marcosd4h temporarily deployed to Docker Hub December 16, 2022 18:55 — with GitHub Actions Inactive

@zwass zwass left a comment

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.

@marcosd4h besides the logging, this lgtm. If there's a reason for not doing the logging I'm good to merge as-is.

Let's try to merge this today (mergefreeze can be skipped due to bugfix).

}
catch
{

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.

Can/should we be logging in this and other catch blocks?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zwass Yes, this is a good point. Console logs are captured and logged by msiexec, so adding log lines here is a good idea. I've just pushed the change to add support for this.

@marcosd4h
marcosd4h temporarily deployed to Docker Hub December 19, 2022 20:55 — with GitHub Actions Inactive
@marcosd4h

Copy link
Copy Markdown
Contributor Author

@xpkoala This PR adds support for custom action logic on the Windows MSI installer. There is one custom action added here to uninstall old vanilla osquery installs - see scenarios 3 and 4 above. @zwass asked to merge this so the fix can get into the upcoming release and unblock a customer that is having this problem, see #8891. Can you prioritize testing this when having some time available? Let me know if you need helping on setting up your testing environment (win10 box - 2 local accounts - osquery installed by one of the users - orbit installed by the user that did not install osquery)

@marcosd4h
marcosd4h requested a review from zwass December 19, 2022 21:10
@zwass
zwass merged commit 605ae86 into main Dec 20, 2022
@zwass
zwass deleted the 8891-installer-custom-actions branch December 20, 2022 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants