Allow fleet host ID when specifying Gitops manual label hosts - #33078
Conversation
GitHub Actions
Inactive
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughEnables manual GitOps labels to accept Fleet host IDs in addition to string identifiers. Introduces custom JSON unmarshalling for label hosts to accept strings or integers (serialized as strings), updates SQL to match hosts by ID, and adjusts tests and testdata to reflect the new parsing and matching behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as GitOps YAML
participant Loader as Config Loader
participant API as Fleet API
participant Parser as HostsSlice Unmarshal
participant DB as MySQL (labels)
participant LM as label_membership
User->>Loader: Provide label specs (hosts: ["host1", 2, ...])
Loader->>API: ApplyLabelSpecsWithAuthor(specs)
API->>Parser: Unmarshal LabelSpec.Hosts
Note right of Parser: Accept strings and integers<br/>Reject non-integer numbers
Parser-->>API: Hosts normalized as []string
API->>DB: INSERT…SELECT for manual labels<br/>(match by hostname, serial, uuid, or ID)
DB-->>LM: Upsert matching host memberships
DB-->>API: Result
API-->>Loader: Done
sequenceDiagram
autonumber
actor User as GitOps YAML (invalid)
participant Loader
participant API
participant Parser
User->>Loader: hosts: [2.5]
Loader->>API: ApplyLabelSpecsWithAuthor
API->>Parser: Unmarshal hosts
Parser-->>API: Error "hosts must be strings or integers, got float 2.5"
API-->>Loader: Validation error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/datastore/mysql/labels.go (1)
170-188: Batch size must account for 4 IN lists now (not 3).Adjust batchHostnames to avoid exceeding MySQL’s parameter limit when 4 copies are bound.
Apply this diff:
func batchHostnames(hostnames []string) [][]string { @@ - // WARNING: This is used in ApplyLabelSpecsWithAuthor and the batch sizes have to be small - // enough to allow for three copies each hostname list in the query. The batch size is 20_000 - // because 60_001 binding arguments is less than the maximum of 65,535. - - const batchSize = 20_000 // Large, but well under the undocumented limit + // WARNING: Used in ApplyLabelSpecsWithAuthor. The query binds the host list 4 times + // (hostname, hardware_serial, uuid, id) plus 1 for label_id. Keep 1 + 4*N <= 65_535. + // Choose 16_000 to stay comfortably under the limit: 1 + 4*16_000 = 64,001. + const batchSize = 16_000
🧹 Nitpick comments (2)
server/fleet/labels.go (1)
186-209: Unmarshal robustness: handle null and tighten error formatting.
- Handle JSON null for hosts to avoid failing on “hosts: ” cases.
- Switch error formatting to %g (matches tests).
Apply this diff:
func (s *HostsSlice) UnmarshalJSON(data []byte) error { - var raw []interface{} + // Accept null (treat as empty slice) + if string(data) == "null" { + *s = nil + return nil + } + var raw []interface{} if err := json.Unmarshal(data, &raw); err != nil { return err } var result []string for _, v := range raw { switch val := v.(type) { case string: result = append(result, val) case float64: // Check if the float64 is actually an integer if val != float64(int64(val)) { - return fmt.Errorf("hosts must be strings or integers, got float %f", val) + return fmt.Errorf("hosts must be strings or integers, got float %g", val) } // Convert to string - result = append(result, fmt.Sprintf("%.0f", val)) + result = append(result, fmt.Sprintf("%.0f", val)) default: return fmt.Errorf("hosts must be strings or integers, got %T", v) } } *s = result return nil }server/datastore/mysql/labels_test.go (1)
31-44: Update expectations if batch size changes to 16k.If you adopt the 16k batch to respect 4×IN, this test must reflect 7 batches with the last sized 14k.
Proposed update:
- require.Equal(t, 6, len(batched)) - assert.Equal(t, large[:20_000], batched[0]) - assert.Equal(t, large[20_000:40_000], batched[1]) - assert.Equal(t, large[40_000:60_000], batched[2]) - assert.Equal(t, large[60_000:80_000], batched[3]) - assert.Equal(t, large[80_000:100_000], batched[4]) - assert.Equal(t, large[100_000:110_000], batched[5]) + require.Equal(t, 7, len(batched)) + assert.Equal(t, large[:16_000], batched[0]) + assert.Equal(t, large[16_000:32_000], batched[1]) + assert.Equal(t, large[32_000:48_000], batched[2]) + assert.Equal(t, large[48_000:64_000], batched[3]) + assert.Equal(t, large[64_000:80_000], batched[4]) + assert.Equal(t, large[80_000:96_000], batched[5]) + assert.Equal(t, large[96_000:110_000], batched[6])
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
changes/32014-allow-fleet-host-ids-in-gitops-labels(1 hunks)pkg/spec/gitops_test.go(2 hunks)pkg/spec/testdata/global_config_no_paths.yml(1 hunks)pkg/spec/testdata/top.labels.yml(1 hunks)server/datastore/mysql/labels.go(1 hunks)server/datastore/mysql/labels_test.go(2 hunks)server/fleet/labels.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/datastore/mysql/labels.goserver/fleet/labels.goserver/datastore/mysql/labels_test.gopkg/spec/gitops_test.go
🔇 Additional comments (6)
changes/32014-allow-fleet-host-ids-in-gitops-labels (1)
1-1: Changelog entry reads well.Clear, scoped, and matches the PR behavior.
pkg/spec/testdata/top.labels.yml (1)
9-9: LGTM: numeric host ID in testdata.Matches new parsing semantics (strings or integers).
pkg/spec/testdata/global_config_no_paths.yml (1)
205-205: LGTM: numeric host ID in testdata.Consistent with the updated unmarshalling rules.
pkg/spec/gitops_test.go (1)
275-276: LGTM: expecting stringified host ID.This validates the “int in YAML → string in struct” behavior.
server/fleet/labels.go (1)
219-221: LabelSpec.Hosts type swap looks correct.The switch to HostsSlice will engage the custom unmarshalling; no API breakage for code using []string semantics.
server/datastore/mysql/labels_test.go (1)
2008-2016: LGTM: coverage for host ID membership.Good addition validating ID-based matching.
Also applies to: 2026-2039
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #33078 +/- ##
=======================================
Coverage 63.87% 63.87%
=======================================
Files 2046 2046
Lines 201924 201962 +38
Branches 6686 6686
=======================================
+ Hits 128971 129011 +40
Misses 62779 62779
+ Partials 10174 10172 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
GitHub Actions
Error
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Error
GitHub Actions
Inactive
| assert.Equal(t, "host1", gitops.Labels[1].Hosts[0]) | ||
| assert.Equal(t, "host2", gitops.Labels[1].Hosts[1]) | ||
| assert.Equal(t, "2", gitops.Labels[1].Hosts[1]) |
There was a problem hiding this comment.
Testing that we can use a number in the YAML (see updates to the global_config_no_paths.yml and top.labels.yml test files.
| // enough to allow for three copies each hostname list in the query. The batch size is 20_000 | ||
| // enough to allow for three copies each hostname list in the query. The batch size is 15_000 | ||
| // because 60_001 binding arguments is less than the maximum of 65,535. | ||
|
|
||
| const batchSize = 20_000 // Large, but well under the undocumented limit | ||
| const batchSize = 15_000 // Large, but well under the undocumented limit |
There was a problem hiding this comment.
Because we're adding another arg binding to the SQL query above, we now have 4 bindings so to keep it at 60k total per patch we need to drop the batch size down.
| // Because `Hosts` for manual labels matches both host name AND host ID, | ||
| // specifying "1" will match both host with ID 1 (whose name is "0") | ||
| // and host with name "1". | ||
| expectedSpecs[4].Hosts = []string{"0", "1", "2", "3", "4"} | ||
| return expectedSpecs |
There was a problem hiding this comment.
I could have updated the test so that the host names have letters in them, but decided to keep them as-is to reinforce that we can now have collisions between ID and host name, serial or uuid.
| "foo.local", | ||
| "hwd2", | ||
| "uuid3", | ||
| strconv.Itoa(int(host4.ID)), //nolint:gosec // dismiss G115 |
There was a problem hiding this comment.
Again testing that numbers in the label spec make it through ok.
2 participants
for #32014
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Validation