Skip to content

Commit

Permalink
Dispatcher improvements (istio#6034)
Browse files Browse the repository at this point in the history
- Fix bug around handling of quotas. Mixer wasn't observing the requested quota name and
getting quota from all defines instances. It now limits itself to the requested instances
and produces an error if the proxy requests quota from an unknown instance, instead of failing
open.

- Pass CheckResult, QuotaResult, and QuotaMethodArgs by value. This
is cleaner, avoids transient allocations, and eliminates some code paths.
  • Loading branch information
geeknoid authored and istio-testing committed Jun 7, 2018
1 parent e0664b1 commit 4f93a9d
Show file tree
Hide file tree
Showing 19 changed files with 250 additions and 128 deletions.
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ including Citadel (acting as Certificate Authority), node agent, etc.
contains platform-specific code to populate the
[abstract service model](https://istio.io/docs/concepts/traffic-management/overview.html), dynamically reconfigure the proxies
when the application topology changes, as well as translate
[routing rules](https://istio.io/docs/reference/config/traffic-rules/routing-rules.html) into proxy specific configuration.
[routing rules](https://istio.io/docs/reference/config/istio.networking.v1alpha3/) into proxy specific configuration.
- [istioctl](istioctl/). This directory contains code for the
[_istioctl_](https://istio.io/docs/reference/commands/istioctl.html) command line utility.
- [mixer](mixer/). This directory
Expand Down
2 changes: 1 addition & 1 deletion codecov.requirement
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ istio.io/istio/mixer/cmd/mixc/cmd:36 [38.1]
istio.io/istio/mixer/cmd/mixs:0 [0.0]
istio.io/istio/mixer/cmd/mixs/cmd:15 [17.6]
istio.io/istio/mixer/cmd/shared:0 [0.0]
istio.io/istio/mixer/pkg/adapter:93 [95.6]
istio.io/istio/mixer/pkg/adapter:100 [100.0]
istio.io/istio/mixer/pkg/adapter/test:0 [0.0]
istio.io/istio/mixer/pkg/api:100 [100.0]
istio.io/istio/mixer/pkg/attribute:100 [100.0]
Expand Down
1 change: 1 addition & 0 deletions codecov.skip
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
istio.io/istio/mixer/pkg/adapter/test
istio.io/istio/mixer/pkg/config/storetest
istio.io/istio/mixer/pkg/mockapi
istio.io/istio/mixer/pkg/perf
Expand Down
16 changes: 8 additions & 8 deletions mixer/adapter/redisquota/redisquota_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Request 30 when 50 is available": {
attrs: map[string]interface{}{},
quotas: map[string]istio_mixer_v1.CheckRequest_QuotaParams{
"key1": {
"requestCount": {
Amount: 30,
BestEffort: true,
},
Expand All @@ -105,7 +105,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Returns": [
{
"Quota": {
"key1": {
"requestCount": {
"ValidDuration": 30000000000,
"Amount": 30
}
Expand All @@ -118,7 +118,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Exceed allocation request with bestEffort": {
attrs: map[string]interface{}{},
quotas: map[string]istio_mixer_v1.CheckRequest_QuotaParams{
"key2": {
"requestCount": {
Amount: 60,
BestEffort: true,
},
Expand All @@ -128,7 +128,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Returns": [
{
"Quota": {
"key2": {
"requestCount": {
"ValidDuration": 30000000000,
"Amount": 50
}
Expand All @@ -141,7 +141,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Exceed allocation request without bestEffort": {
attrs: map[string]interface{}{},
quotas: map[string]istio_mixer_v1.CheckRequest_QuotaParams{
"key3": {
"requestCount": {
Amount: 60,
BestEffort: false,
},
Expand All @@ -151,7 +151,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Returns": [
{
"Quota": {
"key3": {
"requestCount": {
"ValidDuration": 0,
"Amount": 0
}
Expand All @@ -167,7 +167,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"destination.service": "ratings",
},
quotas: map[string]istio_mixer_v1.CheckRequest_QuotaParams{
"overridden": {
"requestCount": {
Amount: 15,
BestEffort: true,
},
Expand All @@ -178,7 +178,7 @@ func runServerWithSelectedAlgorithm(t *testing.T, algorithm string) {
"Returns": [
{
"Quota": {
"overridden": {
"requestCount": {
"ValidDuration": 30000000000,
"Amount": 12
}
Expand Down
5 changes: 5 additions & 0 deletions mixer/pkg/adapter/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ type CheckResult struct {
ValidUseCount int32
}

// IsDefault returns true if the CheckResult is in its zero state
func (r *CheckResult) IsDefault() bool {
return status.IsOK(r.Status) && r.ValidDuration == 0 && r.ValidUseCount == 0
}

func (r *CheckResult) String() string {
return fmt.Sprintf("CheckResult: status:%s, duration:%d, usecount:%d", status.String(r.Status), r.ValidDuration, r.ValidDuration)
}
48 changes: 48 additions & 0 deletions mixer/pkg/adapter/check_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
// Copyright 2017 Istio Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package adapter

import (
"testing"

"istio.io/istio/mixer/pkg/status"
)

func TestCheckResult(t *testing.T) {
cr := CheckResult{}
s := cr.String()
if s == "" {
t.Error("Expected valid string")
}

if !cr.IsDefault() {
t.Error("Expecting IsDefault to return true")
}

cr.Status = status.WithCancelled("FAIL")
if cr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}

cr = CheckResult{ValidUseCount: 1}
if cr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}

cr = CheckResult{ValidDuration: 10}
if cr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}
}
7 changes: 7 additions & 0 deletions mixer/pkg/adapter/quotas.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"time"

rpc "github.com/gogo/googleapis/google/rpc"

"istio.io/istio/mixer/pkg/status"
)

type (
Expand Down Expand Up @@ -50,3 +52,8 @@ type (
Amount int64
}
)

// IsDefault returns true if the QuotaResult is in its zero state
func (r *QuotaResult) IsDefault() bool {
return status.IsOK(r.Status) && r.ValidDuration == 0 && r.Amount == 0
}
44 changes: 44 additions & 0 deletions mixer/pkg/adapter/quotas_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
// Copyright 2017 Istio Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package adapter

import (
"testing"

"istio.io/istio/mixer/pkg/status"
)

func TestQuotaResult(t *testing.T) {
qr := QuotaResult{}

if !qr.IsDefault() {
t.Error("Expecting IsDefault to return true")
}

qr.Status = status.WithCancelled("FAIL")
if qr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}

qr = QuotaResult{Amount: 1}
if qr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}

qr = QuotaResult{ValidDuration: 10}
if qr.IsDefault() {
t.Error("Expecting IsDefault to return false")
}
}
24 changes: 2 additions & 22 deletions mixer/pkg/api/grpcServer.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package api

import (
"fmt"
"time"

multierror "github.com/hashicorp/go-multierror"
opentracing "github.com/opentracing/opentracing-go"
Expand Down Expand Up @@ -51,13 +50,6 @@ type (
}
)

const (
// defaultValidDuration is the default duration for which a check or quota result is valid.
defaultValidDuration = 10 * time.Second
// defaultValidUseCount is the default number of calls for which a check or quota result is valid.
defaultValidUseCount = 200
)

var lg = log.RegisterScope("api", "API dispatcher messages.", 0)

// NewGRPCServer creates a gRPC serving stack.
Expand Down Expand Up @@ -121,13 +113,6 @@ func (s *grpcServer) check(legacyCtx legacyContext.Context, req *mixerpb.CheckRe
return nil, grpc.Errorf(codes.Internal, err.Error())
}

if cr == nil {
cr = &adapter.CheckResult{
ValidDuration: defaultValidDuration,
ValidUseCount: defaultValidUseCount,
}
}

if status.IsOK(cr.Status) {
lg.Debug("Check approved")
} else {
Expand All @@ -147,7 +132,7 @@ func (s *grpcServer) check(legacyCtx legacyContext.Context, req *mixerpb.CheckRe
resp.Quotas = make(map[string]mixerpb.CheckResponse_QuotaResult, len(req.Quotas))

for name, param := range req.Quotas {
qma := &dispatcher.QuotaMethodArgs{
qma := dispatcher.QuotaMethodArgs{
Quota: name,
Amount: param.Amount,
DeduplicationID: req.DeduplicationId + name,
Expand All @@ -161,17 +146,12 @@ func (s *grpcServer) check(legacyCtx legacyContext.Context, req *mixerpb.CheckRe

crqr := mixerpb.CheckResponse_QuotaResult{}

var qr *adapter.QuotaResult
var qr adapter.QuotaResult
qr, err = s.dispatcher.Quota(legacyCtx, checkBag, qma)
if err != nil {
err = fmt.Errorf("performing quota alloc failed: %v", err)
lg.Errora("Quota failure:", err.Error())
// we continue the quota loop even after this error
} else if qr == nil {
// If qma.Quota does not apply to this request give the client what it asked for.
// Effectively the quota is unlimited.
crqr.ValidDuration = defaultValidDuration
crqr.GrantedAmount = qma.Amount
} else {
if !status.IsOK(qr.Status) {
lg.Debugf("Quota denied: %v", qr.Status)
Expand Down
Loading

0 comments on commit 4f93a9d

Please sign in to comment.