Kong / go-kong

Go binding for Kong's admin API
Apache License 2.0
87 stars 41 forks source link

fix(*): avoid setting nil for missing fields #463

Closed samugi closed 4 weeks ago

samugi commented 1 month ago

when filling defaults, the existing logic sets nil on missing fields, but in Kong Gateway explicit nils result in the corresponding fields to be unset, causing problems such as: KAG-5157

This change removes any unset fields that don't have a default from the configuration instead of explicitly setting them to nil.

@reviewers: could this be a breaking change? I tested it in decK and it seems to work but I'm worried about other projects using this lib. An alternative would be creating another utils function with the new behavior (which I think would require updating at least deck, go-database-reconciler and probably kubernetes-ingress-controller), but if we're fairly confident the func is only used while interacting with decK then it might be considered safe enough.

Searching occurrences.

For more details see: KAG-5210

codecov-commenter commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 59.94%. Comparing base (c64125c) to head (6dbf5df).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #463 +/- ## ========================================== + Coverage 59.85% 59.94% +0.08% ========================================== Files 71 71 Lines 4449 4449 ========================================== + Hits 2663 2667 +4 + Misses 1167 1165 -2 + Partials 619 617 -2 ``` | [Flag](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | Coverage Δ | | |---|---|---| | [2.1](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `36.36% <100.00%> (+0.08%)` | :arrow_up: | | [2.2](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `48.77% <100.00%> (+0.08%)` | :arrow_up: | | [2.3](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `49.40% <100.00%> (+0.08%)` | :arrow_up: | | [2.4](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `49.44% <100.00%> (+0.08%)` | :arrow_up: | | [2.5](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `49.44% <100.00%> (+0.08%)` | :arrow_up: | | [2.6](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `49.44% <100.00%> (+0.08%)` | :arrow_up: | | [2.7](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `51.13% <100.00%> (+0.08%)` | :arrow_up: | | [2.8](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `51.13% <100.00%> (+0.08%)` | :arrow_up: | | [3.0](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `55.27% <100.00%> (+0.08%)` | :arrow_up: | | [3.1](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `56.88% <100.00%> (+0.08%)` | :arrow_up: | | [3.2](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `56.88% <100.00%> (+0.08%)` | :arrow_up: | | [3.3](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `56.88% <100.00%> (+0.08%)` | :arrow_up: | | [3.4](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `59.24% <100.00%> (+0.08%)` | :arrow_up: | | [3.5](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `57.06% <100.00%> (+0.08%)` | :arrow_up: | | [3.6](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `57.06% <100.00%> (+0.08%)` | :arrow_up: | | [community](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `44.14% <100.00%> (+0.08%)` | :arrow_up: | | [enterprise](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `58.37% <100.00%> (+0.08%)` | :arrow_up: | | [integration](https://app.codecov.io/gh/Kong/go-kong/pull/463/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong) | `59.94% <100.00%> (+0.08%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Kong#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

samugi commented 1 month ago

not sure why the linter got mad

randmonkey commented 1 month ago

not sure why the linter got mad

https://github.com/Kong/go-kong/actions/runs/10493904848/job/29068947573?pr=463 Looks like the linter was updated some day but it did not notify us

pmalek commented 1 month ago

@samugi https://github.com/Kong/go-kong/pull/464 This should solve the linter issues and align linter version to be the same on CI and locallly

bungle commented 1 month ago

For visibility:

I do not fully understand why there is even this FillPluginsDefaults. I think it is wrong that the defaults or nulls are filled to config (prior sending them to Kong API). If user has .yaml file, the tool should not invent any fields in it, and send only those that user has specified, e.g. even when user has specified field: ~ (aka null), that should be sent to kong, but nothing else. Kong, at least the Lua CP will fill the defaults, and do proper thing.

Now if tool itself needs these to be filled (e.g. for easier diffing or something), it is another question, but that should not affect what is sent to wire to Kong API (provided that Koko API works the same).

This of course means that the tool should also never use PATCH or POST, just PUT and DELETE and GET. And if it needs for this to invent anything, I would say only the id but that you need to take some care with. E.g. multiple runs should probably produce same id (or primary key).

That said, we can first stop filling nulls (as it fixes the immediate issue of deprecated shorthand precedence), and perhaps later (in other PR) stop filling defaults.

mheap commented 1 month ago

That said, we can first stop filling nulls (as it fixes the immediate issue of deprecated shorthand precedence), and perhaps later (in other PR) stop filling defaults.

Stop filling default nulls, you mean? Active null should still be accepted

samugi commented 1 month ago

That said, we can first stop filling nulls (as it fixes the immediate issue of deprecated shorthand precedence), and perhaps later (in other PR) stop filling defaults.

Stop filling default nulls, you mean? Active null should still be accepted

yes @mheap that is correct, this change is intended to only apply to auto-filled defaults

samugi commented 1 month ago

Note: this appears to be breaking the decK diff output (where now all nulls show up as diffs) - probably expected given the change here, because of this, so we might need to do it differently (?), e.g. like @bungle says, by preventing default to be filled at all when the config is sent to Kong (and keep the behavior unchanged for diffs). I suspect that change must be done ~in decK~ somewhere else instead of here.

samugi commented 1 month ago

second attempt: https://github.com/Kong/go-database-reconciler/pull/133