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.

Sign up for GitHub

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

Jump to bottom

refactor: error processing for xPolicy #3302

Merged
merged 8 commits into from
May 23, 2024
Merged

refactor: error processing for xPolicy #3302

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9a75549
add append '.' to err message for xpolicy
shawnh2 Apr 29, 2024
8092fe6
refactor xpolicy err message
shawnh2 Apr 29, 2024
666ed47
Merge branch 'main' into xpolicy-err
zirain Apr 30, 2024
b62fba8
Merge branch 'main' into xpolicy-err
shawnh2 May 6, 2024
17929c5
resolve conflicts
shawnh2 May 11, 2024
2bdf980
refactor error processing logic for EEP & CTP
shawnh2 May 11, 2024
1a280ee
resolve conflicts
shawnh2 May 23, 2024
f4b0742
fix test and add refactor error processing login
shawnh2 May 23, 2024
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
176 changes: 102 additions & 74 deletions internal/gatewayapi/backendtrafficpolicy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,15 @@
package gatewayapi

import (
"errors"
"fmt"
"math"
"net"
"sort"
"strings"
"time"

"github.com/pkg/errors"
perr "github.com/pkg/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
Expand Down Expand Up @@ -286,22 +287,23 @@

func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.BackendTrafficPolicy, route RouteContext, xdsIR XdsIRMap) error {
var (
rl *ir.RateLimit
lb *ir.LoadBalancer
pp *ir.ProxyProtocol
hc *ir.HealthCheck
cb *ir.CircuitBreaker
fi *ir.FaultInjection
to *ir.Timeout
ka *ir.TCPKeepalive
rt *ir.Retry
err error
rl *ir.RateLimit
lb *ir.LoadBalancer
pp *ir.ProxyProtocol
hc *ir.HealthCheck
cb *ir.CircuitBreaker
fi *ir.FaultInjection
to *ir.Timeout
ka *ir.TCPKeepalive
rt *ir.Retry
err, errs error
)

// Build IR
if policy.Spec.RateLimit != nil {
if rl, err = t.buildRateLimit(policy); err != nil {
return errors.Wrap(err, "RateLimit")
err = perr.WithMessage(err, "RateLimit")
errs = errors.Join(errs, err)

Check warning on line 306 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L305-L306 

Added lines #L305 - L306 were not covered by tests
}
}
if policy.Spec.LoadBalancer != nil {
Expand All @@ -315,30 +317,37 @@
}
if policy.Spec.CircuitBreaker != nil {
if cb, err = t.buildCircuitBreaker(policy); err != nil {
return errors.Wrap(err, "CircuitBreaker")
err = perr.WithMessage(err, "CircuitBreaker")
errs = errors.Join(errs, err)
}
}

if policy.Spec.FaultInjection != nil {
fi = t.buildFaultInjection(policy)
}
if policy.Spec.TCPKeepalive != nil {
if ka, err = t.buildTCPKeepAlive(policy); err != nil {
return errors.Wrap(err, "TCPKeepalive")
err = perr.WithMessage(err, "TCPKeepalive")
errs = errors.Join(errs, err)

Check warning on line 330 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L329-L330 

Added lines #L329 - L330 were not covered by tests
}
}
if policy.Spec.Retry != nil {
rt = t.buildRetry(policy)
}
// Apply IR to all relevant routes
prefix := irRoutePrefix(route)

if policy.Spec.Timeout != nil {
if to, err = t.buildTimeout(policy, nil); err != nil {
return errors.Wrap(err, "Timeout")
err = perr.WithMessage(err, "Timeout")
errs = errors.Join(errs, err)
}
}

// Early return if got any errors
if errs != nil {
return errs
}

// Apply IR to all relevant routes
prefix := irRoutePrefix(route)

for _, x := range xdsIR {
for _, tcp := range x.TCP {
for _, r := range tcp.Routes {
Expand Down Expand Up @@ -384,10 +393,9 @@

// Some timeout setting originate from the route.
if policy.Spec.Timeout != nil {
if to, err = t.buildTimeout(policy, r); err != nil {
return errors.Wrap(err, "Timeout")
if to, err = t.buildTimeout(policy, r); err == nil {
r.Traffic.Timeout = to
}
r.Traffic.Timeout = to
}

if policy.Spec.UseClientProtocol != nil {
Expand All @@ -403,22 +411,23 @@

func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.BackendTrafficPolicy, gateway *GatewayContext, xdsIR XdsIRMap) error {
var (
rl *ir.RateLimit
lb *ir.LoadBalancer
pp *ir.ProxyProtocol
hc *ir.HealthCheck
cb *ir.CircuitBreaker
fi *ir.FaultInjection
ct *ir.Timeout
ka *ir.TCPKeepalive
rt *ir.Retry
err error
rl *ir.RateLimit
lb *ir.LoadBalancer
pp *ir.ProxyProtocol
hc *ir.HealthCheck
cb *ir.CircuitBreaker
fi *ir.FaultInjection
ct *ir.Timeout
ka *ir.TCPKeepalive
rt *ir.Retry
err, errs error
)

// Build IR
if policy.Spec.RateLimit != nil {
if rl, err = t.buildRateLimit(policy); err != nil {
return errors.Wrap(err, "RateLimit")
err = perr.WithMessage(err, "RateLimit")
errs = errors.Join(errs, err)
}
}
if policy.Spec.LoadBalancer != nil {
Expand All @@ -432,20 +441,33 @@
}
if policy.Spec.CircuitBreaker != nil {
if cb, err = t.buildCircuitBreaker(policy); err != nil {
return errors.Wrap(err, "CircuitBreaker")
err = perr.WithMessage(err, "CircuitBreaker")
errs = errors.Join(errs, err)
}
}
if policy.Spec.FaultInjection != nil {
fi = t.buildFaultInjection(policy)
}
if policy.Spec.TCPKeepalive != nil {
if ka, err = t.buildTCPKeepAlive(policy); err != nil {
return errors.Wrap(err, "TCPKeepalive")
err = perr.WithMessage(err, "TCPKeepalive")
errs = errors.Join(errs, err)

Check warning on line 454 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L453-L454 

Added lines #L453 - L454 were not covered by tests
}
}
if policy.Spec.Retry != nil {
rt = t.buildRetry(policy)
}
if policy.Spec.Timeout != nil {
if ct, err = t.buildTimeout(policy, nil); err != nil {
err = perr.WithMessage(err, "Timeout")
errs = errors.Join(errs, err)
}
}

// Early return if got any errors
if errs != nil {
return errs
}

// Apply IR to all the routes within the specific Gateway
// If the feature is already set, then skip it, since it must be have
Expand All @@ -456,12 +478,6 @@

policyTarget := irStringKey(policy.Namespace, string(policy.Spec.TargetRef.Name))

if policy.Spec.Timeout != nil {
if ct, err = t.buildTimeout(policy, nil); err != nil {
return errors.Wrap(err, "Timeout")
}
}

for _, tcp := range x.TCP {
gatewayName := tcp.Name[0:strings.LastIndex(tcp.Name, "/")]
if t.MergeGateways && gatewayName != policyTarget {
Expand Down Expand Up @@ -541,10 +557,9 @@
r.Traffic.HealthCheck.SetHTTPHostIfAbsent(r.Hostname)

if policy.Spec.Timeout != nil {
if ct, err = t.buildTimeout(policy, r); err != nil {
return errors.Wrap(err, "Timeout")
if ct, err = t.buildTimeout(policy, r); err == nil {
r.Traffic.Timeout = ct
}
r.Traffic.Timeout = ct
}

if policy.Spec.UseClientProtocol != nil {
Expand Down Expand Up @@ -1009,20 +1024,23 @@

func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTTPRoute) (*ir.Timeout, error) {
var (
tto *ir.TCPTimeout
hto *ir.HTTPTimeout
tto *ir.TCPTimeout
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this part is reverted from https://github.com/envoyproxy/gateway/pull/2846/files#diff-ce86bc9474dbc964bb6180e6a110b708dd85081193dca54a2fa7cda70d742247L930, since BTP has supported dealing with multi-errors now

hto *ir.HTTPTimeout
terr bool
errs error
)

pto := policy.Spec.Timeout

if pto.TCP != nil && pto.TCP.ConnectTimeout != nil {
d, err := time.ParseDuration(string(*pto.TCP.ConnectTimeout))
if err != nil {
return nil, fmt.Errorf("invalid ConnectTimeout value %s", *pto.TCP.ConnectTimeout)
}

tto = &ir.TCPTimeout{
ConnectTimeout: ptr.To(metav1.Duration{Duration: d}),
terr = true
errs = errors.Join(errs, fmt.Errorf("invalid ConnectTimeout value %s", *pto.TCP.ConnectTimeout))
} else {
tto = &ir.TCPTimeout{
ConnectTimeout: ptr.To(metav1.Duration{Duration: d}),
}
}
}

Expand All @@ -1033,19 +1051,21 @@
if pto.HTTP.ConnectionIdleTimeout != nil {
d, err := time.ParseDuration(string(*pto.HTTP.ConnectionIdleTimeout))
if err != nil {
return nil, fmt.Errorf("invalid ConnectionIdleTimeout value %s", *pto.HTTP.ConnectionIdleTimeout)
terr = true
errs = errors.Join(errs, fmt.Errorf("invalid ConnectionIdleTimeout value %s", *pto.HTTP.ConnectionIdleTimeout))

Check warning on line 1055 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L1054-L1055 

Added lines #L1054 - L1055 were not covered by tests
} else {
cit = ptr.To(metav1.Duration{Duration: d})
}

cit = ptr.To(metav1.Duration{Duration: d})
}

if pto.HTTP.MaxConnectionDuration != nil {
d, err := time.ParseDuration(string(*pto.HTTP.MaxConnectionDuration))
if err != nil {
return nil, fmt.Errorf("invalid MaxConnectionDuration value %s", *pto.HTTP.MaxConnectionDuration)
terr = true
errs = errors.Join(errs, fmt.Errorf("invalid MaxConnectionDuration value %s", *pto.HTTP.MaxConnectionDuration))
} else {
mcd = ptr.To(metav1.Duration{Duration: d})
}

mcd = ptr.To(metav1.Duration{Duration: d})
}

hto = &ir.HTTPTimeout{
Expand All @@ -1056,28 +1076,36 @@

// http request timeout is translated during the gateway-api route resource translation
// merge route timeout setting with backendtrafficpolicy timeout settings
if r != nil &&
r.Traffic != nil &&
r.Traffic.Timeout != nil &&
r.Traffic.Timeout.HTTP != nil &&
r.Traffic.Timeout.HTTP.RequestTimeout != nil {
if hto == nil {
hto = &ir.HTTPTimeout{
RequestTimeout: r.Traffic.Timeout.HTTP.RequestTimeout,
if terr {
if r != nil && r.Traffic != nil && r.Traffic.Timeout != nil {
return r.Traffic.Timeout.DeepCopy(), errs

Check warning on line 1081 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L1081 

Added line #L1081 was not covered by tests
}
} else {
// http request timeout is translated during the gateway-api route resource translation
// merge route timeout setting with backendtrafficpolicy timeout settings
if r != nil &&
r.Traffic != nil &&
r.Traffic.Timeout != nil &&
r.Traffic.Timeout.HTTP != nil &&
r.Traffic.Timeout.HTTP.RequestTimeout != nil {
if hto == nil {
hto = &ir.HTTPTimeout{
RequestTimeout: r.Traffic.Timeout.HTTP.RequestTimeout,

Check warning on line 1093 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L1091-L1093 

Added lines #L1091 - L1093 were not covered by tests
}
} else {
hto.RequestTimeout = r.Traffic.Timeout.HTTP.RequestTimeout

Check warning on line 1096 in internal/gatewayapi/backendtrafficpolicy.go

View check run for this annotation

Codecov / codecov/patch

internal/gatewayapi/backendtrafficpolicy.go#L1095-L1096 

Added lines #L1095 - L1096 were not covered by tests
}
} else {
hto.RequestTimeout = r.Traffic.Timeout.HTTP.RequestTimeout
}
}

if hto != nil || tto != nil {
return &ir.Timeout{
TCP: tto,
HTTP: hto,
}, nil
if hto != nil || tto != nil {
return &ir.Timeout{
TCP: tto,
HTTP: hto,
}, nil
}
}

return nil, nil
return nil, errs
}

func int64ToUint32(in int64) (uint32, bool) {
Expand Down
Loading
Loading