Check for calendar updates after callbacks from Google by getvictor · Pull Request #20156 · fleetdm/fleet

getvictor · 2024-07-02T16:16:31Z

#19352

Video explaining code changes: https://www.loom.com/share/370200a276b84aa388effd6ebd762e01?sid=038508c4-f3c2-40c0-baf6-6b6df682d1f0

In maintenance windows using Google Calendar, calendar event is now recreated within 30 seconds if deleted or moved to the past.

Added new endpoint for Google Calendar: /api/_version_/fleet/calendar/webhook/{event_uuid}
Added UUID to calendar_events table to make webhook lookup more efficient
webhook endpoint will only recreate event if needed -- it will not fire webhook. Webhook is still done by the cron job.

Checklist for submitter

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

Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
See Changes files for more information.
Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
Added/updated tests
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).
Manual QA for all new/changed functionality
- For Orbit and Fleet Desktop changes:

lucasmrod

LGTM!

Left a couple of questions to under stand some of the changes before approving.

We should let the product team know that this new feature happens for new events, right?

lucasmrod · 2024-07-08T10:44:01Z


 func Up_20240626195531(tx *sql.Tx) error {
-	if _, err := tx.Exec(`ALTER TABLE calendar_events ADD COLUMN timezone VARCHAR(64) NULL`); err != nil {
+	if _, err := tx.Exec(`ALTER TABLE calendar_events ADD COLUMN timezone VARCHAR(64) COLLATE utf8mb4_unicode_ci NULL`); err != nil {


This migration wasn't released yet, right?

(And thanks for catching this!)

lucasmrod · 2024-07-08T10:57:00Z

+func Up_20240707134035(tx *sql.Tx) error {
+	// UUID is a 36-character string with the most common 8-4-4-4-12 format, xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx
+	// Reference: https://en.wikipedia.org/wiki/Universally_unique_identifier#Textual_representation
+	if _, err := tx.Exec(`ALTER TABLE calendar_events ADD COLUMN uuid VARCHAR(36) COLLATE utf8mb4_unicode_ci NOT NULL`); err != nil {


We use UUID v4, the entirely random version of UUIDs. Such randomness make writes expensive (rebalancing
B+ trees on random-value writes is expensive). See https://planetscale.com/blog/the-problem-with-using-a-uuid-primary-key-in-mysql. That said, it is maybe a-ok for now as this is not a heavy used table like hosts.

Maybe an optimization for later: we could use BINARY(16); see https://dev.mysql.com/blog-archive/storing-uuid-values-in-mysql-tables/

This makes sense. I'll try to get this optimization in before the release.

lucasmrod · 2024-07-08T11:13:35Z

+			team1Policy1Calendar.ID: ptr.Bool(true),
+			team1Policy2.ID:         ptr.Bool(false),


Curious why you are changing from team2 to team1.

This looked like a copy/paste error -- I think it was supposed to be team1 initially.

lucasmrod · 2024-07-08T11:23:00Z

+		},
+	)
+	require.NoError(t, err)
+	team1Policy2, err := s.ds.NewTeamPolicy(


Rename to team1Policy2Calendar? (has CalendarEventsEnabled : true), or set CalendarEventsEnabled: false

Ok, will make the change for the next PR.

lucasmrod · 2024-07-08T11:36:50Z

+	// Apply current migration.
+	applyNext(t, db)
+
+	// check that it's NULL


// check that it's not NULL
?

Yes, will fix in the next PR

…20277) #19352 Fix for code review comment: #20156 (comment) Also includes changes from #20252 # Checklist for submitter If some of the following don't apply, delete the relevant line.  - [x] Added/updated tests - [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] Manual QA for all new/changed functionality

…20277) #19352 Fix for code review comment: #20156 (comment) Also includes changes from #20252 # Checklist for submitter If some of the following don't apply, delete the relevant line.  - [x] Added/updated tests - [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] Manual QA for all new/changed functionality (cherry picked from commit 7bcd61a)

getvictor temporarily deployed to Docker Hub July 2, 2024 16:16 — with GitHub Actions Inactive

github-advanced-security AI found potential problems Jul 2, 2024

View reviewed changes

Comment thread server/service/calendar/calendar.go Outdated

Check failure

Code scanning / CodeQL

Incorrect conversion between integer types

Incorrect conversion of an unsigned 64-bit integer from [strconv.ParseUint](1) to a lower bit size type uint without an upper bound check.

getvictor force-pushed the victor/19352-calendar-real-time branch from 928268c to 11f8cd2 Compare July 2, 2024 16:47

getvictor temporarily deployed to Docker Hub July 2, 2024 16:47 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 2, 2024 18:53 — with GitHub Actions Inactive

getvictor force-pushed the victor/19352-calendar-real-time branch from 8b6c6ec to 119665c Compare July 2, 2024 19:15

getvictor temporarily deployed to Docker Hub July 2, 2024 19:15 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 2, 2024 21:48 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 2, 2024 22:13 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 2, 2024 22:29 — with GitHub Actions Inactive

getvictor force-pushed the victor/19352-calendar-real-time branch from 67b2a55 to b129369 Compare July 3, 2024 13:48

getvictor temporarily deployed to Docker Hub July 3, 2024 13:48 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 3, 2024 14:15 — with GitHub Actions Inactive

getvictor temporarily deployed to Docker Hub July 3, 2024 14:47 — with GitHub Actions Inactive

getvictor marked this pull request as ready for review July 3, 2024 15:05

getvictor requested review from a team, gillespi314, lucasmrod and roperzh as code owners July 3, 2024 15:05

getvictor assigned lucasmrod Jul 3, 2024

getvictor temporarily deployed to Docker Hub July 3, 2024 15:08 — with GitHub Actions Inactive

getvictor mentioned this pull request Jul 5, 2024

Add hostname to calendar event (maintenance window) bodies #20235

Merged

3 tasks

getvictor added 8 commits July 7, 2024 10:32

Initial implementation

beefb16

Fixing tests

28b1f5c

Added comment.

6966e8c

Added calendar integration test.

d7291c9

Cleanup and test fixes

eab8b00

More tests

73aaf73

Merge to main

569f1fd

Added optimization to stop event channel when webhook is pending.

2905552

getvictor added 3 commits July 7, 2024 10:46

Fix test.

25ed493

Added a comment -- no functional changes.

cba6598

Merge to main

d184869

getvictor force-pushed the victor/19352-calendar-real-time branch from c82cf43 to d184869 Compare July 7, 2024 15:50

getvictor temporarily deployed to Docker Hub July 7, 2024 15:50 — with GitHub Actions Inactive

lucasmrod reviewed Jul 8, 2024

View reviewed changes

lucasmrod approved these changes Jul 8, 2024

View reviewed changes

getvictor merged commit df141cd into main Jul 8, 2024

getvictor deleted the victor/19352-calendar-real-time branch July 8, 2024 15:20

getvictor mentioned this pull request Jul 9, 2024

Distributed lock and store calendar_events UUID as binary in MySQL #20277

Merged

6 tasks

		team1Policy1Calendar.ID: ptr.Bool(true),
		team1Policy2.ID: ptr.Bool(false),

Conversation

getvictor commented Jul 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist for submitter

Uh oh!

Check failure

lucasmrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

getvictor commented Jul 2, 2024 •

edited

Loading