Skip to content

Commit b6cd571

Browse files
committed
Address review: require query for dynamic labels, move platform validation into switch
- Dynamic case now rejects empty query (matches GitOps parser) - Platform validation moved inside dynamic case only (manual/host_vitals already reject any platform, so the top-level check was misleading) - Added test for dynamic label missing query
1 parent cbc195b commit b6cd571

2 files changed

Lines changed: 20 additions & 6 deletions

File tree

server/service/labels.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -563,12 +563,6 @@ func (svc *Service) ApplyLabelSpecs(ctx context.Context, specs []*fleet.LabelSpe
563563
var specLabelNamesNeedingMoving []string // should match namesToMove once specs have been checked
564564

565565
for _, spec := range specs {
566-
if _, ok := fleet.ValidLabelPlatformVariants[spec.Platform]; !ok {
567-
return fleet.NewUserMessageError(
568-
ctxerr.Errorf(ctx, "invalid platform: %s", spec.Platform), http.StatusUnprocessableEntity,
569-
)
570-
}
571-
572566
// Validate mutually exclusive field combinations per label membership type
573567
switch spec.LabelMembershipType {
574568
case fleet.LabelMembershipTypeManual:
@@ -588,6 +582,11 @@ func (svc *Service) ApplyLabelSpecs(ctx context.Context, specs []*fleet.LabelSpe
588582
)
589583
}
590584
case fleet.LabelMembershipTypeDynamic:
585+
if spec.Query == "" {
586+
return fleet.NewUserMessageError(
587+
ctxerr.Errorf(ctx, "label %s is declared as dynamic but is missing a query", spec.Name), http.StatusUnprocessableEntity,
588+
)
589+
}
591590
if spec.HostVitalsCriteria != nil {
592591
return fleet.NewUserMessageError(
593592
ctxerr.Errorf(ctx, "label %s is declared as dynamic but contains criteria", spec.Name), http.StatusUnprocessableEntity,
@@ -598,6 +597,11 @@ func (svc *Service) ApplyLabelSpecs(ctx context.Context, specs []*fleet.LabelSpe
598597
ctxerr.Errorf(ctx, "label %s is declared as dynamic but contains hosts", spec.Name), http.StatusUnprocessableEntity,
599598
)
600599
}
600+
if _, ok := fleet.ValidLabelPlatformVariants[spec.Platform]; !ok {
601+
return fleet.NewUserMessageError(
602+
ctxerr.Errorf(ctx, "label %s has invalid platform: %s", spec.Name, spec.Platform), http.StatusUnprocessableEntity,
603+
)
604+
}
601605
case fleet.LabelMembershipTypeHostVitals:
602606
if spec.HostVitalsCriteria == nil {
603607
return fleet.NewUserMessageError(

server/service/labels_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -768,6 +768,16 @@ func TestApplyLabelSpecsManualLabelNilHosts(t *testing.T) {
768768
require.Error(t, err)
769769
require.ErrorContains(t, err, "declared as dynamic but contains hosts")
770770

771+
// Dynamic label without query should be rejected
772+
err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{
773+
{
774+
Name: "dynamic_no_query",
775+
LabelMembershipType: fleet.LabelMembershipTypeDynamic,
776+
},
777+
}, nil, nil)
778+
require.Error(t, err)
779+
require.ErrorContains(t, err, "declared as dynamic but is missing a query")
780+
771781
// Dynamic label with criteria should be rejected
772782
err = svc.ApplyLabelSpecs(ctx, []*fleet.LabelSpec{
773783
{

0 commit comments

Comments
 (0)