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

refactor: error processing for xPolicy #3302

Merged
merged 8 commits into from
May 23, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 51 additions & 37 deletions internal/gatewayapi/backendtrafficpolicy.go
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 @@ -311,7 +312,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
// Build IR
if policy.Spec.RateLimit != nil {
if rl, err = t.buildRateLimit(policy); err != nil {
return errors.Wrap(err, "RateLimit")
return perr.WithMessage(err, "RateLimit")
}
}
if policy.Spec.LoadBalancer != nil {
Expand All @@ -325,7 +326,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
}
if policy.Spec.CircuitBreaker != nil {
if cb, err = t.buildCircuitBreaker(policy); err != nil {
return errors.Wrap(err, "CircuitBreaker")
return perr.WithMessage(err, "CircuitBreaker")
}
}

Expand All @@ -334,7 +335,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
}
if policy.Spec.TCPKeepalive != nil {
if ka, err = t.buildTCPKeepAlive(policy); err != nil {
return errors.Wrap(err, "TCPKeepalive")
return perr.WithMessage(err, "TCPKeepalive")
}
}
if policy.Spec.Retry != nil {
Expand All @@ -345,7 +346,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen

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

Expand Down Expand Up @@ -386,7 +387,7 @@ func (t *Translator) translateBackendTrafficPolicyForRoute(policy *egv1a1.Backen
// 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")
return perr.WithMessage(err, "Timeout")
}
r.Timeout = to
}
Expand Down Expand Up @@ -419,7 +420,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back
// Build IR
if policy.Spec.RateLimit != nil {
if rl, err = t.buildRateLimit(policy); err != nil {
return errors.Wrap(err, "RateLimit")
return perr.WithMessage(err, "RateLimit")
}
}
if policy.Spec.LoadBalancer != nil {
Expand All @@ -433,15 +434,15 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back
}
if policy.Spec.CircuitBreaker != nil {
if cb, err = t.buildCircuitBreaker(policy); err != nil {
return errors.Wrap(err, "CircuitBreaker")
return perr.WithMessage(err, "CircuitBreaker")
}
}
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")
return perr.WithMessage(err, "TCPKeepalive")
}
}
if policy.Spec.Retry != nil {
Expand All @@ -462,7 +463,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back

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

Expand Down Expand Up @@ -558,7 +559,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back

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

if r.Timeout == nil {
Expand Down Expand Up @@ -1015,20 +1016,23 @@ func (t *Translator) buildCircuitBreaker(policy *egv1a1.BackendTrafficPolicy) (*

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.

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 @@ -1039,19 +1043,21 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT
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))
} 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 @@ -1062,24 +1068,32 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT

// http request timeout is translated during the gateway-api route resource translation
// merge route timeout setting with backendtrafficpolicy timeout settings
if r != nil && r.Timeout != nil && r.Timeout.HTTP != nil && r.Timeout.HTTP.RequestTimeout != nil {
if hto == nil {
hto = &ir.HTTPTimeout{
RequestTimeout: r.Timeout.HTTP.RequestTimeout,
if terr {
if r != nil && r.Timeout != nil {
return r.Timeout.DeepCopy(), errs
}
} 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.Timeout != nil && r.Timeout.HTTP != nil && r.Timeout.HTTP.RequestTimeout != nil {
if hto == nil {
hto = &ir.HTTPTimeout{
RequestTimeout: r.Timeout.HTTP.RequestTimeout,
}
} else {
hto.RequestTimeout = r.Timeout.HTTP.RequestTimeout
}
} else {
hto.RequestTimeout = r.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
9 changes: 8 additions & 1 deletion internal/gatewayapi/securitypolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"strconv"
"strings"

perr "github.com/pkg/errors"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -360,6 +361,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
if oidc, err = t.buildOIDC(
policy,
resources); err != nil {
err = perr.WithMessage(err, "OIDC")
errs = errors.Join(errs, err)
}
}
Expand All @@ -368,6 +370,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
if basicAuth, err = t.buildBasicAuth(
policy,
resources); err != nil {
err = perr.WithMessage(err, "BasicAuth")
errs = errors.Join(errs, err)
}
}
Expand All @@ -376,6 +379,7 @@ func (t *Translator) translateSecurityPolicyForRoute(
if extAuth, err = t.buildExtAuth(
policy,
resources); err != nil {
err = perr.WithMessage(err, "ExtAuth")
errs = errors.Join(errs, err)
}
}
Expand Down Expand Up @@ -432,6 +436,7 @@ func (t *Translator) translateSecurityPolicyForGateway(
if oidc, err = t.buildOIDC(
policy,
resources); err != nil {
err = perr.WithMessage(err, "OIDC")
errs = errors.Join(errs, err)
}
}
Expand All @@ -440,6 +445,7 @@ func (t *Translator) translateSecurityPolicyForGateway(
if basicAuth, err = t.buildBasicAuth(
policy,
resources); err != nil {
err = perr.WithMessage(err, "BasicAuth")
errs = errors.Join(errs, err)
}
}
Expand All @@ -448,6 +454,7 @@ func (t *Translator) translateSecurityPolicyForGateway(
if extAuth, err = t.buildExtAuth(
policy,
resources); err != nil {
err = perr.WithMessage(err, "ExtAuth")
errs = errors.Join(errs, err)
}
}
Expand Down Expand Up @@ -597,7 +604,7 @@ func (t *Translator) buildOIDC(
// Generate a unique cookie suffix for oauth filters
suffix := utils.Digest32(string(policy.UID))

// Get the HMAC secret
// Get the HMAC secret.
// HMAC secret is generated by the CertGen job and stored in a secret
// We need to rotate the HMAC secret in the future, probably the same
// way we rotate the certs generated by the CertGen job.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ backendTLSPolicies:
sectionName: http
conditions:
- lastTransitionTime: null
message: No ca found in configmap no-ca-cmap
message: No ca found in configmap no-ca-cmap.
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ backendTLSPolicies:
conditions:
- lastTransitionTime: null
message: Target ref to Service backends/http-backend not permitted by any
ReferenceGrant
ReferenceGrant.
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ backendTrafficPolicies:
sectionName: http
conditions:
- lastTransitionTime: null
message: 'CircuitBreaker: invalid MaxRequestsPerConnection value -1'
message: 'CircuitBreaker: invalid MaxRequestsPerConnection value -1.'
reason: Invalid
status: "False"
type: Accepted
Expand Down Expand Up @@ -52,7 +52,7 @@ backendTrafficPolicies:
sectionName: http
conditions:
- lastTransitionTime: null
message: 'CircuitBreaker: invalid MaxParallelRetries value -1'
message: 'CircuitBreaker: invalid MaxParallelRetries value -1.'
reason: Invalid
status: "False"
type: Accepted
Expand Down Expand Up @@ -80,7 +80,7 @@ backendTrafficPolicies:
namespace: envoy-gateway
conditions:
- lastTransitionTime: null
message: 'CircuitBreaker: invalid MaxConnections value -1'
message: 'CircuitBreaker: invalid MaxConnections value -1.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ backendTrafficPolicies:
conditions:
- lastTransitionTime: null
message: 'RateLimit: local rateLimit rule limit unit must be a multiple of
the default limit unit'
the default limit unit.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ backendTrafficPolicies:
namespace: envoy-gateway
conditions:
- lastTransitionTime: null
message: 'RateLimit: local rateLimit does not support distinct HeaderMatch'
message: 'RateLimit: local rateLimit does not support distinct HeaderMatch.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ backendTrafficPolicies:
conditions:
- lastTransitionTime: null
message: 'RateLimit: local rateLimit can not have more than one rule without
clientSelectors'
clientSelectors.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ backendTrafficPolicies:
conditions:
- lastTransitionTime: null
message: 'RateLimit: regex "*.illegal.regex" is invalid: error parsing regexp:
missing argument to repetition operator: `*`'
missing argument to repetition operator: `*`.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ backendTrafficPolicies:
namespace: envoy-gateway
conditions:
- lastTransitionTime: null
message: 'Timeout: invalid MaxConnectionDuration value 22mib'
message: 'Timeout: invalid MaxConnectionDuration value 22mib.'
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ clientTrafficPolicies:
sectionName: http-1
conditions:
- lastTransitionTime: null
message: Invalid BufferLimit value 500m
message: Invalid BufferLimit value 500m.
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ clientTrafficPolicies:
sectionName: http-1
conditions:
- lastTransitionTime: null
message: BufferLimit value 100G is out of range, must be between 0 and 4294967295
message: BufferLimit value 100G is out of range, must be between 0 and 4294967295.
reason: Invalid
status: "False"
type: Accepted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ clientTrafficPolicies:
sectionName: http-1
conditions:
- lastTransitionTime: null
message: Invalid CloseDelay value 10mib
message: Invalid CloseDelay value 10mib.
reason: Invalid
status: "False"
type: Accepted
Expand Down