diff --git a/CHANGELOG.md b/CHANGELOG.md index 55ce6ca6..60c053e8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,11 @@ ## v2.0.1 -FEATURE: Added several new `admin` API endpoints for interfacing with additional management controls: finding limit classes by label, finding applied/applying/removing limit classes from accounts, getting/setting skip interstitial status for an account (https://github.com/openziti/zrok/issues/726) +FEATURE: Added several new `admin` API endpoints for interfacing with additional management controls: finding limit classes by label, finding applied/applying/removing limit classes from accounts, getting/setting skip interstitial status for an account (https://github.com/openziti/zrok/issues/1210) + +CHANGE: Removed the legacy `admin` `/grants` endpoint. Its prior synchronization behavior is now replaced by the new skip interstitial grant management endpoints. (https://github.com/openziti/zrok/issues/726) + +CHANGE: Applying limit classes now validates requested assignments to prevent conflicting effective limit class combinations on an account (https://github.com/openziti/zrok/issues/726) ## v2.0.0 diff --git a/controller/applyLimitClasses.go b/controller/applyLimitClasses.go index 8778a822..43383a5e 100644 --- a/controller/applyLimitClasses.go +++ b/controller/applyLimitClasses.go @@ -6,6 +6,7 @@ import ( "github.com/openziti/zrok/v2/controller/store" "github.com/openziti/zrok/v2/rest_model_zrok" "github.com/openziti/zrok/v2/rest_server_zrok/operations/admin" + "github.com/openziti/zrok/v2/sdk/golang/sdk" ) type applyLimitClassesHandler struct{} @@ -33,13 +34,49 @@ func (h *applyLimitClassesHandler) Handle(params admin.ApplyLimitClassesParams, return admin.NewApplyLimitClassesNotFound() } + existingLcs, err := str.FindAppliedLimitClassesForAccount(acct.Id, trx) + if err != nil { + dl.Errorf("error finding applied limit classes for '%v': %v", params.Body.Email, err) + return admin.NewApplyLimitClassesInternalServerError() + } + + seenIds := make(map[int]bool) + for _, lc := range existingLcs { + seenIds[lc.Id] = true + } + + occupiedSlots := newLimitClassSlots() + for _, lc := range existingLcs { + if conflict := occupiedSlots.add(lc); conflict { + dl.Errorf("existing applied limit classes for '%v' are already in a conflicting state", params.Body.Email) + return admin.NewApplyLimitClassesInternalServerError() + } + } + + var toApply []*store.AppliedLimitClass for _, lcId := range params.Body.LimitClassIds { - if _, err := str.GetLimitClass(int(lcId), trx); err != nil { + if seenIds[int(lcId)] { + continue + } + + lc, err := str.GetLimitClass(int(lcId), trx) + if err != nil { dl.Errorf("error finding limit class '%v': %v", lcId, err) return admin.NewApplyLimitClassesNotFound() } - if _, err := str.ApplyLimitClass(&store.AppliedLimitClass{AccountId: acct.Id, LimitClassId: int(lcId)}, trx); err != nil { - dl.Errorf("error applying limit class '%v' to '%v': %v", lcId, params.Body.Email, err) + + if conflict := occupiedSlots.add(lc); conflict { + dl.Errorf("applying limit class '%v' to '%v' would create conflicting effective limits", lcId, params.Body.Email) + return admin.NewApplyLimitClassesInternalServerError() + } + + seenIds[lc.Id] = true + toApply = append(toApply, &store.AppliedLimitClass{AccountId: acct.Id, LimitClassId: lc.Id}) + } + + for _, applied := range toApply { + if _, err := str.ApplyLimitClass(applied, trx); err != nil { + dl.Errorf("error applying limit class '%v' to '%v': %v", applied.LimitClassId, params.Body.Email, err) return admin.NewApplyLimitClassesInternalServerError() } } @@ -51,3 +88,55 @@ func (h *applyLimitClassesHandler) Handle(params admin.ApplyLimitClassesParams, return admin.NewApplyLimitClassesOK() } + +type limitClassSlots struct { + resource *store.LimitClass + bwWarning *store.LimitClass + bwLimit *store.LimitClass + scopes map[sdk.BackendMode]*store.LimitClass +} + +func newLimitClassSlots() *limitClassSlots { + return &limitClassSlots{ + scopes: make(map[sdk.BackendMode]*store.LimitClass), + } +} + +func (slots *limitClassSlots) add(lc *store.LimitClass) bool { + if lc == nil { + return false + } + + if lc.IsResourceCountClass() { + if slots.resource != nil && slots.resource.Id != lc.Id { + return true + } + slots.resource = lc + return false + } + + if lc.IsUnscopedBandwidthClass() { + if lc.LimitAction == store.WarningLimitAction { + if slots.bwWarning != nil && slots.bwWarning.Id != lc.Id { + return true + } + slots.bwWarning = lc + return false + } + if slots.bwLimit != nil && slots.bwLimit.Id != lc.Id { + return true + } + slots.bwLimit = lc + return false + } + + if lc.IsScopedBandwidthClass() { + backendMode := *lc.BackendMode + if existing, found := slots.scopes[backendMode]; found && existing.Id != lc.Id { + return true + } + slots.scopes[backendMode] = lc + } + + return false +} diff --git a/controller/limits/userLimits.go b/controller/limits/userLimits.go index 4e367feb..36e455c5 100644 --- a/controller/limits/userLimits.go +++ b/controller/limits/userLimits.go @@ -56,15 +56,15 @@ func (a *Agent) getUserLimits(acctId int, trx *sqlx.Tx) (*userLimits, error) { return nil, errors.Wrapf(err, "error finding applied limit classes for account '%d'", acctId) } for _, alc := range alcs { - if a.isResourceCountClass(alc) { + if alc.IsResourceCountClass() { resource = alc - } else if a.isUnscopedBandwidthClass(alc) { + } else if alc.IsUnscopedBandwidthClass() { if alc.LimitAction == store.WarningLimitAction { bwWarning = alc } else { bwLimit = alc } - } else if a.isScopedLimitClass(alc) { + } else if alc.IsScopedBandwidthClass() { scopes[*alc.BackendMode] = alc } else { dl.Warnf("unknown type of limit class '%v' for account '#%d'", alc, acctId) @@ -79,45 +79,3 @@ func (a *Agent) getUserLimits(acctId int, trx *sqlx.Tx) (*userLimits, error) { return userLimits, nil } - -func (a *Agent) isResourceCountClass(alc *store.LimitClass) bool { - if alc.BackendMode != nil { - return false - } - if alc.Environments == store.Unlimited && alc.Shares == store.Unlimited && alc.ReservedShares == store.Unlimited && alc.UniqueNames == store.Unlimited && alc.ShareFrontends == store.Unlimited { - return false - } - return true -} - -func (a *Agent) isUnscopedBandwidthClass(alc *store.LimitClass) bool { - if alc.BackendMode != nil { - return false - } - if alc.Environments > store.Unlimited || alc.Shares > store.Unlimited || alc.ReservedShares > store.Unlimited || alc.UniqueNames > store.Unlimited || alc.ShareFrontends > store.Unlimited { - return false - } - if alc.PeriodMinutes < 1 { - return false - } - if alc.RxBytes == store.Unlimited && alc.TxBytes == store.Unlimited && alc.TotalBytes == store.Unlimited { - return false - } - return true -} - -func (a *Agent) isScopedLimitClass(alc *store.LimitClass) bool { - if alc.BackendMode == nil { - return false - } - if alc.Environments > store.Unlimited { - return false - } - if alc.PeriodMinutes < 1 { - return false - } - if alc.RxBytes == store.Unlimited && alc.TxBytes == store.Unlimited && alc.TotalBytes == store.Unlimited { - return false - } - return true -} diff --git a/controller/limits/userLimits_simple_test.go b/controller/limits/userLimits_simple_test.go index 5cf356d3..c352cc04 100644 --- a/controller/limits/userLimits_simple_test.go +++ b/controller/limits/userLimits_simple_test.go @@ -72,24 +72,20 @@ func TestUserLimits_ToBandwidthArraySimple(t *testing.T) { } } -func TestAgent_ClassificationMethods(t *testing.T) { - a := &Agent{ - cfg: DefaultConfig(), - } - +func TestLimitClass_ClassificationMethods(t *testing.T) { t.Run("isResourceCountClass", func(t *testing.T) { validRC := &store.LimitClass{ Model: store.Model{Id: 1}, Environments: 10, } - assert.True(t, a.isResourceCountClass(validRC)) + assert.True(t, validRC.IsResourceCountClass()) proxyMode := sdk.ProxyBackendMode invalidRC := &store.LimitClass{ Model: store.Model{Id: 2}, BackendMode: &proxyMode, } - assert.False(t, a.isResourceCountClass(invalidRC)) + assert.False(t, invalidRC.IsResourceCountClass()) }) t.Run("isUnscopedBandwidthClass", func(t *testing.T) { @@ -103,7 +99,7 @@ func TestAgent_ClassificationMethods(t *testing.T) { UniqueNames: store.Unlimited, ShareFrontends: store.Unlimited, } - assert.True(t, a.isUnscopedBandwidthClass(validBwc)) + assert.True(t, validBwc.IsUnscopedBandwidthClass()) proxyMode := sdk.ProxyBackendMode invalidBwc := &store.LimitClass{ @@ -112,7 +108,7 @@ func TestAgent_ClassificationMethods(t *testing.T) { PeriodMinutes: 60, RxBytes: 1024, } - assert.False(t, a.isUnscopedBandwidthClass(invalidBwc)) + assert.False(t, invalidBwc.IsUnscopedBandwidthClass()) }) t.Run("isScopedLimitClass", func(t *testing.T) { @@ -128,18 +124,18 @@ func TestAgent_ClassificationMethods(t *testing.T) { UniqueNames: store.Unlimited, ShareFrontends: store.Unlimited, } - assert.True(t, a.isScopedLimitClass(validSLC)) + assert.True(t, validSLC.IsScopedBandwidthClass()) invalidSLC := &store.LimitClass{ Model: store.Model{Id: 2}, PeriodMinutes: 60, RxBytes: 1024, } - assert.False(t, a.isScopedLimitClass(invalidSLC)) + assert.False(t, invalidSLC.IsScopedBandwidthClass()) }) } // helper function to get string pointer func strPtr(s string) *string { return &s -} \ No newline at end of file +} diff --git a/controller/skipInterstitialGrant.go b/controller/skipInterstitialGrant.go index b267f1a9..5a30c023 100644 --- a/controller/skipInterstitialGrant.go +++ b/controller/skipInterstitialGrant.go @@ -81,13 +81,13 @@ func (h *grantSkipInterstitialHandler) Handle(params admin.GrantSkipInterstitial return admin.NewGrantSkipInterstitialInternalServerError() } - if err := trx.Commit(); err != nil { - dl.Errorf("error committing transaction: %v", err) + if err := syncSkipInterstitialForAccount(acct, true); err != nil { + dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err) return admin.NewGrantSkipInterstitialInternalServerError() } - if err := syncSkipInterstitialForAccount(acct, true); err != nil { - dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err) + if err := trx.Commit(); err != nil { + dl.Errorf("error committing transaction: %v", err) return admin.NewGrantSkipInterstitialInternalServerError() } @@ -126,21 +126,23 @@ func (h *revokeSkipInterstitialHandler) Handle(params admin.RevokeSkipInterstiti return admin.NewRevokeSkipInterstitialInternalServerError() } - if err := trx.Commit(); err != nil { - dl.Errorf("error committing transaction: %v", err) + if err := syncSkipInterstitialForAccount(acct, false); err != nil { + dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err) return admin.NewRevokeSkipInterstitialInternalServerError() } - if err := syncSkipInterstitialForAccount(acct, false); err != nil { - dl.Errorf("error syncing skip interstitial for '%v': %v", params.Body.Email, err) + if err := trx.Commit(); err != nil { + dl.Errorf("error committing transaction: %v", err) return admin.NewRevokeSkipInterstitialInternalServerError() } return admin.NewRevokeSkipInterstitialOK() } -// syncSkipInterstitialForAccount synchronizes the interstitial setting on all -// public (non-drive) share Ziti configs for the given account. +// syncSkipInterstitialForAccount best-effort synchronizes the interstitial +// setting on existing public (non-drive) share Ziti configs for the given +// account. Failures while processing individual shares are logged and skipped, +// since affected shares can be recreated to correct transient issues. func syncSkipInterstitialForAccount(acct *store.Account, skipInterstitial bool) error { trx, err := str.Begin() if err != nil { diff --git a/controller/store/limitClass.go b/controller/store/limitClass.go index c89e73f6..60a3b698 100644 --- a/controller/store/limitClass.go +++ b/controller/store/limitClass.go @@ -111,6 +111,48 @@ func (lc LimitClass) GetLimitAction() LimitAction { return lc.LimitAction } +func (lc LimitClass) IsResourceCountClass() bool { + if lc.BackendMode != nil { + return false + } + if lc.Environments == Unlimited && lc.Shares == Unlimited && lc.ReservedShares == Unlimited && lc.UniqueNames == Unlimited && lc.ShareFrontends == Unlimited { + return false + } + return true +} + +func (lc LimitClass) IsUnscopedBandwidthClass() bool { + if lc.BackendMode != nil { + return false + } + if lc.Environments > Unlimited || lc.Shares > Unlimited || lc.ReservedShares > Unlimited || lc.UniqueNames > Unlimited || lc.ShareFrontends > Unlimited { + return false + } + if lc.PeriodMinutes < 1 { + return false + } + if lc.RxBytes == Unlimited && lc.TxBytes == Unlimited && lc.TotalBytes == Unlimited { + return false + } + return true +} + +func (lc LimitClass) IsScopedBandwidthClass() bool { + if lc.BackendMode == nil { + return false + } + if lc.Environments > Unlimited { + return false + } + if lc.PeriodMinutes < 1 { + return false + } + if lc.RxBytes == Unlimited && lc.TxBytes == Unlimited && lc.TotalBytes == Unlimited { + return false + } + return true +} + func (lc LimitClass) String() string { out := "LimitClass<" if lc.Label != nil && *lc.Label != "" { diff --git a/controller/store/skipInterstitialGrant.go b/controller/store/skipInterstitialGrant.go index 647088eb..d84f00cc 100644 --- a/controller/store/skipInterstitialGrant.go +++ b/controller/store/skipInterstitialGrant.go @@ -18,6 +18,14 @@ func (str *Store) IsAccountGrantedSkipInterstitial(acctId int, trx *sqlx.Tx) (bo } func (str *Store) GrantSkipInterstitial(acctId int, trx *sqlx.Tx) error { + granted, err := str.IsAccountGrantedSkipInterstitial(acctId, trx) + if err != nil { + return err + } + if granted { + return nil + } + stmt, err := trx.Prepare("insert into skip_interstitial_grants (account_id) values ($1)") if err != nil { return errors.Wrap(err, "error preparing skip_interstitial_grants insert statement")