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

support preprod dual install #1834

Merged
merged 9 commits into from
Aug 16, 2024
Merged

Conversation

zackattack01
Copy link
Contributor

@zackattack01 zackattack01 commented Aug 14, 2024

to support multiple installations:

  • wires build time identifier used into launcher.flags
  • uses this from windows service configuration to determine the correct service name and launcher registry keys
    • minor refactoring to prevent repeatedly opening and closing service manager/service connections
  • makes flare shipper's enroll secret collection fall back to knapsack's EnrollSecretPath if set, to ensure collecting the correct secret when run in situ
  • adds multiple installation check to flares, to note any potential discrepancies that may arise from multiple installations in flare checkups

@zackattack01 zackattack01 force-pushed the zack/support_preprod_dual_install branch from dc77ec7 to 2289b74 Compare August 14, 2024 18:07
Comment on lines 271 to 282
// TODO this will need to respect the identifier when determining the secret file location for dual-launcher installations
b, err := os.ReadFile(launcher.DefaultPath(launcher.SecretFile))
Copy link
Contributor

Choose a reason for hiding this comment

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

oooo.... This one we might need to fix. Can we shift it to using the secret in something configured instead of DefaultPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can make this fallback to reading from knapsack.EnrollSecretPath() first and that will fix this when running in situ at least. fixing for CLI triggered flares looks tougher

@zackattack01 zackattack01 force-pushed the zack/support_preprod_dual_install branch from 4cd67c9 to 978c39a Compare August 14, 2024 21:51
@zackattack01 zackattack01 marked this pull request as ready for review August 15, 2024 16:33
James-Pickett
James-Pickett previously approved these changes Aug 15, 2024
Copy link
Contributor

@James-Pickett James-Pickett left a comment

Choose a reason for hiding this comment

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

Nice

directionless
directionless previously approved these changes Aug 16, 2024
Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I think this is okay to try!

// by multiple installations. This is less of an issue when the flare is run in situ, but should be noted because
// we may need to pay closer attention to the results. When run standalone without a config argument passed, it would be
// possible for flare to default to reading the directories for the wrong installation
func noteMultipleInstallations(z *zip.Writer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels weird not to push this through the normal checkup interface. But okay...

Copy link
Contributor

@directionless directionless left a comment

Choose a reason for hiding this comment

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

I think this is okay! Woo!

@zackattack01 zackattack01 added this pull request to the merge queue Aug 16, 2024
Merged via the queue into main with commit c48ff7c Aug 16, 2024
29 checks passed
@zackattack01 zackattack01 deleted the zack/support_preprod_dual_install branch August 16, 2024 20:46
@RebeccaMahany RebeccaMahany added the component:build&packaging Build and Package label Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:build&packaging Build and Package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants