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

Add clearer error for VPP token constraint failure #21967

Merged
merged 4 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/21890-vpp-token-error
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Improve messaging for VPP token constraint errors
17 changes: 15 additions & 2 deletions server/datastore/mysql/vpp.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/fleetdm/fleet/v4/server/contexts/ctxerr"
"github.com/fleetdm/fleet/v4/server/fleet"
"github.com/fleetdm/fleet/v4/server/mdm/nanomdm/mdm"
"github.com/go-sql-driver/mysql"
"github.com/jmoiron/sqlx"
)

Expand Down Expand Up @@ -780,6 +781,7 @@ TEAMLOOP:
}

func (ds *Datastore) UpdateVPPTokenTeams(ctx context.Context, id uint, teams []uint) (*fleet.VPPTokenDB, error) {
stmtTeamName := `SELECT name FROM teams WHERE id = ?`
stmtRemove := `DELETE FROM vpp_token_teams WHERE vpp_token_id = ?`
stmtInsert := `
INSERT INTO
Expand Down Expand Up @@ -858,6 +860,17 @@ func (ds *Datastore) UpdateVPPTokenTeams(ctx context.Context, id uint, teams []u
})

if err != nil {
var mysqlErr *mysql.MySQLError
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example of how a duplicate error is handed elsewhere in the codebase, sorry should have linked this originally! IsDuplicate does the cast to a mysql.MySQLError, so that should be ok to leave out here.

https://github.com/fleetdm/fleet/blob/main/server/datastore/mysql/app_configs.go#L172-L175

Copy link
Member Author

Choose a reason for hiding this comment

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

I do the cast so I can extract the ID of the duplicate below, but good to know as well!

// https://dev.mysql.com/doc/mysql-errors/8.4/en/server-error-reference.html#error_er_dup_entry
if errors.As(err, &mysqlErr) && IsDuplicate(err) {
var dupeTeamID uint
var dupeTeamName string
fmt.Sscanf(mysqlErr.Message, "Duplicate entry '%d' for", &dupeTeamID)
if err := sqlx.GetContext(ctx, ds.reader(ctx), &dupeTeamName, stmtTeamName, dupeTeamID); err != nil {
return nil, ctxerr.Wrap(ctx, err, "getting team name for vpp token conflict error")
}
return nil, ctxerr.Wrap(ctx, fleet.ErrVPPTokenTeamConstraint{Name: dupeTeamName, ID: &dupeTeamID})
}
return nil, ctxerr.Wrap(ctx, err, "modifying vpp token team associations")
}

Expand Down Expand Up @@ -1127,7 +1140,7 @@ func checkVPPNullTeam(ctx context.Context, tx sqlx.ExtContext, currentID *uint,
}

if allTeamsFound && currentID != nil && *currentID != id {
return ctxerr.Wrap(ctx, errors.New("All teams token already exists"))
return ctxerr.Wrap(ctx, fleet.ErrVPPTokenTeamConstraint{Name: fleet.ReservedNameAllTeams})
}

if nullTeam != fleet.NullTeamNone {
Expand All @@ -1139,7 +1152,7 @@ func checkVPPNullTeam(ctx context.Context, tx sqlx.ExtContext, currentID *uint,
return ctxerr.Wrap(ctx, err, "scanning row in check vpp token null team")
}
if currentID == nil || *currentID != id {
return ctxerr.Errorf(ctx, "vpp token for team %s already exists", nullTeam)
return ctxerr.Wrap(ctx, fleet.ErrVPPTokenTeamConstraint{Name: nullTeam.PrettyName()})
}
}

Expand Down
2 changes: 2 additions & 0 deletions server/datastore/mysql/vpp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -981,8 +981,10 @@ func testVPPTokensCRUD(t *testing.T, ds *Datastore) {
assert.Error(t, err)
_, err = ds.UpdateVPPTokenTeams(ctx, tokBadConstraint.ID, []uint{team.ID})
assert.Error(t, err)
assert.ErrorContains(t, err, "\"Kritters\" team already has a VPP token.")
_, err = ds.UpdateVPPTokenTeams(ctx, tokBadConstraint.ID, []uint{0})
assert.Error(t, err)
assert.ErrorContains(t, err, "\"No team\" team already has a VPP token.")

toks, err = ds.ListVPPTokens(ctx)
assert.NoError(t, err)
Expand Down
11 changes: 11 additions & 0 deletions server/fleet/mdm.go
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,17 @@ const (
NullTeamNoTeam NullTeamType = "noteam"
)

func (n NullTeamType) PrettyName() string {
switch n {
case NullTeamAllTeams:
return ReservedNameAllTeams
case NullTeamNoTeam:
return ReservedNameNoTeam
default:
return string(n)
}
}

type AppleDevice int

const (
Expand Down
10 changes: 10 additions & 0 deletions server/fleet/vpp.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fleet

import (
"fmt"
"time"
)

Expand Down Expand Up @@ -67,3 +68,12 @@ type VPPAppStatusSummary struct {
// Failed is the number of hosts that have the VPP app installation failed.
Failed uint `json:"failed" db:"failed"`
}

type ErrVPPTokenTeamConstraint struct {
Name string
ID *uint
}

func (e ErrVPPTokenTeamConstraint) Error() string {
return fmt.Sprintf("Error: %q team already has a VPP token. Each team can only have one VPP token.", e.Name)
}
4 changes: 4 additions & 0 deletions server/service/appconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,10 @@ func (svc *Service) ModifyAppConfig(ctx context.Context, p []byte, applyOpts fle
if appConfig.MDM.VolumePurchasingProgram.Set && appConfig.MDM.VolumePurchasingProgram.Valid {
for tokenID, tokenTeams := range vppAssignments {
if _, err := svc.ds.UpdateVPPTokenTeams(ctx, tokenID, tokenTeams); err != nil {
var errTokConstraint fleet.ErrVPPTokenTeamConstraint
if errors.As(err, &errTokConstraint) {
return nil, ctxerr.Wrap(ctx, fleet.NewUserMessageError(errTokConstraint, http.StatusConflict))
}
return nil, ctxerr.Wrap(ctx, err, "saving ABM token assignments")
}
}
Expand Down
Loading