Handle missing TCP upstreams as warnings #8461

ashishb-solo · 2023-07-12T15:14:26Z

Description

Prior to the fix for #8405, errors for TCP listeners were not being reported. Upon fixing this, gloo pods began reporting validation errors when deleting upstreams if TCP gateways still depended on them (see for example: https://github.com/solo-io/solo-projects/issues/5156). In HTTP gateways, we typically handle these as warnings rather than errors (due to user patterns). This change treats TCP gateways the same as HTTP gateways, and changes reporting of missing upstreams from errors to warnings.

Context

This was causing test flakes, and also may have been impacting users who were reporting issues deleting TCP gateways.

Checklist:

I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/main/changelogutils) which references the issue that is resolved.
If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
I followed guidelines laid out in the Gloo Edge contribution guide
I opened a draft PR or added the work in progress label if my PR is not ready for review
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves https://github.com/solo-io/solo-projects/issues/5163

github-actions · 2023-07-12T15:43:42Z

Visit the preview URL for this PR (updated for commit 11683c4):

https://gloo-edge--pr8461-5163-missing-tcp-ups-yfjo5roc.web.app

_{(expires Fri, 21 Jul 2023 15:06:35 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: 77c2b86e287749579b7ff9cadb81e099042ef677}

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

changelog/v1.15.0-beta18/handle-missing-tcp-upstreams.yaml

projects/gloo/pkg/utils/validation/proxy_validation.go

projects/gloo/pkg/plugins/tcp/plugin.go

projects/gloo/pkg/utils/vhosts.go

solo-changelog-bot · 2023-07-12T15:56:11Z

Issues linked to changelog:
https://github.com/solo-io/solo-projects/issues/5163

Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

projects/gloo/api/grpc/validation/gloo_validation.proto

jenshu · 2023-07-12T17:17:51Z

projects/gloo/pkg/utils/validation/proxy_validation.go

+func (invHostErr *TcpHostError) Error() string {
+	var errStr string
+	if invHostErr.Err != nil {
+		invHostErr.Err.Error()


Suggested change

invHostErr.Err.Error()

return invHostErr.Err.Error()

i think this is made obsolete by the changes in this comment: https://github.com/solo-io/gloo/pull/8461/files/3a5d40209a80d62a16170bd2817ab85f2992439c#r1261386540

jenshu · 2023-07-12T17:26:30Z

projects/gloo/pkg/utils/validation/proxy_validation.go

@@ -16,6 +16,21 @@ var (
 	RouteIdentifierTxt = "Route Name"
 )

+// TcpHostError reports an error and the host which is associated with the
+// error
+type TcpHostError struct {


if this is always treated as a warning, should we rename it TcpHostWarning?

this has been done here: c69486e#diff-d8eb0189e46d1e52cd986c9ca9442663af1ea2287d7757d6483fd4926ef6f16dR35

jenshu · 2023-07-12T17:28:57Z

projects/gloo/pkg/translator/filter_chain.go

-				validationapi.TcpListenerReport_Error_ProcessingError,
-				fmt.Sprintf("listener %s: %s", t.parentListener.GetName(), err.Error()))
+			// unwrap the multierror object returned by CreateTcpFilterChains;
+			// treat DestinationNotFoundError as a warning and all others as


it looks like we're not actually checking for DestinationNotFoundError below, we're just converting all TcpHostErrors into InvalidDestinationWarnings. if we add more warning/error types in the future, this logic may not work anymore

i'm not 100% sure i understood what you're calling out here, but i did add some more rigorous checks in this commit: 42c951e

namely i made sure that the error type was a warning before using the AppendTcpHostWarning function call, and otherwise used the AppendTcpListenerError function. i think this resolves your concern, but if not, do let me know

jenshu · 2023-07-12T17:29:16Z

projects/gloo/pkg/translator/filter_chain.go

+					}
+				}
+			} else {
+				validation.AppendTCPListenerError(t.report,


if it's not a multierror, do we still need to check for type?

added that as part of this commit: 42c951e

Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com>

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

jenshu · 2023-07-12T19:12:28Z

projects/gloo/pkg/translator/filter_chain.go

+				if tcpHostWarn, ok := err.(*validation.TcpHostWarning); ok && tcpHostWarn.ErrorLevel() == validation.ErrorLevels_WARNING {
+					validation.AppendTcpHostWarning(
+						t.report.GetTcpHostReports()[tcpHostWarn.HostNum],
+						validationapi.TcpHostReport_Warning_InvalidDestinationWarning,
+						fmt.Sprintf("listener %s: %s", t.parentListener.GetName(), tcpHostWarn.Error()))
+				} else {
+					validation.AppendTCPListenerError(t.report,
+						validationapi.TcpListenerReport_Error_ProcessingError,
+						fmt.Sprintf("listener %s: %s", t.parentListener.GetName(), err.Error()))
+				}


looks like this whole section, which also appears above, could be extracted out into a reusable func

yessss thanks, that was really bothering me. i just couldn't figure out how to address it. here's the fix: 42c951e

Earlier commits simply added more fields to the ErrorWithKnownLevel interface to aid in providing the necessary information required to report the error. This commit is an attempt to break that out into a separate object so additional fields do not need to be added every time more contextual information is needed to correctly report the error

projects/gloo/pkg/utils/vhosts.go

projects/gloo/pkg/utils/validation/proxy_validation.go

jenshu · 2023-07-13T17:22:41Z

projects/gloo/pkg/translator/filter_chain.go

+				switch errType := errReport.(type) {
+				case validation.ErrorWithKnownLevel:
+					switch errType.ErrorLevel() {
+					case validation.ErrorLevels_WARNING:


i feel like passing around error levels and hostNum adds extra complexity.

instead of needing pass through hostNum, can we just make sure that when the error itself is created, it contains a reference to the TcpHost (i.e. we could include tcpHost.Name as part of the error message)?

instead of checking type = (warning or error) and inferring whether it should be translated to a TcpHostWarning or TCPListenerError, can we just check the actual error type, i.e. if the error is of type TcpHostWarning then here we call AppendTcpHostWarning, otherwise we call AppendTCPListenerError?

if we do this, i think we can remove the whole ErrorWithKnownLevel interface

We are likely going to be adding more warning type errors in the future right? shouldnt we have a standard way to denote that it should just be a warning?

agreed, regarding the complexity. i tried to do what you suggested initially, but then i found out that CreateFilterChain is a part of an interface, and I don't think we can change it, so it is messy to pass that information into the function. hence i had to find a way to pass hostNum back upward. that's a large part of what makes this tricky...

perhaps a middle ground would be to stop passing up the error level (warning/error) and instead just pass the error type InvalidDestinationError up to the caller, and let the caller decide whether it should be an error or warning?

projects/gloo/pkg/utils/validation/proxy_validation.go

ashishb-solo · 2023-07-12T16:22:37Z

projects/gloo/pkg/validation/server_test.go

-				Expect(warnings).To(HaveLen(3))
-				Expect(errors).To(HaveOccurred())
+				Expect(warnings).To(HaveLen(4))
+				Expect(errors).NotTo(HaveOccurred())


The error that's no longer occurring is this:

TcpListener Error: ProcessingError. Reason: listener tcp-listener: 1 error occurred: * *v1.Upstream { gloo-system.test } not found

changelog/v1.15.0-beta18/handle-missing-tcp-upstreams.yaml

projects/gloo/api/grpc/validation/gloo_validation.proto

projects/gloo/pkg/utils/validation/proxy_validation.go

projects/gloo/pkg/plugins/tcp/plugin.go

projects/gloo/pkg/utils/vhosts.go

projects/gloo/pkg/utils/validation/proxy_validation.go

ashishb-solo · 2023-07-13T17:38:35Z

projects/gloo/pkg/translator/filter_chain.go

+				switch errType := errReport.(type) {
+				case validation.ErrorWithKnownLevel:
+					switch errType.ErrorLevel() {
+					case validation.ErrorLevels_WARNING:


agreed, regarding the complexity. i tried to do what you suggested initially, but then i found out that CreateFilterChain is a part of an interface, and I don't think we can change it, so it is messy to pass that information into the function. hence i had to find a way to pass hostNum back upward. that's a large part of what makes this tricky...

ashishb-solo · 2023-07-13T17:39:07Z

projects/gloo/pkg/translator/filter_chain.go

+				switch errType := errReport.(type) {
+				case validation.ErrorWithKnownLevel:
+					switch errType.ErrorLevel() {
+					case validation.ErrorLevels_WARNING:


perhaps a middle ground would be to stop passing up the error level (warning/error) and instead just pass the error type InvalidDestinationError up to the caller, and let the caller decide whether it should be an error or warning?

Previously, we were just using InvalidDestinationWarning for all warning types - we remove that now and use a switch statement to use the right kind

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

projects/gloo/pkg/translator/filter_chain.go

* Handle missing TCP upstreams as warnings * Move changelog to new location * Fix a broken test * Apply suggestions from code review Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io> * Fix error in changelog * Address nil safety issue on TcpHostError * Fix nil safety check * Address a few review comments * Update projects/gloo/api/grpc/validation/gloo_validation.proto Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com> * Populate ErrorLevel for TcpHostWarning * Mark InvalidDestinationError as deprecated * Add some stricter error checking * Move duplicated code into a reusable function * Codegen * Fix an error from refactor * Use the ErrorWithKnownLevel interface to switch on error report types * Handle the case where tcp host num is not provided * Provide a separate Context object to carry additional information Earlier commits simply added more fields to the ErrorWithKnownLevel interface to aid in providing the necessary information required to report the error. This commit is an attempt to break that out into a separate object so additional fields do not need to be added every time more contextual information is needed to correctly report the error * Add docstrings on exported types * Adding changelog file to new location * Deleting changelog file from old location * Adding changelog file to new location * Deleting changelog file from old location * Clean up hard-coded warning type Previously, we were just using InvalidDestinationWarning for all warning types - we remove that now and use a switch statement to use the right kind * Fix an inaccurate comment --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io> Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot>

* Handle missing TCP upstreams as warnings (#8461) * Handle missing TCP upstreams as warnings * Move changelog to new location * Fix a broken test * Apply suggestions from code review Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io> * Fix error in changelog * Address nil safety issue on TcpHostError * Fix nil safety check * Address a few review comments * Update projects/gloo/api/grpc/validation/gloo_validation.proto Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com> * Populate ErrorLevel for TcpHostWarning * Mark InvalidDestinationError as deprecated * Add some stricter error checking * Move duplicated code into a reusable function * Codegen * Fix an error from refactor * Use the ErrorWithKnownLevel interface to switch on error report types * Handle the case where tcp host num is not provided * Provide a separate Context object to carry additional information Earlier commits simply added more fields to the ErrorWithKnownLevel interface to aid in providing the necessary information required to report the error. This commit is an attempt to break that out into a separate object so additional fields do not need to be added every time more contextual information is needed to correctly report the error * Add docstrings on exported types * Adding changelog file to new location * Deleting changelog file from old location * Adding changelog file to new location * Deleting changelog file from old location * Clean up hard-coded warning type Previously, we were just using InvalidDestinationWarning for all warning types - we remove that now and use a switch statement to use the right kind * Fix an inaccurate comment --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io> Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com> Co-authored-by: changelog-bot <changelog-bot> * Fix changelog location * Mark as resolving issue --------- Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io> Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com>

Handle missing TCP upstreams as warnings

db79f53

ashishb-solo requested a review from a team as a code owner July 12, 2023 15:14

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Jul 12, 2023

soloio-bulldozer bot and others added 2 commits July 12, 2023 15:14

Merge main into 5163-missing-tcp-upstreams-as-warnings

3a5d402

Move changelog to new location

8019e03

ashishb-solo added 2 commits July 12, 2023 11:55

Fix a broken test

39b514c

Merge branch '5163-missing-tcp-upstreams-as-warnings' of github.com:s…

df6b420

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

nfuden reviewed Jul 12, 2023

View reviewed changes

ashishb-solo and others added 5 commits July 12, 2023 12:03

Apply suggestions from code review

f3ec49a

Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>

Fix error in changelog

090f09c

Merge branch '5163-missing-tcp-upstreams-as-warnings' of github.com:s…

9977083

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

Address nil safety issue on TcpHostError

190b629

Fix nil safety check

4763857

jenshu reviewed Jul 12, 2023

View reviewed changes

projects/gloo/api/grpc/validation/gloo_validation.proto Outdated Show resolved Hide resolved

jenshu reviewed Jul 12, 2023

View reviewed changes

projects/gloo/api/grpc/validation/gloo_validation.proto Outdated Show resolved Hide resolved

jenshu reviewed Jul 12, 2023

View reviewed changes

ashishb-solo and others added 6 commits July 12, 2023 14:25

Address a few review comments

c69486e

Update projects/gloo/api/grpc/validation/gloo_validation.proto

72e302b

Co-authored-by: Jenny Shu <28537278+jenshu@users.noreply.github.com>

Populate ErrorLevel for TcpHostWarning

4080455

Mark InvalidDestinationError as deprecated

d5a2b70

Merge branch '5163-missing-tcp-upstreams-as-warnings' of github.com:s…

a01069d

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

Add some stricter error checking

42c951e

jenshu reviewed Jul 12, 2023

View reviewed changes

ashishb-solo added 2 commits July 12, 2023 15:21

Move duplicated code into a reusable function

f77fec8

Codegen

a2745fe

ashishb-solo added 4 commits July 12, 2023 15:55

Fix an error from refactor

8a6516f

Use the ErrorWithKnownLevel interface to switch on error report types

fd185ed

Handle the case where tcp host num is not provided

78231ce

nfuden reviewed Jul 13, 2023

View reviewed changes

projects/gloo/pkg/utils/vhosts.go Show resolved Hide resolved

projects/gloo/pkg/utils/validation/proxy_validation.go Show resolved Hide resolved

Add docstrings on exported types

ee915ca

nfuden requested a review from jenshu July 13, 2023 13:35

jenshu reviewed Jul 13, 2023

View reviewed changes

soloio-bulldozer bot and others added 3 commits July 13, 2023 20:14

Merge refs/heads/main into 5163-missing-tcp-upstreams-as-warnings

95c53f4

Adding changelog file to new location

8be1cc9

Deleting changelog file from old location

f1bb819

ashishb-solo commented Jul 13, 2023

View reviewed changes

soloio-bulldozer bot and others added 6 commits July 13, 2023 21:32

Merge refs/heads/main into 5163-missing-tcp-upstreams-as-warnings

1a59a5a

Adding changelog file to new location

49cfc6b

Deleting changelog file from old location

1d11382

Merge refs/heads/main into 5163-missing-tcp-upstreams-as-warnings

cedef8f

Clean up hard-coded warning type

7a161b2

Previously, we were just using InvalidDestinationWarning for all warning types - we remove that now and use a switch statement to use the right kind

Merge branch '5163-missing-tcp-upstreams-as-warnings' of github.com:s…

6467109

…olo-io/gloo into 5163-missing-tcp-upstreams-as-warnings

jenshu reviewed Jul 14, 2023

View reviewed changes

projects/gloo/pkg/translator/filter_chain.go Outdated Show resolved Hide resolved

Fix an inaccurate comment

11683c4

jenshu approved these changes Jul 14, 2023

View reviewed changes

ashishb-solo requested a review from nfuden July 14, 2023 15:09

nfuden approved these changes Jul 14, 2023

View reviewed changes

soloio-bulldozer bot merged commit 181c57f into main Jul 14, 2023
12 checks passed

soloio-bulldozer bot deleted the 5163-missing-tcp-upstreams-as-warnings branch July 14, 2023 15:34

ashishb-solo mentioned this pull request Oct 26, 2023

isolateVirtualHostsBySslConfig changing behavior in hybridGateways validation #8834

Closed

sam-heilbron mentioned this pull request Jun 12, 2024

Validation passes settings in params #9587

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle missing TCP upstreams as warnings #8461

Handle missing TCP upstreams as warnings #8461

ashishb-solo commented Jul 12, 2023 •

edited by solo-changelog-bot bot

Loading

github-actions bot commented Jul 12, 2023 •

edited

Loading

solo-changelog-bot bot commented Jul 12, 2023

jenshu Jul 12, 2023

ashishb-solo Jul 12, 2023

jenshu Jul 12, 2023

ashishb-solo Jul 12, 2023

jenshu Jul 12, 2023

ashishb-solo Jul 12, 2023

jenshu Jul 12, 2023

ashishb-solo Jul 12, 2023

jenshu Jul 12, 2023

ashishb-solo Jul 12, 2023

jenshu Jul 13, 2023 •

edited

Loading

nfuden Jul 13, 2023

ashishb-solo Jul 13, 2023

ashishb-solo Jul 13, 2023

ashishb-solo Jul 12, 2023

ashishb-solo Jul 13, 2023

ashishb-solo Jul 13, 2023

Handle missing TCP upstreams as warnings #8461

Handle missing TCP upstreams as warnings #8461

Conversation

ashishb-solo commented Jul 12, 2023 • edited by solo-changelog-bot bot Loading

Description

Context

Checklist:

github-actions bot commented Jul 12, 2023 • edited Loading

solo-changelog-bot bot commented Jul 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshu Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashishb-solo commented Jul 12, 2023 •

edited by solo-changelog-bot bot

Loading

github-actions bot commented Jul 12, 2023 •

edited

Loading

jenshu Jul 13, 2023 •

edited

Loading