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

Disk encryption keys are now archived when created/updated #25638

Merged
merged 8 commits into from
Jan 22, 2025

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Jan 21, 2025

For #25609

Manual QA in progress. Putting this "In Review" since it is a P1.

Video explaining the PR: https://youtu.be/bUwIdjBLqiM

Checklist for submitter

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
  • If database migrations are included, checked table schema to confirm autoupdate
  • For database migrations:
    • Checked schema for all modified table for columns that will auto-update timestamps during migration.
    • Confirmed that updating the timestamps is acceptable, and will not cause unwanted side effects.
    • Ensured the correct collation is explicitly set for character columns (COLLATE utf8mb4_unicode_ci).
  • Added/updated automated tests
  • 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)
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 73.62637% with 72 lines in your changes missing coverage. Please review.

Project coverage is 63.61%. Comparing base (0860996) to head (682bce2).
Report is 27 commits behind head on main.

Files with missing lines Patch % Lines
server/datastore/mysql/disk_encryption.go 75.34% 43 Missing and 11 partials ⚠️
...20250121094045_AddHostDiskEncryptionKeysArchive.go 65.30% 12 Missing and 5 partials ⚠️
server/service/orbit.go 50.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25638      +/-   ##
==========================================
+ Coverage   63.59%   63.61%   +0.02%     
==========================================
  Files        1622     1624       +2     
  Lines      155038   155289     +251     
  Branches     3964     3964              
==========================================
+ Hits        98589    98788     +199     
- Misses      48681    48704      +23     
- Partials     7768     7797      +29     
Flag Coverage Δ
backend 64.47% <73.62%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@getvictor getvictor changed the title Disk encryption keys are now archived when host moves teams Disk encryption keys are now archived when created/updated Jan 22, 2025
@getvictor getvictor marked this pull request as ready for review January 22, 2025 18:19
@getvictor getvictor requested a review from a team as a code owner January 22, 2025 18:19
Copy link
Contributor

@gillespi314 gillespi314 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for relocating various pieces into the new disk_encryption file.

Comment on lines +277 to +278
// We are using Apple's encryption profile to determine if any hosts, including Windows and Linux, are encrypted.
// This is a safe assumption since encryption is enabled for the whole team.
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make further changes to this PR, would you mind adding a note here that we should consider basing this on the applicable config settings rather than the Apple profile (which we can leave for a future improvement).

@getvictor getvictor merged commit 62b7412 into main Jan 22, 2025
35 checks passed
@getvictor getvictor deleted the victor/25609-archive-encryption-keys branch January 22, 2025 20:54
getvictor added a commit that referenced this pull request Jan 22, 2025
For #25609

Manual QA in progress. Putting this "In Review" since it is a P1.

Video explaining the PR: https://youtu.be/bUwIdjBLqiM

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
- [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

(cherry picked from commit 62b7412)
getvictor added a commit that referenced this pull request Jan 22, 2025
For #25609

Manual QA in progress. Putting this "In Review" since it is a P1.

Video explaining the PR: https://youtu.be/bUwIdjBLqiM

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
- [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

(cherry picked from commit 62b7412)
getvictor added a commit that referenced this pull request Jan 23, 2025
…25697)

For #25609

Video explaining the PR: https://youtu.be/bUwIdjBLqiM

# Checklist for submitter

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
- [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

(cherry picked from commit 62b7412)
georgekarrv pushed a commit that referenced this pull request Jan 24, 2025
For #25609

Manual QA in progress. Putting this "In Review" since it is a P1.

Video explaining the PR: https://youtu.be/bUwIdjBLqiM

- [x] Changes file added for user-visible changes in `changes/`,
`orbit/changes/` or `ee/fleetd-chrome/changes`.
- [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants