Skip to content

Add fleetctl upgrade-packs command to migrate 2017 packs to queries - #13078

Merged
mna merged 20 commits into
mainfrom
mna-12656-migrate-user-packs-yaml
Aug 8, 2023
Merged

Add fleetctl upgrade-packs command to migrate 2017 packs to queries#13078
mna merged 20 commits into
mainfrom
mna-12656-migrate-user-packs-yaml

Conversation

@mna

@mna mna commented Aug 1, 2023

Copy link
Copy Markdown
Contributor

#12656

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/.
    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
  • Manual QA for all new/changed functionality

@mna
mna temporarily deployed to Docker Hub August 1, 2023 14:44 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 14:45 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 15:18 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 17:29 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 17:29 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:04 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:05 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:06 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:31 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:31 — with GitHub Actions Inactive
Comment thread cmd/fleetctl/upgrade_packs_test.go Outdated

// expects a global query for p1 (targets a label) and per-team queries for p2 (t1 and t2)
b, err := os.ReadFile(filepath.Join("testdata", "expectedUpgradePacks.yml"))
require.NoError(t, err)

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.

For a reason I can't understand right now, this fails constantly on CI (but passes locally) with this error:

=== RUN   TestFleetctlUpgradePacks_NonEmpty
    upgrade_packs_test.go:326: 
        	Error Trace:	/home/runner/work/fleet/fleet/cmd/fleetctl/upgrade_packs_test.go:326
        	Error:      	Received unexpected error:
        	            	open testdata/expectedUpgradePacks.yml: no such file or directory
        	Test:       	TestFleetctlUpgradePacks_NonEmpty
--- FAIL: TestFleetctlUpgradePacks_NonEmpty (0.32s)

Of course the file is in this PR/branch/commit. This reads the content the same way that we do in e.g. get_test.go or apply_test.go. I'm at a loss, I will move the expected content as a string in the test, but if anyone has any idea (I did check file permissions and it is the same as the other testdata files AFAICS).

@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:56 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 1, 2023 18:56 — with GitHub Actions Inactive
@codecov

codecov Bot commented Aug 1, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 81.19% and project coverage change: +0.13% 🎉

Comparison is base (554e024) 58.88% compared to head (e079300) 59.02%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #13078      +/-   ##
==========================================
+ Coverage   58.88%   59.02%   +0.13%     
==========================================
  Files         867      867              
  Lines       71296    71356      +60     
  Branches     1850     1802      -48     
==========================================
+ Hits        41986    42121     +135     
+ Misses      25891    25809      -82     
- Partials     3419     3426       +7     
Flag Coverage Δ
backend 59.42% <81.19%> (+0.09%) ⬆️

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

Files Changed Coverage Δ
cmd/fleetctl/fleetctl.go 63.76% <ø> (ø)
server/service/client_packs.go 0.00% <0.00%> (ø)
cmd/fleetctl/upgrade_packs.go 85.64% <85.64%> (ø)
cmd/fleetctl/get.go 73.83% <100.00%> (+0.50%) ⬆️

... and 30 files with indirect coverage changes

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

@mna
mna marked this pull request as ready for review August 2, 2023 13:06
@mna
mna requested review from a team and rachaelshaw as code owners August 2, 2023 13:06
@mna
mna temporarily deployed to Docker Hub August 2, 2023 13:08 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 2, 2023 13:08 — with GitHub Actions Inactive

@gillespi314 gillespi314 left a comment

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.

@@ -0,0 +1,434 @@
package main

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.

Great tests!

Comment thread docs/Get started/FAQ.md
Packs are a function of osquery that provide a portable format to import /export queries in and out of platforms like Fleet. These osquery packs still exist, but have been removed from the Fleet UI. Access via API is still available for backwards compatibility.
Within Fleet we've introduced the concept of teams in Fleet premium to target specific groups of hosts, but you can also still use scheduled queries in Fleet free (works like packs) to target all your hosts.

The `fleetctl upgrade-packs` command can be used to convert existing packs to queries.

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.

Seeing this in context, I'm realizing we probably need to update the "Where did Packs go?" answer to be more aligned with the "Why this way?" section about packs. That said, this change totally fits with the existing content, so I don't want to let that hold up this PR— just saying this as an FYI.

cc @zhumo not sure how we track inconsistencies in the docs right now so just filed it as a bug; happy to take that one on after this PR is merged.

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.

I think that's a good approach Rachael, please go ahead.

@rachaelshaw rachaelshaw 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.

1 note about the docs as an FYI, but no need for it hold up this PR ✅

@mna
mna temporarily deployed to Docker Hub August 7, 2023 21:47 — with GitHub Actions Inactive
@mna
mna temporarily deployed to Docker Hub August 7, 2023 21:47 — with GitHub Actions Inactive
@mna

mna commented Aug 8, 2023

Copy link
Copy Markdown
Contributor Author

The 2 failing CI checks are known to fail consistently those last few days, unrelated to this specific PR. Merging.

@mna
mna merged commit 37ba43d into main Aug 8, 2023
@mna
mna deleted the mna-12656-migrate-user-packs-yaml branch August 8, 2023 12:21
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