feat: add entities defaults from their schemas #573

GGabriele · 2022-01-27T14:14:11Z

Today deck populates entities defaults defining them as part
of the Defaulter utility. This PR introduces the ability
to pull the schema of an entity from the Admin API and to
add its defaults from there.

What this PR doesn't do is change a thing on Defaulter.
A refactoring PR will follow.

This needs Kong/go-kong#119

before this change: deck keep showing diffs for unfilled defaults

$ cat kong.yaml
services:
- name: svc1
  host: mockbin.org
  tags:
  - team-svc1
  routes:
  - name: r1
    https_redirect_status_code: 301
    paths:
    - /r1
upstreams:
- name: upstream1
  targets:
  - target: 198.51.100.11:80

$ deck sync
updating upstream upstream1  {
-  "algorithm": "round-robin",
   "hash_fallback": "none",
   "hash_on": "none",
   "hash_on_cookie_path": "/",
   "healthchecks": {
     "active": {
       "concurrency": 10,
       "healthy": {
         "http_statuses": [
           200,
           302
         ],
         "interval": 0,
         "successes": 0
       },
       "http_path": "/",
-      "https_verify_certificate": true,
       "timeout": 1,
       "type": "http",
       "unhealthy": {
         "http_failures": 0,
         "http_statuses": [
           429,
           404,
           500,
           501,
           502,
           503,
           504,
           505
         ],
         "interval": 0,
         "tcp_failures": 0,
         "timeouts": 0
       }
     },
     "passive": {
       "healthy": {
         "http_statuses": [
           200,
           201,
           202,
           203,
           204,
           205,
           206,
           207,
           208,
           226,
           300,
           301,
           302,
           303,
           304,
           305,
           306,
           307,
           308
         ],
         "successes": 0
       },
-      "type": "http",
       "unhealthy": {
         "http_failures": 0,
         "http_statuses": [
           429,
           500,
           503
         ],
         "tcp_failures": 0,
         "timeouts": 0
       }
     },
-    "threshold": 0
   },
   "id": "0898618e-df83-4289-b112-0490d74051a6",
   "name": "upstream1",
   "slots": 10000
 }

updating service svc1  {
   "connect_timeout": 60000,
   "host": "mockbin.org",
   "id": "5b2f5b96-dc80-4b0f-9efa-10f3789b2a72",
   "name": "svc1",
   "port": 80,
   "protocol": "http",
   "read_timeout": 60000,
-  "retries": 5,
   "tags": [
     "team-svc1"
   ],
   "write_timeout": 60000
 }

updating route r1  {
   "https_redirect_status_code": 301,
   "id": "acbb5161-be3d-4f78-9cf1-855c71fd01c2",
   "name": "r1",
-  "path_handling": "v0",
   "paths": [
     "/r1"
   ],
   "preserve_host": false,
   "protocols": [
     "http",
     "https"
   ],
   "regex_priority": 0,
-  "request_buffering": true,
-  "response_buffering": true,
   "service": {
     "id": "5b2f5b96-dc80-4b0f-9efa-10f3789b2a72"
   },
   "strip_path": false
 }

Summary:
  Created: 0
  Updated: 3
  Deleted: 0

after this change: all defaults are added

$ deck sync
Summary:
  Created: 0
  Updated: 0
  Deleted: 0

codecov-commenter · 2022-01-27T14:15:27Z

Codecov Report

Attention: Patch coverage is 45.96273% with 87 lines in your changes missing coverage. Please review.

Project coverage is 51.06%. Comparing base (ebe0248) to head (67d1dcc).
Report is 589 commits behind head on main.

Files with missing lines	Patch %	Lines
utils/defaulter.go	37.20%	70 Missing and 11 partials ⚠️
file/builder.go	80.64%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #573      +/-   ##
==========================================
- Coverage   51.15%   51.06%   -0.10%     
==========================================
  Files          73       73              
  Lines        8009     8139     +130     
==========================================
+ Hits         4097     4156      +59     
- Misses       3542     3605      +63     
- Partials      370      378       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go.mod

hbagdi · 2022-01-27T19:48:30Z

file/builder.go

@@ -551,6 +551,9 @@ func (b *stateBuilder) services() {
 }

 func (b *stateBuilder) ingestService(s *FService) error {
+	if err := b.addEntityDefaults("services", &s.Service); err != nil {
+		return fmt.Errorf("add defaults to service '%v': %v", *s.Name, err)
+	}


See line below:

b.defaulter.MustSet(&s.Service)

Can we please keep the defaulting logic in the defaulter?
Take "Defaulter" as an input to StateBuilder if that is required.

hbagdi

This is looking much better.
With the change, I'm requesting here (prioritized defaults), this can get a bit more complex but the complexity should be hidden in the "Deafulter".

hbagdi · 2022-01-28T18:33:08Z

file/builder.go

+		b.defaulter, err = defaulter(kongDefaults)
+		if err != nil {
+			return nil, nil, fmt.Errorf("creating defaulter: %w", err)
+		}


The KongDefaults thing hasn't been around for long enough and most users won't fill it in.
We should have a precedence order here:

Take an entity, populate fields take first priority

Fill defaults as present in KongDefaults

Fill defaults via the API

A couple of comments after my last commit:

this change makes the hardcoded defaults in deck obsolete, so I'm removing them here

which means I had to adapt a bit the tests too. I moved the "old" default objects inside the test files in order to minimize the changes on tests, since we are already changing a lot of code. This should keep confidence of changes higher

let me know what you think!

utils/defaulter.go

hbagdi · 2022-01-31T22:48:49Z

@rainest This is a high-risk change. Could you please review and test this change?

rainest · 2022-02-01T00:40:13Z

Loading the branch into the ingress controller and running integration tests surfaced one issue--possibly more, but they're not obvious (the same tests fail repeatedly, but there's no schema violation shown). Some entities are effectively several different entities under a trench coat, and those sub-entities can swap out different parts of the parent schema as they choose. Routes use and require a different default for strip_path, so this breaks gRPC route creation unless you explicitly set it. By adding

      protocols:  
      - grpc
      - grpcs

to the example route in the OP, you get

16:29:10-0800 esenin $ /tmp/bdeck sync -s /tmp/kong.yaml
updating route r1  {
   "https_redirect_status_code": 301,
   "id": "790e50d2-e19f-438b-ac16-470107183658",
   "name": "r1",
   "path_handling": "v0",
   "paths": [
     "/r1"
   ],
   "preserve_host": false,
   "protocols": [
-    "http",
+    "grpc",
-    "https"
+    "grpcs"
   ],
   "regex_priority": 0,
   "request_buffering": true,
   "response_buffering": true,
   "service": {
     "id": "dd530429-739a-471f-80fc-6a90f1074893"
   },
   "strip_path": true
 }

Summary:
  Created: 0
  Updated: 0
  Deleted: 0
Error: 1 errors occurred:
	while processing event: {Update} route r1 failed: HTTP status 400 (message: "schema violation (strip_path: cannot set 'strip_path' when 'protocols' is 'grpc' or 'grpcs')")

It doesn't look like anything other than routes exhibits that problem (at least as far as file naming--nothing else has a _subschemas.lua.

Checking further on the other things that failed (mostly UDP routes).

rainest · 2022-02-01T23:21:21Z

Other KIC failures are poorly-configured timeouts on our end and can be ignored.

For actual deck usage the only thing that comes to mind is the existing defaults system, and this is apparently overriding that. After syncing test1.yaml.txt with current main, syncing again with this branch overwrites the value from the manifest default:

15:13:21-0800 esenin $ /tmp/adeck sync -s /tmp/poing.yaml
creating service example_service
creating route mockpath
Summary:
  Created: 2
  Updated: 0
  Deleted: 0
15:13:24-0800 esenin $ /tmp/bdeck sync -s /tmp/poing.yaml 
updating route mockpath  {
   "https_redirect_status_code": 426,
   "id": "aad4b34e-bd8b-451c-be44-2e78d8559e66",
   "name": "mockpath",
   "path_handling": "v0",
   "paths": [
     "/mock"
   ],
   "preserve_host": false,
   "protocols": [
     "http",
     "https"
   ],
   "regex_priority": 0,
   "request_buffering": true,
   "response_buffering": true,
   "service": {
     "id": "bdaca057-5da5-4f36-b7cb-9e8f809a904e"
   },
-  "strip_path": false
+  "strip_path": true
 }

Summary:
  Created: 0
  Updated: 1
  Deleted: 0

That seems unwanted, since the defaults in the manifest are user-specified, and should take precedence as such.

Other than that and base functionality (which seems fine), are there any other feature interactions we'd want to review?

Tangentially, why the heck does syncing test2.yaml.txt just delete everything with the defaults in place? If you remove that block, it syncs the resources normally (excluding the invalid route). This happens on current main also, it's not something this branch broke.

GGabriele · 2022-02-02T09:32:24Z

Tangentially, why the heck does syncing test2.yaml.txt just delete everything with the defaults in place? If you remove that block, it syncs the resources normally (excluding the invalid route). This happens on current main also, it's not something this branch broke.

Apparently deck is not happy about some leading spaces in the first couple of lines. Removing them will make sync work as expected

GGabriele · 2022-02-02T09:44:29Z

For actual deck usage the only thing that comes to mind is the existing defaults system, and this is apparently overriding that. After syncing test1.yaml.txt with current main, syncing again with this branch overwrites the value from the manifest default:

Actually, the manifest default is supported here, and it takes precedence over schemas defaults:

$ cat kong.yaml
_format_version: "0.1"
_info:
  defaults:
    service:
      port: 8080
services:
- name: svc1
  host: mockbin.org
  tags:
  - team-svc1

$ deck sync
creating service svc1
Summary:
  Created: 1
  Updated: 0
  Deleted: 0

$ deck sync
Summary:
  Created: 0
  Updated: 0
  Deleted: 0

$ http :8001/services/svc1 | jq .port
8080

What's happening in your case is that current version of mergo overwrites fields when set with a zero-value (strip_path: false in this case) while the default from the schema is non-zero (true in this case). That's the same thing that caused this (and actually this).

Would you mind testing this again? I just pushed a commit updating go-kong from this Kong/go-kong#125 , which is a patch that will enable us to fix this and other related issues.

Note: we can do the dependency update on another PR to not make this too big, but let's first test and validate the whole package if possible

rainest · 2022-02-02T18:56:52Z

Works for me with the change, and importing the modified into KIC didn't trigger any test failures, so Kong/go-kong#126

GGabriele · 2022-02-07T17:47:12Z

@hbagdi @rainest I made few changes to make sure to achieve the following order of precedence:

values set in the state file
values set in the {_info: defaults:} object in the state file
schema defaults coming from Admin API (excluded Konnect)
hardcoded defaults under utils/constants.go (Konnect-only)

Let me know what you think!

rainest · 2022-02-07T18:54:46Z

We'd very much like to avoid having all users set strip_path manually for gRPC routes. In the KIC context that's a "you just have to know to set it" gotcha, else your sync breaks. We could add a webhook case for it, but that's still a bit annoying since you'll still get a rejection that you need to fix.

It looks like we can work around the non-standard schema behavior by having a conditional in the middle of ingestRoute(): after running MustSet(), if r.Route.Protocols contains grpc or grpcs, set r.Route.StripPath = false. Any reason not to do that to handle the special case?

Prior to setting the defaults, we should have the same condition check if there's a user-set strip_path: true that we can't honor and error out. Seems a bit silly as there's only one possible value, but it seems better to alert users that their requested state is impossible rather than silently correct their mistake.

GGabriele · 2022-02-07T22:03:50Z

handle case of strip_path and grpc protocols

@rainest can you please check the last commit? I added a patch to handle the case you mentioned.

rainest · 2022-02-08T00:38:47Z

With the latest changes, automated KIC tests pass without modification, so we're good from my perspective: https://github.com/Kong/kubernetes-ingress-controller/runs/5102116046?check_suite_focus=true

hbagdi · 2022-02-09T09:34:26Z

file/builder.go

-		kongDefaults = b.targetContent.Info.Defaults
-	}
-	b.defaulter, err = defaulter(kongDefaults)
+	defaulter, err := defaulter(b.ctx, b.client, b.targetContent, b.isKonnect)


Not in scope but we really need to pas the defaulter as an input to rather than constructing it here.

The only reason I did it so is to avoid duplicating the same exact lines for Kong and Konnect

Separation of concern takes priority over superficial duplication here because it is worth the complexity that will be removed once this is moved out will help test and maintain code.

file/builder.go

hbagdi · 2022-02-09T09:37:47Z

utils/defaulter.go

-func GetKongDefaulter() (*Defaulter, error) {
-	var d Defaulter
-	err := d.Register(&serviceDefaults)
+func GetKongDefaulter(kongDefaults interface{}, isKonnect bool) (*Defaulter, error) {


Take input DefaulterOpts with these two.

Rename isKonnect to disableDynamicDefaults

hbagdi · 2022-02-09T09:39:07Z

utils/defaulter.go

+// 3. schema defaults coming from Admin API (excluded Konnect)
+// 4. hardcoded defaults under utils/constants.go (Konnect-only)
+func GetDefaulter(
+	ctx context.Context, client *kong.Client, kongDefaults interface{}, isKonnect bool,


Take in all args as options, except ctx.

hbagdi · 2022-02-09T09:39:44Z

utils/defaulter.go

+	if client != nil && !isKonnect {
+		return getKongDefaulterWithClient(ctx, client, kongDefaults)
+	}
+	return GetKongDefaulter(kongDefaults, isKonnect)


Can we only export a single method - either this or the parent one? KISS - keep it simple silly.

hbagdi · 2022-02-09T09:40:29Z

tests/integration/sync_test.go

@@ -52,7 +52,7 @@ func testKongState(
 		t.Errorf(err.Error())
 	}
 	opt := []cmp.Option{
-		cmpopts.IgnoreFields(kong.Service{}, "ID", "CreatedAt", "UpdatedAt"),
+		cmpopts.IgnoreFields(kong.Service{}, "ID", "CreatedAt", "UpdatedAt", "Enabled"),


code smell: we will have new properties being defined all the time. Our tests should handle these.

yeah, I will take care of this in the other "test" PR along with other test-related changes.

Today deck populates entities defaults defining them as part of the Defaulter utility. This PR introduces the ability to pull the schema of an entity from the Admin API and to add its defaults from there. What this PR doesn't do is change a thing on Defaulter. A refactoring PR will follow.

hbagdi

Please merge with approval from @rainest

hbagdi · 2022-02-09T15:01:48Z

file/builder.go

+			return initialStripPath, nil
+		}
+		return kong.Bool(false), nil
+	}


Question: If strip path can't be set when protocol is grpc, why return false on the two returns above?

strip_path is a required field, so when protocol is grpc it needs to be set to false (true being the schema default)

for _, p := range route.Protocols { if *p == "grpc" || *p == "grpcs" { if stripPath != nil { return nil, fmt.Errorf("strip path not allowed") return kong.Bool(false), nil } }

Is that not sufficient?

Not directly...I just reworked/cleaned a bit the code to have something like you are suggesting (67d1dcc)

Pull entity schemas from the admin API to fill in their default values rather than using default values in the deck codebase.

GGabriele requested a review from a team as a code owner January 27, 2022 14:14

hbagdi reviewed Jan 27, 2022

View reviewed changes

hbagdi reviewed Jan 28, 2022

View reviewed changes

GGabriele force-pushed the entities_defaults branch from 452b037 to bcbe6f6 Compare January 31, 2022 12:42

GGabriele mentioned this pull request Jan 31, 2022

doc(changelog): release 0.26.0 Kong/go-kong#124

Merged

GGabriele force-pushed the entities_defaults branch from b67f04d to 4a70657 Compare February 2, 2022 09:46

GGabriele force-pushed the entities_defaults branch 3 times, most recently from 9609b41 to f6d2719 Compare February 7, 2022 17:45

rainest requested a review from hbagdi February 8, 2022 00:38

lena-larionova mentioned this pull request Feb 9, 2022

[DOCU-2177] Deck support for plugin defaults Kong/docs.konghq.com#3636

Merged

hbagdi reviewed Feb 9, 2022

View reviewed changes

GGabriele force-pushed the entities_defaults branch 2 times, most recently from 5a0a28a to 3bb2182 Compare February 9, 2022 12:50

GGabriele added 5 commits February 9, 2022 13:59

Rework

f2ec428

Fix test

822b8d3

rework for precedence

aae61fe

Update go-kong

10c210d

GGabriele added 9 commits February 9, 2022 13:59

run update-codegen

2b35595

update dependeces

daa6921

ignore Enabled field

34e58a9

get default constants back in

2688f83

use static defaults for konnect

4e445b4

improve logic

95d43c9

handle case of strip_path and grpc protocols

62615ed

fix konnect nad add test

09a1e82

introduce DefaulterOpts and unexport GetKongDefaulter

e9d0bfd

GGabriele force-pushed the entities_defaults branch from 3bb2182 to e9d0bfd Compare February 9, 2022 12:59

update dependeces

35dd1c1

hbagdi reviewed Feb 9, 2022

View reviewed changes

cleanup strip_path related code

67d1dcc

rainest approved these changes Feb 11, 2022

View reviewed changes

rainest merged commit f34df6e into main Feb 11, 2022

rainest deleted the entities_defaults branch February 11, 2022 22:05

This was referenced Feb 12, 2022

decK uses the wrong default for strip_path #555

Closed

fix:default value miss in route & upstream #343

Closed

This was referenced Feb 22, 2022

chore(deps) update deck to v1.11.0 Kong/kubernetes-ingress-controller#2286

Merged

fix(builder) set strip_path after defaults #596

Merged

shaneutt mentioned this pull request Mar 22, 2022

KIC update all routes & upsteam because some attributes not set to default value Kong/kubernetes-ingress-controller#1212

Closed

GGabriele mentioned this pull request Aug 2, 2022

feat: allow setting defaults for services' host and port #724

Merged

AntoineJac pushed a commit that referenced this pull request Jan 23, 2024

feat: add entities defaults from their schemas (#573)

f45ccaf

Pull entity schemas from the admin API to fill in their default values rather than using default values in the deck codebase.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add entities defaults from their schemas #573

feat: add entities defaults from their schemas #573

GGabriele commented Jan 27, 2022

codecov-commenter commented Jan 27, 2022 •

edited

Loading

hbagdi Jan 27, 2022

hbagdi left a comment

hbagdi Jan 28, 2022

GGabriele Jan 31, 2022 •

edited

Loading

hbagdi commented Jan 31, 2022

rainest commented Feb 1, 2022

rainest commented Feb 1, 2022

GGabriele commented Feb 2, 2022

GGabriele commented Feb 2, 2022 •

edited

Loading

rainest commented Feb 2, 2022

GGabriele commented Feb 7, 2022

rainest commented Feb 7, 2022

GGabriele commented Feb 7, 2022

rainest commented Feb 8, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi Feb 9, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi left a comment

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

hbagdi Feb 9, 2022

GGabriele Feb 9, 2022

feat: add entities defaults from their schemas #573

feat: add entities defaults from their schemas #573

Conversation

GGabriele commented Jan 27, 2022

codecov-commenter commented Jan 27, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GGabriele Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

hbagdi commented Jan 31, 2022

rainest commented Feb 1, 2022

rainest commented Feb 1, 2022

GGabriele commented Feb 2, 2022

GGabriele commented Feb 2, 2022 • edited Loading

rainest commented Feb 2, 2022

GGabriele commented Feb 7, 2022

rainest commented Feb 7, 2022

GGabriele commented Feb 7, 2022

rainest commented Feb 8, 2022

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

Choose a reason for hiding this comment

hbagdi left a 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

codecov-commenter commented Jan 27, 2022 •

edited

Loading

GGabriele Jan 31, 2022 •

edited

Loading

GGabriele commented Feb 2, 2022 •

edited

Loading