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

FMA: Installing macOS app bundles when they're already installed breaks the app #24148

Open
7 of 10 tasks
noahtalerman opened this issue Nov 25, 2024 · 23 comments
Open
7 of 10 tasks
Assignees
Labels
~backend Backend-related issue. bug Something isn't working as documented customer-numa #g-software Software product group :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.
Milestone

Comments

@noahtalerman
Copy link
Member

noahtalerman commented Nov 25, 2024

Fleet version: Observed in Fleet's dogfood environment

Web browser and operating system: macOS 14.6.1


πŸ’₯ Β Actual behavior

Check out the Loom video here.

Restarting my Mac resolved the issue.

πŸ§‘β€πŸ’» Β Steps to reproduce

See Loom video here.

πŸ•―οΈ More info (optional)

@kennyb-222 observed that sometimes the install fails when VS Code is already installed. Check out the recording here.

That recording^ includes suggested tweaks to the install script for VS Code.

@kennyb-222 on why this is happening:

  • "macOS doesn’t handle copying app bundles over existing ones very well since Big Sur (I believe) and later. It’s generally best practice to move the existing app out of the way before deploying a new one. In the video, I also highlighted how modifying the generated install script resolves this issue."
  • "I used VS Code as an example, but seems like an inherent issue with how Fleet handles all app installs and likely affects all apps that are copied to the /Applications dir using cp."

To fix

If the app has already been installed and launched/configured by the user, the following apps will break upon re-install so will need the install script tweaked (or a fix applied):

  1. 1Password
  2. Brave
  3. Docker
  4. Figma
  5. Chrome
  6. VSCode
  7. Firefox
  8. Notion
  9. Slack
  10. WhatsApp

QA Plan

  • Start with previous version, ensure all FMAs listed above have been added to at least one team.
  • On another team, add all FMAs listed above but edit install scripts (make note of changes made)
  • Upgrade to fix version, validate that the migration updates existing install scripts for the FMAs that did not have edited install scripts.
  • Validate that the migration DOES NOT update any existing FMAs that had their install scripts edited prior to upgrade
  • Add each FMA listed above and validate that they have the updated install scripts
  • For each FMA listed above, a manual install is successful
  • For each FMA listed above, an automatic install is successful
  • For each FMA listed above, opening app, closing app, then reinstalling app is successful
  • For each FMA listed above, opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • When reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.
@noahtalerman noahtalerman added bug Something isn't working as documented #g-mdm MDM product group :incoming New issue in triage process. customer-numa labels Nov 25, 2024
@noahtalerman
Copy link
Member Author

nherent issue with how Fleet handles all app installs and likely affects all apps that are copied to the /Applications dir using cp

@PezHub just checking, as part of testing for Fleet-maintained apps, do we test installing the apps on workstations that already have the apps installed?

cc @georgekarrv

@PezHub
Copy link
Contributor

PezHub commented Nov 25, 2024

I did (Column H) but tbh I did not test installing over existing apps if they were already open, I simply looked to see that the FMA was in the Applications folder once it completed. I did test that behavior (if app was open) for uninstalls tho (Column J).

https://docs.google.com/spreadsheets/d/1L2u7r3vZrNrEa6aOKFRKmiFDqpHOsXWekVr8VYFgMoE/edit?gid=1818352066#gid=1818352066

It would be nice to have the install check for existing version and include the logic to quit if open

@PezHub
Copy link
Contributor

PezHub commented Dec 2, 2024

Additional QA Notes:

  • if VSCode is previously installed but never launched, a re-install over it (fleet-maintained app) will succeed and the app will launch. This explains why my initial tests passed
  • a previously installed and launched version of VS Code will break when user or admin installs the fleet-maintained app over it. Same results that @noahtalerman posted in his video
  • confirmed the same behavior with another FMA (chrome)

So adding additional logic to check for existing installs and take action if present (similar to what Keeny mentioned above) makes sense

@noahtalerman noahtalerman added :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. and removed :incoming New issue in triage process. labels Dec 4, 2024
@noahtalerman
Copy link
Member Author

  • if VSCode is previously installed but never launched, a re-install over it (fleet-maintained app) will succeed and the app will launch. This explains why my initial tests passed
  • a previously installed and launched version of VS Code will break when user or admin installs the fleet-maintained app over it. Same results that @noahtalerman posted in his video
  • confirmed the same behavior with another FMA (chrome)

So adding additional logic to check for existing installs and take action if present (similar to what Keeny mentioned above) makes sense

@PezHub instead of adding additional logic to check, can we tweak the install script (see how Kenny did in recording here), to work w/o breaking the app in both of the above cases?

Just realized this wasn't on the release board. I moved it there. FYI @georgekarrv

@lukeheath lukeheath added ~released bug This bug was found in a stable release. and removed ~released bug This bug was found in a stable release. labels Dec 6, 2024
@lukeheath
Copy link
Member

Fleet version: Observed in Fleet's dogfood environment

@noahtalerman I'm not clear on if this is an unreleased bug or released bug. Would you please update? Thanks!

@PezHub PezHub added the ~released bug This bug was found in a stable release. label Dec 9, 2024
@PezHub
Copy link
Contributor

PezHub commented Dec 9, 2024

  • added ~released bug label
  • confirmed all 20 FMAs will re-install successfully over existing apps if they are never launched

However, if the FMA's have already been installed and launched/configured by the user, the following apps will break upon re-install so will need the install script tweaked (or a fix applied):

  1. 1Password
  2. Brave
  3. Docker
  4. Figma
  5. Chrome
  6. VSCode
  7. Firefox
  8. Notion
  9. Slack
  10. WhatsApp

@noahtalerman
Copy link
Member Author

However, if the FMA's have already been installed and launched/configured by the user, the following apps will break upon re-install so will need the install script tweaked (or a fix applied):

  1. 1Password
  2. Brave
  3. Docker
  4. Figma
  5. Chrome
  6. VSCode
  7. Firefox
  8. Notion
  9. Slack
  10. WhatsApp

@PezHub thanks! I added your list to the issue description under the "To fix" section.

FYI @georgekarrv

@georgekarrv georgekarrv added this to the 4.62.0-tentative milestone Dec 11, 2024
@getvictor
Copy link
Member

@mostlikelee should this be in #g-software?

@mostlikelee mostlikelee added ~backend Backend-related issue. #g-software Software product group and removed #g-mdm MDM product group labels Jan 2, 2025
@iansltx iansltx removed this from the 4.62.0 milestone Jan 2, 2025
@mostlikelee
Copy link
Contributor

renaming the issue to cover all app bundle copy installs

@mostlikelee mostlikelee changed the title Installing VS Code when it's already installed breaks VS Code Installing macOS app bundles when they're already installed breaks the app Jan 3, 2025
@mostlikelee mostlikelee changed the title Installing macOS app bundles when they're already installed breaks the app FMA: Installing macOS app bundles when they're already installed breaks the app Jan 3, 2025
@mostlikelee
Copy link
Contributor

Hey team! Please add your planning poker estimate with Zenhub @jahzielv @iansltx

@jahzielv
Copy link
Contributor

jahzielv commented Jan 7, 2025

in g-software standup today we discussed with @eugkuo and determined that we should be closing the application when a re-install is happening.

cc @mostlikelee

@jahzielv
Copy link
Contributor

jahzielv commented Jan 8, 2025

QA plan

  1. Validate that the migration updates existing install scripts for the above list of FMAs. Any of those FMAs that were previously added to a team and had their install scripts edited shouldn't have updated.
  2. Validate that triggering an ingestion of FMA data writes the updated install scripts
  3. For each of the FMAs listed, check that
    a. installing it manually works
    b. automatic install works
    c. closing the app and then installing it again works
    d. re-installing the app while the app is opened on the host should quit the app and successfully re-install

@iansltx
Copy link
Member

iansltx commented Jan 8, 2025

Additional QA check:

When reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Questions:

  1. Should we fail the install when the app is open and doesn't close?
  2. If the install fails when reinstalling an existing app, what should we do? Normally we run an uninstall script, but that uninstall script almost certainly doesn't have "move the backup app back" in it right now.

jahzielv added a commit that referenced this issue Jan 9, 2025
…5238)

> For #24148

# Checklist for submitter

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

<!-- Note that API documentation changes are now addressed by the
product design team. -->

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
See [Changes
files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/Committing-Changes.md#changes-files)
for more information.
- [x] Input data is properly validated, `SELECT *` is avoided, SQL
injection is prevented (using placeholders for values in statements)
- [x] If database migrations are included, checked table schema to
confirm autoupdate
- For database migrations:
- [x] Checked schema for all modified table for columns that will
auto-update timestamps during migration.
- [x] Confirmed that updating the timestamps is acceptable, and will not
cause unwanted side effects.
- [x] Ensured the correct collation is explicitly set for character
columns (`COLLATE utf8mb4_unicode_ci`).
- [x] Added/updated automated tests
- [x] A detailed QA plan exists on the associated ticket (if it isn't
there, work with the product group's QA engineer to add it)
- [x] Manual QA for all new/changed functionality
@jmwatts
Copy link
Member

jmwatts commented Jan 10, 2025

Full QA test plan with results (In progress)

QA Plan

Setup:
Start with previous version, ensure all FMAs listed above have been added to at least one team.
On another team, add all FMAs listed above but edit install scripts (make note of changes made)

Upgrade:

  • Upgrade to fix version, validate that the migration runs
    2025/01/10 14:18:08 [2025-01-09] Update FMA Install Scripts
  • Validate that the migration updates existing install scripts for the FMAs that did not have edited install scripts.
  • Validate that the migration DOES NOT update any existing FMAs that had their install scripts edited prior to upgrade

After Upgrade:
Add each FMA listed above and validate that they have the updated install scripts

1Password

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Brave

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Docker

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful Docker FMA installation fails with current install scriptΒ #25874
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Figma

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Chrome

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

VSCode

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Firefox

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Notion

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

Slack

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

WhatsApp

  • Validate it has the updated install scripts
  • Validate a manual install is successful
  • Validate an automatic install is successful
  • Validate opening app, closing app, then reinstalling app is successful
  • Validate opening app and leaving open while reinstalling forces the app to quit and reinstall is successful
  • Validate when reinstalling, at the end of a successful install we shouldn't have the backup of the old app in the temp dir.

@jmwatts
Copy link
Member

jmwatts commented Jan 10, 2025

@jahzielv I feel a little picky about this but... there are extra lines at the beginning of the updated scripts... is that intentional?
Before upgrade:
Screenshot 2025-01-10 at 2 18 49β€―PM

After upgrade:
Screenshot 2025-01-10 at 2 22 38β€―PM

@jahzielv
Copy link
Contributor

@jmwatts wasn't intentional, shouldn't affect functionality but I can look into removing them if we want (agree that it looks a bit weird)

@jmwatts
Copy link
Member

jmwatts commented Jan 10, 2025

@jahzielv I'm getting failed installs for Brave and Firefox so far when the app was already installed and open when reinstalling. Text files are attached below with the output.
Additionally, I'm seeing a lot of folders in /tmp. This is what it looked like after installing and re-installing 8/10 apps. When should those disappear?
Screenshot 2025-01-10 at 3 57 49β€―PM
BraveRe-installFail.txt
FirefoxRe-installFail.txt

@jahzielv
Copy link
Contributor

@jmwatts thanks for the report! I'll dig into those failures.

Yeah, we shouldn't be seeing stuff linger in /tmp. Orbit should be removing those. I'll try to repro on my end.

@jahzielv
Copy link
Contributor

did some pairing with @jmwatts this morning! Our findings:

  • the extra folders in /tmp are an existing bug. We defer the temp dir cleanup after trying to download the installer, so if there is an error in downloading the installer (like there was when Janis was testing, the installer is very large and ngrok timed out during download), we do not clean up the temp dir we created. We should move that defer higher up. Janis is filing a bug for this.
  • I was able to repro the firefox error pasted above. I'm looking into it now, probably something I changed in between my own tests and the PR merging.

@jmwatts
Copy link
Member

jmwatts commented Jan 14, 2025

@jahzielv will there be additional changes for the Firefox and Brave installers in this ticket or will those need an unreleased bug ticket to address the issues?

@jahzielv
Copy link
Contributor

jahzielv commented Jan 15, 2025

@jmwatts I will file the PR under this ticket! correction, I will attempt to reproduce again (I wasn't able to this morning) and if I'm able I'll file an unreleased bug

@jahzielv
Copy link
Contributor

I was able to reproduce, so filed an unreleased bug here: #25467
working on it now!

@jmwatts
Copy link
Member

jmwatts commented Jan 30, 2025

QA Notes
#25467 is resolved for Firefox and Brave.
#25373 is slated for fleetd 1.39.0
One additional line was removed from the updated script - it looks better now.
Per previous notes and QA Plan, 1Password, Brave, Figma, Chrome, VSCode, Firefox, Notion, Slack, and WhatsApp are installing and reinstalling successfully now.
#25874 is filed for Docker installation fails. Will wait for that to be finished and tested before this ticket will be moved to "Ready for release".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
~backend Backend-related issue. bug Something isn't working as documented customer-numa #g-software Software product group :release Ready to write code. Scheduled in a release. See "Making changes" in handbook. ~released bug This bug was found in a stable release.
Development

No branches or pull requests

9 participants