feat: return error response bodies in config updates and fold config service into Kong client

[rainest]

Returns Kong POST /config bodies from ReloadDeclarativeRawConfig().

Folds ConfigService into kong.Client. go-kong services generally interact with a single distinct Kong entity (a route, service, consumer, etc.) whereas POST /config sends a blob of various kinds of entities in a single request.

Releases 0.37.0. Note that these are both breaking changes, but, the config service was only recently added and only used in one place in KIC, and we're updating that part of KIC (in part to make use of the now-returned response body) anyway, so shouldn't be cause for concern.

[codecov-commenter]

Codecov Report

Base: 53.41% // Head: 53.52% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (7468719) compared to base (c8ad8ec).
Patch coverage: 75.67% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #271      +/-   ##
==========================================
+ Coverage   53.41%   53.52%   +0.11%     
==========================================
  Files          51       50       -1     
  Lines        4647     4652       +5     
==========================================
+ Hits         2482     2490       +8     
+ Misses       1637     1634       -3     
  Partials      528      528              

Flag	Coverage Δ
2.1.4	36.45% <67.56%> (+0.06%)	⬆️
2.2.1.3	46.02% <2.70%> (-0.05%)	⬇️
2.2.2	36.45% <67.56%> (+0.06%)	⬆️
2.3.3	38.17% <67.56%> (+0.06%)	⬆️
2.3.3.4	46.02% <2.70%> (-0.05%)	⬇️
2.4.0	38.26% <67.56%> (+0.06%)	⬆️
2.4.1.3	46.02% <2.70%> (-0.05%)	⬇️
2.5.1	38.26% <67.56%> (+0.06%)	⬆️
2.5.1.2	46.02% <2.70%> (-0.05%)	⬇️
2.6.0	38.26% <67.56%> (+0.06%)	⬆️
2.6.0.2	46.02% <2.70%> (-0.05%)	⬇️
2.7.0	38.26% <67.56%> (+0.06%)	⬆️
2.7.0.0	47.69% <2.70%> (-0.06%)	⬇️
2.8.0	38.26% <67.56%> (+0.06%)	⬆️
2.8.2.2	47.69% <2.70%> (-0.06%)	⬇️
3.0.1	37.66% <67.56%> (+0.06%)	⬆️
3.0.1.0	48.19% <2.70%> (-0.06%)	⬇️
3.1.0	39.29% <67.56%> (+0.06%)	⬆️
3.1.0.0	49.82% <2.70%> (-0.06%)	⬇️
community	39.96% <75.67%> (+0.12%)	⬆️
enterprise	50.42% <2.70%> (-0.06%)	⬇️
enterprise-nightly	49.82% <2.70%> (-0.06%)	⬇️
integration	53.52% <75.67%> (+0.11%)	⬆️
nightly	39.35% <75.67%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
kong/client.go	67.20% <75.67%> (+1.31%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pmalek]

Since we're running tests in this repo against actual Kong instances we should add a tests for this functionality (guarded by a version check)

[rainest]

Since we're running tests in this repo against actual Kong instances we should add a tests for this functionality (guarded by a version check)

We are stuck for lack of an actual Kong version that includes this, however. Kong/kong#10161 isn't in master yet.

[pmalek]

Since we're running tests in this repo against actual Kong instances we should add a tests for this functionality (guarded by a version check)

We are stuck for lack of an actual Kong version that includes this, however. Kong/kong#10161 isn't in master yet.

Yup, hence my proposing to guard this by a version check (when we know which version it will be).

[rainest]

On further thought, is there anything we want to test re the body? go-kong isn't (yet) responsible for anything beyond receiving and returning it. All more complex logic is currently downstream (i.e. the error parsing in Kong/kubernetes-ingress-controller#3446).

go-kong could be responsible for error parsing and probably should be responsible for it long-term, but at present we only have the existing Kong error format, which we effectively cannot parse, and the as-yet unreleased Kong/kong#10161 format, which we can parse and do parse in KIC#3446

Both the new Kong error format and KIC parsing are completely new, however, so I expect we may encounter things we want to change on either side. Does therefore it make sense to keep the go-kong body handling loose (i.e. it provides the body to downstream as-is and doesn't care about the contents at all itself) or lock that into go-kong now?

With the controller as the sole user of the new format for the time being, I think we may want to leave the body parsing logic (and associated version-gated tests) there for now, see if we get feedback from the field, and if the initial implementation seems solid, graduate body parsing into go-kong functionality? IMO we create an issue to collect any feedback on the KIC usage and follow-up on making it generic go-kong functionality later.

[pmalek]

Just 2 nits.

As for processing/manipulating the body in go-kong vs KIC: I'm fine with leaving it here. It's actually better I'd say.

As for the tests: I just wanted to suggest a test where we assert for a given request/endpoint/payload tuple we get an expected body returned from ReloadDeclarativeRawConfig.

[rainest]

golangci-lint apparently decided it should be upset about year-old code in this PR. I opened #273 since IDK exactly what we're doing with that copy instead of read, and fixing it is out of scope for this PR.

[rainest]

7008ad7 makes the suggested change from chat discussion, which removes the config service and folds its functionality into the Kong client.

[rainest]

f7b64c0 because I forgot to move the test after removing the config service.

ab5b18a since something I did as part of that broke error reporting, and because the old error semantics were a bit off when always returning the body.

[rainest]

Alright, one more for the road. Added an additional breaking feature, a61b0d5, because Kong/kong#10161 requires a new query string to return the error format that Kong/kubernetes-ingress-controller#3446 wants to use.

[pmalek]

I made some minor tweaks to the test itself and added the body contents to the error we return in case the response code indicates an error.

But there's actually a more important thing here: the test fails with the following error:

{"name":"invalid declarative configuration","message":"declarative config is invalid: {flatten_errors=\"unknown field\"}","fields":{"flatten_errors":"unknown field"},"code":14}

The reason for this is that we don't test against a dbless kong 😱

go-kong/.ci/setup_kong.sh

Line 49 in 4b75690

-e "KONG_DATABASE=postgres" \

I've created #274 to track this.

[rainest]

I made some minor tweaks to the test itself and added the body contents to the error we return in case the response code indicates an error.

Do we indeed want to do this? I'd actually removed this from the original error since we're now always returning bodies unless we can't read them, so if you get an error back you have the body, and can either log it or inspect it as needed. That body could be quite large, so I figured it made sense to omit it from the error string if you don't care beyond a brief explanation.

But there's actually a more important thing here: the test fails with the following error

This is... odd. Is it somehow interpreting the query string as a field in some cases? Doing a packet capture shows no flatten_errors in the body, and attempting to replicate the request manually succeeds:

https://gist.github.com/rainest/b956bf4db29abb04418fd002bf3b0a1a

That looks like a gateway-side bug maybe.

[rainest]

Alright, 348dd08 to fix the error. Kong has an odd "bug" (not sure, since we appear to be aware of the behavior--we code around it--but keep it as-is) where query string fields are treated as part of the config body. Special-case known fields (flatten_errors and check_hash) are explicitly stripped by the API code, but versions prior to 3.2 don't strip flatten_errors because they don't know about it.

I have no idea why we do this, but the upshot is we need version-dependent behavior for part of the test only. This isn't something go-kong typically does, so the change adds some borrowed code from the "skip entire test by version" helper.

[flrgh]

This is... odd. Is it somehow interpreting the query string as a field in some cases? Doing a packet capture shows no flatten_errors in the body, and attempting to replicate the request manually succeeds:

For some more background, this is an unfortunate quirk of the web framework that the admin API uses. It lumps query args, form parameters, and I believe even request body JSON all into one single object (self.params) that is passed to the endpoint/route handler:

Name	Description
self.params	A table containing all request parameters merged together, including query parameters and form-encoded parameters from the body of the request. See Request Parameters for more details.

So POST /config?foo=bar with a JSON content-type and JSON body of { "baz": "bat" } yields a lua table that looks like { foo = "bar", baz = "bat" } for self.params. We process this table by parsing and stripping known query args, but any unknown query args will wind up being passed through to the declarative config schema validator.