Kong / go-database-reconciler

Apache License 2.0
5 stars 3 forks source link

fix(diff+file): avoid filling defaults in config for kong #133

Closed samugi closed 1 month ago

samugi commented 1 month ago

Summary

the previous logic was filling defaults in the configuration that was passed to Kong. This was problematic, especially where nils were populated as defaults, e.g. if a shorthand_field was passed with some value and the corresponding new field was auto-populated as nil by decK , the auto-populated nil value would take precedence in Kong thus causing the shorthand_field to be ignored (see linked issues).

This change applies the default values only to configurations used for diff, the original configuration from the file is passed to Kong as is.

Configuration

_format_version: "3.0"
plugins:
- config:
    endpoint: http://example.test
  enabled: true
  name: opentelemetry
  protocols:
  - grpc
  - grpcs
  - http
  - https

Before the change

deck gateway sync kong.yml --verbose 1

PUT /plugins/83c19ebe-4cd5-4ac2-8f6b-416af32bdfaf HTTP/1.1
Host: localhost:8001
User-Agent: Go-http-client/1.1
Content-Length: 743
Content-Type: application/json
Accept-Encoding: gzip

{
  "created_at":1724513513,
  "id":"83c19ebe-4cd5-4ac2-8f6b-416af32bdfaf",
  "name":"opentelemetry",
  "config":{"endpoint":"http://example.test","traces_endpoint":null,"batch_flush_delay":null,"batch_span_count":null,"connect_timeout":1000,"header_type":"preserve","headers":null,"http_response_header_for_traceid":null,"logs_endpoint":null,"propagation":{"clear":null,"default_format":"w3c","extract":null,"inject":null},"queue":{"concurrency_limit":1,"initial_retry_delay":0.01,"max_batch_size":200,"max_bytes":null,"max_coalescing_delay":1,"max_entries":10000,"max_retry_delay":60,"max_retry_time":60},"read_timeout":5000,"resource_attributes":null,"sampling_rate":null,"send_timeout":5000},
  "enabled":true,
  "protocols":["grpc","grpcs","http","https"]}

After the change

deck gateway sync kong.yml --verbose 1

PUT /plugins/83c19ebe-4cd5-4ac2-8f6b-416af32bdfaf HTTP/1.1
Host: localhost:8001
User-Agent: Go-http-client/1.1
Content-Length: 195
Content-Type: application/json
Accept-Encoding: gzip

{
  "created_at":1724513513,
  "id":"83c19ebe-4cd5-4ac2-8f6b-416af32bdfaf",
  "name":"opentelemetry",
  "config":{"endpoint":"http://example.test"},
  "enabled":true,
  "protocols":["grpc","grpcs","http","https"]
}

Issues resolved

KAG-5157 KAG-5210

Documentation

Testing

CLAassistant commented 1 month ago

CLA assistant check
All committers have signed the CLA.

codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 15 lines in your changes missing coverage. Please review.

Project coverage is 42.78%. Comparing base (64dd801) to head (decf4d3).

Files Patch % Lines
pkg/diff/diff.go 0.00% 15 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #133 +/- ## ========================================== + Coverage 42.76% 42.78% +0.01% ========================================== Files 75 75 Lines 8820 8809 -11 ========================================== - Hits 3772 3769 -3 + Misses 4586 4579 -7 + Partials 462 461 -1 ```

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

samugi commented 1 month ago

Does this change make the filling defaults only run for diff printing differences but not to sync really updating configuration?

Correct @randmonkey , I added examples of how the change affects deck in the PR description.

Would this approach still require changes in Kong/go-kong?

No, this PR replaces the other one I opened, I will close the old PR if/once this is approved

bungle commented 1 month ago

Does decK or these tools ever use PATCH or POST?

samugi commented 1 month ago

Does decK or these tools ever use PATCH or POST?

not that I'm aware of, but that's a good question for @Kong/team-apiops maybe

samugi commented 1 month ago

failing CI because httpbin.org is down ... I'll try to rerun / merge later