feat: logs friendly names with plugins #662

GGabriele · 2022-04-26T14:55:02Z

Based on the initial work in #459

Changes from the first PR:

rebased on top of main and solved conflicts
reworked a bit some logic
added nil checks
reworked and reorganized commits and commits messages for an (hopefully) easier review process

rainest

Mostly tidying. The changelog process change was something I was thinking about for a while (IMO it has been a significant workflow improvement in the other repos where we've adopted it), and the buried breaking change here is something that's hard to catch when instead writing it at the end, so asking for some scope creep to change process here as well.

rainest · 2022-04-29T19:54:31Z

utils/utils.go

+func AsKongConsumer(c kong.Consumer) *kong.Consumer {
+	consumer := &kong.Consumer{ID: kong.String(*c.ID)}
+	if c.Username != nil {
+		consumer.Username = kong.String(*c.Username)
+	}
+	return consumer
+}
+
+func AsKongService(s kong.Service) *kong.Service {
+	service := &kong.Service{ID: kong.String(*s.ID)}
+	if s.Name != nil {
+		service.Name = kong.String(*s.Name)
+	}
+	return service
+}
+
+func AsKongRoute(r kong.Route) *kong.Route {
+	route := &kong.Route{ID: kong.String(*r.ID)}
+	if r.Name != nil {
+		route.Name = kong.String(*r.Name)
+	}
+	return route
+}


These are exported and should have doc comments, especially since it's kinda hard to understand why you'd use them--reading them is kinda weird since it looks like they take a kong.X and return a copy of it, kinda, with a bunch of fields stripped out?

Naming like AsFooBar(f foo.Bar) *foo.Bar reads weird since I'd normally expect that for functions that transform one type into a different similar type, but these instead take a type and return the same type with most fields omitted, so we should consider a different name for them even.

Yeah I totally agree...what about GetStrippedRoute or GetRouteWithIDAndName?

GetIDNameOnlyServiceCopy ?

The doc comments are the more important part. Something like

// GetRouteWithIDAndName takes a kong.Route and returns a reference to a new kong.Route. The new kong.Route has the ID and Name fields from the input route, but none of the other input route's fields.

And above all such functions:

// These functions return stripped copies (ID and Name only) of Kong resource objects. We use these elsewhere [in place] because we cannot use the original objects, as objects with all fields present would [result in some unwanted outcome].

with the [placeholder] bits filled in. It's not immediately clear to me why we wouldn't use the original objects (they do already have their Name and ID, so it seems like we'd just be able to use them as-is), so while I assume there is some reason, I can't intuit it, and suspect that future readers will have the same confusion.

Hijacking the end of this for a pre-release changelog entry reminder also.

did a bit more of investigation here and it seems that we actually only need to zero-out the timestamps from the entities, instead of creating new ones only with IDs and Names 1364c15

That said, it was done this way (setting only the IDs in current main and adding the Names in the original PR) because that's just what's needed for referenced entities. For example, this test is currently failing because the embedded entity now has all its field, instead of having just the few required ones.

(it's better to fully visualise it from here)

WDYT?

Ah, now it makes sense. That explanation was basically exactly what I was looking for in the comment 😄 I wasn't making the connection that these were references. With that in mind, the original name/ID only approach makes more sense than timestamp stripping:

// These GetFooReference functions return stripped copies (ID and Name only) of Kong resource structs. We use these // within KongRawState structs to indicate entity relationships. While state files indicate relationships by nesting (A // collection of services is [{name: "foo", id: "1234", connect_timeout: 600000, routes: [{name: "fooRoute"}]}]), // KongRawState is flattened, with all entities listed independently at the top level. To preserve the relationships, these // flattened entities include references (the route from earlier becomes {name: "fooRoute", service: {name: "foo", id: // "1234"}}). // GetRouteReference returns a name+ID only copy of the input route, for use in references from other objects GetRouteReference() { ... return the ID/name only copy }

Totally agree, that wasn't clear to me either :)

2b4c09d

state/types.go

state/builder.go

codecov-commenter · 2022-05-09T09:42:21Z

Codecov Report

Merging #662 (51e19d2) into main (e6da448) will decrease coverage by 0.77%.
The diff coverage is 29.08%.

@@            Coverage Diff             @@
##             main     #662      +/-   ##
==========================================
- Coverage   44.37%   43.59%   -0.78%     
==========================================
  Files          74       74              
  Lines        8778     8966     +188     
==========================================
+ Hits         3895     3909      +14     
- Misses       4522     4688     +166     
- Partials      361      369       +8

Impacted Files	Coverage Δ
state/builder.go	`0.00% <0.00%> (ø)`
utils/utils.go	`32.85% <0.00%> (-11.38%)`	⬇️
file/builder.go	`56.67% <44.44%> (+1.26%)`	⬆️
state/types.go	`65.70% <51.76%> (-8.80%)`	⬇️
state/rbac_endpoint_permission.go	`71.31% <100.00%> (ø)`
cmd/common_konnect.go	`8.08% <0.00%> (+0.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6da448...51e19d2. Read the comment docs.

Kong and Konnect APIs only require IDs for referenced entities. In some cases, injecting the entities name in the requests is problematic. This makes sure these names are stripped before making API requests, while preserving the capability of logging user-friendly messages as added in #662

* feat: add names and usernames to plugin, consumer and route * refactor: use of go-kong's FriendlyName() instead of Identifier() in logs * feat: ignore diff for names in embedded entities * tests: add test for plugins on entities creation Co-authored-by: Matthieu Morel <matthieu.morel@cnp.fr>

Kong and Konnect APIs only require IDs for referenced entities. In some cases, injecting the entities name in the requests is problematic. This makes sure these names are stripped before making API requests, while preserving the capability of logging user-friendly messages as added in #662

GGabriele requested a review from hbagdi April 26, 2022 14:55

GGabriele requested a review from a team as a code owner April 26, 2022 14:55

rainest suggested changes Apr 29, 2022

View reviewed changes

rainest mentioned this pull request May 3, 2022

doc: add unreleased 1.13 changelog #668

Merged

GGabriele force-pushed the ggabriele/feature/entities_identifier branch from 1256568 to 51e19d2 Compare May 9, 2022 09:40

GGabriele force-pushed the ggabriele/feature/entities_identifier branch 3 times, most recently from 2679999 to 1268ed6 Compare May 11, 2022 10:05

mmorel-35 and others added 6 commits May 12, 2022 10:51

feat: add names and usernames to plugin, consumer and route

2b4c09d

refactor: use of go-kong's FriendlyName() instead of Identifier()

cce1ff4

feat: add properties name into state

779edeb

feat: ignore diff for names in embedded entities

bfc17c1

tests: add test for plugins on entities creation

0491590

chore: add changelog entry for logging friendly name for plugins

284d718

GGabriele force-pushed the ggabriele/feature/entities_identifier branch from 1268ed6 to 284d718 Compare May 12, 2022 08:56

rainest approved these changes May 12, 2022

View reviewed changes

rainest merged commit e7b8777 into main May 12, 2022

rainest deleted the ggabriele/feature/entities_identifier branch May 12, 2022 20:51

GGabriele mentioned this pull request May 12, 2022

Use FriendlyNames #459

Closed

GGabriele mentioned this pull request Jun 3, 2022

fix: strip references name for plugins and routes #693

Merged

GGabriele mentioned this pull request Oct 19, 2022

Print Names instead of ID when possible #417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: logs friendly names with plugins #662

feat: logs friendly names with plugins #662

GGabriele commented Apr 26, 2022

rainest left a comment

rainest Apr 29, 2022

GGabriele May 9, 2022

rainest May 10, 2022

GGabriele May 11, 2022 •

edited

Loading

rainest May 11, 2022

GGabriele May 12, 2022

codecov-commenter commented May 9, 2022

feat: logs friendly names with plugins #662

feat: logs friendly names with plugins #662

Conversation

GGabriele commented Apr 26, 2022

rainest left a comment

Choose a reason for hiding this comment

rainest Apr 29, 2022

Choose a reason for hiding this comment

GGabriele May 9, 2022

Choose a reason for hiding this comment

rainest May 10, 2022

Choose a reason for hiding this comment

GGabriele May 11, 2022 • edited Loading

Choose a reason for hiding this comment

rainest May 11, 2022

Choose a reason for hiding this comment

GGabriele May 12, 2022

Choose a reason for hiding this comment

codecov-commenter commented May 9, 2022

Codecov Report

GGabriele May 11, 2022 •

edited

Loading