From 8092fe6029929bbb78bf175d918ce9ee9f9bbd5b Mon Sep 17 00:00:00 2001 From: shawnh2 Date: Mon, 29 Apr 2024 16:48:13 +0800 Subject: [PATCH] refactor xpolicy err message Signed-off-by: shawnh2 --- internal/gatewayapi/backendtrafficpolicy.go | 88 +++++++++++-------- internal/gatewayapi/securitypolicy.go | 9 +- ...-extauth-invalid-no-matching-port.out.yaml | 2 +- ...licy-with-extauth-invalid-no-port.out.yaml | 4 +- ...xtauth-invalid-no-reference-grant.out.yaml | 4 +- ...y-with-extauth-invalid-no-service.out.yaml | 2 +- ...ypolicy-with-jwt-and-invalid-oidc.out.yaml | 4 +- ...typolicy-with-oidc-invalid-issuer.out.yaml | 4 +- ...olicy-with-oidc-invalid-secretref.out.yaml | 6 +- 9 files changed, 72 insertions(+), 51 deletions(-) diff --git a/internal/gatewayapi/backendtrafficpolicy.go b/internal/gatewayapi/backendtrafficpolicy.go index 55f79723be3..c6c1c3757f7 100644 --- a/internal/gatewayapi/backendtrafficpolicy.go +++ b/internal/gatewayapi/backendtrafficpolicy.go @@ -6,6 +6,7 @@ package gatewayapi import ( + "errors" "fmt" "math" "net" @@ -13,7 +14,7 @@ import ( "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" @@ -310,7 +311,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 { @@ -324,7 +325,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") } } @@ -333,7 +334,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 { @@ -344,7 +345,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") } } @@ -385,7 +386,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 } @@ -418,7 +419,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 { @@ -432,7 +433,7 @@ 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 { @@ -440,7 +441,7 @@ func (t *Translator) translateBackendTrafficPolicyForGateway(policy *egv1a1.Back } 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 { @@ -461,7 +462,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") } } @@ -557,7 +558,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 { @@ -1014,8 +1015,10 @@ 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 + hto *ir.HTTPTimeout + terr bool + errs error ) pto := policy.Spec.Timeout @@ -1023,11 +1026,12 @@ func (t *Translator) buildTimeout(policy *egv1a1.BackendTrafficPolicy, r *ir.HTT 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}), + } } } @@ -1038,19 +1042,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{ @@ -1061,24 +1067,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) { diff --git a/internal/gatewayapi/securitypolicy.go b/internal/gatewayapi/securitypolicy.go index f03e46590db..4f7be1837cb 100644 --- a/internal/gatewayapi/securitypolicy.go +++ b/internal/gatewayapi/securitypolicy.go @@ -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" @@ -356,6 +357,7 @@ func (t *Translator) translateSecurityPolicyForRoute( if oidc, err = t.buildOIDC( policy, resources); err != nil { + err = perr.WithMessage(err, "OIDC") errs = errors.Join(errs, err) } } @@ -364,6 +366,7 @@ func (t *Translator) translateSecurityPolicyForRoute( if basicAuth, err = t.buildBasicAuth( policy, resources); err != nil { + err = perr.WithMessage(err, "BasicAuth") errs = errors.Join(errs, err) } } @@ -372,6 +375,7 @@ func (t *Translator) translateSecurityPolicyForRoute( if extAuth, err = t.buildExtAuth( policy, resources); err != nil { + err = perr.WithMessage(err, "ExtAuth") errs = errors.Join(errs, err) } } @@ -427,6 +431,7 @@ func (t *Translator) translateSecurityPolicyForGateway( if oidc, err = t.buildOIDC( policy, resources); err != nil { + err = perr.WithMessage(err, "OIDC") errs = errors.Join(errs, err) } } @@ -435,6 +440,7 @@ func (t *Translator) translateSecurityPolicyForGateway( if basicAuth, err = t.buildBasicAuth( policy, resources); err != nil { + err = perr.WithMessage(err, "BasicAuth") errs = errors.Join(errs, err) } } @@ -443,6 +449,7 @@ func (t *Translator) translateSecurityPolicyForGateway( if extAuth, err = t.buildExtAuth( policy, resources); err != nil { + err = perr.WithMessage(err, "ExtAuth") errs = errors.Join(errs, err) } } @@ -591,7 +598,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. diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml index aabb54434f4..64b870b5e47 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-matching-port.out.yaml @@ -126,7 +126,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: TCP Port 80 not found on service default/http-backend. + message: 'ExtAuth: TCP Port 80 not found on service default/http-backend.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml index 1ef304673d8..67d42873f9b 100755 --- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-port.out.yaml @@ -125,8 +125,8 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: A valid port number corresponding to a port on the Service must be - specified. + message: 'ExtAuth: a valid port number corresponding to a port on the Service + must be specified.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml index 0dc44875c47..eecad291496 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-reference-grant.out.yaml @@ -126,8 +126,8 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: Backend ref to Service envoy-gateway/http-backend not permitted by - any ReferenceGrant. + message: 'ExtAuth: backend ref to Service envoy-gateway/http-backend not permitted + by any ReferenceGrant.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml index de2e3010da3..e5e3f1fc995 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-extauth-invalid-no-service.out.yaml @@ -126,7 +126,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: Service default/http-backend not found. + message: 'ExtAuth: service default/http-backend not found.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml index f2af5bd857a..2d3022a33bb 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-jwt-and-invalid-oidc.out.yaml @@ -174,7 +174,7 @@ securityPolicies: sectionName: http conditions: - lastTransitionTime: null - message: Secret default/client2-secret does not exist. + message: 'OIDC: secret default/client2-secret does not exist.' reason: Invalid status: "False" type: Accepted @@ -219,7 +219,7 @@ securityPolicies: namespace: envoy-gateway conditions: - lastTransitionTime: null - message: Secret envoy-gateway/client1-secret does not exist. + message: 'OIDC: secret envoy-gateway/client1-secret does not exist.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.out.yaml index 34cdbe793ff..1102e64d406 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-issuer.out.yaml @@ -85,8 +85,8 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: 'Error fetching endpoints from issuer: invalid character ''<'' looking - for beginning of value.' + message: 'OIDC: error fetching endpoints from issuer: invalid character ''<'' + looking for beginning of value.' reason: Invalid status: "False" type: Accepted diff --git a/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.out.yaml b/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.out.yaml index 18d499d849c..259908a1ad4 100644 --- a/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.out.yaml +++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc-invalid-secretref.out.yaml @@ -197,7 +197,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: Secret default/client1-secret does not exist. + message: 'OIDC: secret default/client1-secret does not exist.' reason: Invalid status: "False" type: Accepted @@ -234,7 +234,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: Secret ref namespace must be unspecified/empty or default. + message: 'OIDC: secret ref namespace must be unspecified/empty or default.' reason: Invalid status: "False" type: Accepted @@ -270,7 +270,7 @@ securityPolicies: namespace: default conditions: - lastTransitionTime: null - message: Client secret not found in secret default/client3-secret. + message: 'OIDC: client secret not found in secret default/client3-secret.' reason: Invalid status: "False" type: Accepted