Handle workspace in Kong's client #62

mmorel-35 · 2021-05-13T09:31:55Z

hbagdi · 2021-05-15T00:08:51Z

@mmorel-35 I think the right approach here is to do the following that solves major problems in decK's codebase:

introduce a SetWorkspace(string) and ClearWorkspace() method on *kong.Client; this internally recomputes baseURL for the client (we will need to make baseURL thread-safe as it was assumed to not change after client creation)
Modify the behavior of Root() to use /kong whenever a workspace is set in the client
Introduce a Version() method on the client which internally uses Root() and then sends back the version (without semver parsing)

These 3 could be 3 separate PRs that go in one after the other.
The first one here can greatly help improve decK's codebase as changing workspace context won't require instantiating new clients.

mmorel-35 · 2021-05-15T05:16:56Z

If you add a SetWorkspace() this can set an internal workspace field of the client. Then the NewRequest will be in charge of the construction of the URL with the baseURL and the workspace. There can be an internal function to check the presence of a workspace hasWorkspace(). It would then be used by Root to know which endpoint to call.
What would be the use for ClearWorkspace()?

About the Version() method, what about defining a Informations or Kong service which would hold it and also have the methods like IsInMemory(), Database(), IsRBACEnabled() or IsPortalEnabled() which are used between KIC and the go-kong's integration tests?

Concerning thread-safe, how can it be insured ?

codecov-commenter · 2021-05-15T06:47:29Z

Codecov Report

Merging #62 (ee8b17c) into main (c7c6276) will decrease coverage by 5.80%.
The diff coverage is 52.63%.

@@            Coverage Diff             @@
##             main      #62      +/-   ##
==========================================
- Coverage   45.50%   39.70%   -5.81%     
==========================================
  Files          40       40              
  Lines        2525     2539      +14     
==========================================
- Hits         1149     1008     -141     
- Misses       1077     1282     +205     
+ Partials      299      249      -50

Flag	Coverage Δ
2.0.5	`39.70% <52.63%> (+0.05%)`	⬆️
2.1.4	`39.46% <52.63%> (+0.05%)`	⬆️
2.2.2	`39.46% <52.63%> (+0.05%)`	⬆️
2.3.3	`39.46% <52.63%> (+0.05%)`	⬆️
2.4.0	`39.46% <52.63%> (+0.05%)`	⬆️
community	`39.70% <52.63%> (+0.05%)`	⬆️
enterprise	`?`
enterprise-1.5.0.11	`?`
enterprise-2.1.4.6	`?`
enterprise-2.2.1.3	`?`
enterprise-2.3.3.2	`?`
integration	`39.70% <52.63%> (-5.81%)`	⬇️

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

Impacted Files	Coverage Δ
kong/client.go	`63.70% <43.75%> (-1.88%)`	⬇️
kong/request.go	`63.63% <100.00%> (+1.73%)`	⬆️
kong/mtls_auth_service.go	`0.00% <0.00%> (-66.16%)`	⬇️
kong/workspace_service.go	`0.00% <0.00%> (-56.67%)`	⬇️
kong/admin_service.go	`0.00% <0.00%> (-21.52%)`	⬇️
kong/credentials.go	`85.71% <0.00%> (-14.29%)`	⬇️
kong/utils.go	`71.42% <0.00%> (-3.58%)`	⬇️
kong/sni_service.go	`58.75% <0.00%> (ø)`
kong/route_service.go	`61.70% <0.00%> (ø)`
kong/plugin_service.go	`54.90% <0.00%> (ø)`
... and 14 more

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 c7c6276...ee8b17c. Read the comment docs.

mflendrich

@mmorel-35,

If you add a SetWorkspace() this can set an internal workspace field of the client. Then the NewRequest will be in charge of the construction of the URL with the baseURL and the workspace.

See the attached comments - I've proposed a slightly different approach wrt. constructing the base URL, in order to avoid synchronization issues.

There can be an internal function to check the presence of a workspace hasWorkspace(). It would then be used by Root to know which endpoint to call.

See the attached comments as well - we need to design the APIs so that we can ensure valid concurrent access to the workspace setting.

What would be the use for ClearWorkspace()?

Funny legacy reasons. Some operations in decK require a client without a workspace, even if the tool has been run on a specific workspace. For sake of implementation in go-kong, let's just assume that the user of the client will have freedom to set, change and unset the workspace multiple times. But I believe that we could drop the ClearWorkspace idea in favor of SetWorkspace("") but that's not a strong opinion.

About the Version() method, what about defining a Informations or Kong service which would hold it and also have the methods like IsInMemory(), Database(), IsRBACEnabled() or IsPortalEnabled() which are used between KIC and the go-kong's integration tests?

On a high level I'd like us to have an API like this in go-kong, but that's probably out of scope of this PR. Could you please file an issue with a description of your idea, so that we can discuss it in a narrower context? go-kong would definitely benefit from such an improvement but let's pay due diligence before jumping into coding.

Concerning thread-safe, how can it be insured ?

See my attached review comments.

kong/client.go

hbagdi · 2021-05-17T17:58:52Z

kong/request.go

-// body is always marshaled into JSON.
-func (c *Client) NewRequest(method, endpoint string, qs interface{},
+//NewRequestRaw creates a request based on the inputs.
+func (c *Client) NewRequestRaw(method, baseURL string, endpoint string, qs interface{},


What if we rename baseURL to url?
I think it makes the signature a bit more clear.

Using url would mean using the full one, isn't it?
As it's always concatenated with the endpoint parameter, wouldn't it lead to confusion?

I misunderstood. @mflendrich Should we name these as NewWorkspacedRequest NewRequest and then appropriate docs comment? The current API in this patch is not very easy to grok.

I wanted to achieve two results:

no breaking changes to NewRequest

having a new function that accepts the base URL from the outside

Maybe we can relax the former. In that situation, we could do the following movements:

rename NewRequest to NewRequestCurrentWorkspace

rename NewRequestRaw to NewRequestForBaseURL and say explicitly in the docstring that this expects the workspace URL to be passed as the base URL.

WDYT?

hbagdi · 2021-05-17T17:59:57Z

kong/request.go

+func (c *Client) NewRequest(method, endpoint string, qs interface{},
+	body interface{}) (*http.Request, error) {


Is this a bit more descriptive?

Suggested change

func (c *Client) NewRequest(method, endpoint string, qs interface{},

body interface{}) (*http.Request, error) {

func (c *Client) NewRequest(method, relativePath string, qs interface{},

body interface{}) (*http.Request, error) {

Co-authored-by: Harry <harrybagdi@gmail.com>

kong/client.go

Co-authored-by: Michał Flendrich <m.flendrich@gmail.com>

kong/client.go

hbagdi · 2021-05-25T20:43:11Z

ping @mflendrich

mflendrich

Seems that I reviewed this PR a week ago but the comments remained as pending 😩. Sorry about that.

kong/client.go

mflendrich · 2021-05-18T10:38:24Z

kong/request.go

-// body is always marshaled into JSON.
-func (c *Client) NewRequest(method, endpoint string, qs interface{},
+//NewRequestRaw creates a request based on the inputs.
+func (c *Client) NewRequestRaw(method, baseURL string, endpoint string, qs interface{},


I wanted to achieve two results:

no breaking changes to NewRequest

having a new function that accepts the base URL from the outside

Maybe we can relax the former. In that situation, we could do the following movements:

rename NewRequest to NewRequestCurrentWorkspace

rename NewRequestRaw to NewRequestForBaseURL and say explicitly in the docstring that this expects the workspace URL to be passed as the base URL.

WDYT?

mflendrich

No blockers left from my side.

mflendrich · 2021-06-29T13:29:04Z

This may have something to do with a KIC 2.0 issue about the /:workspace/kong query URL being invalid: #62

mmorel-35 · 2021-06-29T14:01:46Z

This PR has not been released yet, in v0.19.0 the root was just / not /kong, with a new release, and it's integration in kic, the issue Kong/kubernetes-ingress-controller#1353 shall be fixed. It is what you meant @mflendrich, right?

Define Kong method

5d4ac09

mmorel-35 requested a review from a team as a code owner May 13, 2021 09:31

client's URLs machinery

e2feb46

mmorel-35 added 3 commits May 15, 2021 08:50

BaseURL was already parsed, let's keep this simple

810fa48

even simpler

ff86895

ClearWorkspace

0647e60

mmorel-35 mentioned this pull request May 15, 2021

Defines the Information Service on top of Kong's Root endpoint #65

Merged

mmorel-35 changed the title ~~Define Kong method~~ Handle workspace in Kong's client May 15, 2021

Definition of a setter for the client's Workspace

4237ad7

mflendrich suggested changes May 17, 2021

View reviewed changes

Integration of @mflendrich comments

10fb929

mmorel-35 requested a review from mflendrich May 17, 2021 12:55

Update request.go

f1665ac

hbagdi requested changes May 17, 2021

View reviewed changes

mmorel-35 and others added 4 commits May 17, 2021 20:18

Update kong/client.go

0a731c8

Co-authored-by: Harry <harrybagdi@gmail.com>

Update client.go

a4ba5b2

Update request.go

c683acd

Update client.go

bb166ea

mflendrich reviewed May 17, 2021

View reviewed changes

kong/client.go Outdated Show resolved Hide resolved

kong/client.go Outdated Show resolved Hide resolved

Update kong/client.go

97d8295

Co-authored-by: Michał Flendrich <m.flendrich@gmail.com>

hbagdi requested a review from mflendrich May 21, 2021 21:59

hbagdi reviewed May 21, 2021

View reviewed changes

kong/client.go Outdated Show resolved Hide resolved

mflendrich reviewed May 28, 2021

View reviewed changes

mflendrich approved these changes May 28, 2021

View reviewed changes

mmorel-35 added 2 commits May 28, 2021 23:53

Update client.go

ae4ecaf

Update request.go

ee8b17c

hbagdi approved these changes Jun 7, 2021

View reviewed changes

hbagdi merged commit 4ab6403 into Kong:main Jun 7, 2021

mmorel-35 deleted the feature/kong_endpoint branch June 8, 2021 05:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle workspace in Kong's client #62

Handle workspace in Kong's client #62

mmorel-35 commented May 13, 2021

hbagdi commented May 15, 2021

mmorel-35 commented May 15, 2021 •

edited

Loading

codecov-commenter commented May 15, 2021 •

edited

Loading

mflendrich left a comment

hbagdi May 17, 2021

mmorel-35 May 17, 2021 •

edited

Loading

hbagdi May 17, 2021

mflendrich May 18, 2021

hbagdi May 17, 2021

hbagdi commented May 25, 2021

mflendrich left a comment

mflendrich May 18, 2021

mflendrich left a comment

mflendrich commented Jun 29, 2021

mmorel-35 commented Jun 29, 2021

		func (c *Client) NewRequest(method, endpoint string, qs interface{},
		body interface{}) (*http.Request, error) {

Handle workspace in Kong's client #62

Handle workspace in Kong's client #62

Conversation

mmorel-35 commented May 13, 2021

hbagdi commented May 15, 2021

mmorel-35 commented May 15, 2021 • edited Loading

codecov-commenter commented May 15, 2021 • edited Loading

Codecov Report

mflendrich left a comment

Choose a reason for hiding this comment

hbagdi May 17, 2021

Choose a reason for hiding this comment

mmorel-35 May 17, 2021 • edited Loading

Choose a reason for hiding this comment

hbagdi May 17, 2021

Choose a reason for hiding this comment

mflendrich May 18, 2021

Choose a reason for hiding this comment

hbagdi May 17, 2021

Choose a reason for hiding this comment

hbagdi commented May 25, 2021

mflendrich left a comment

Choose a reason for hiding this comment

mflendrich May 18, 2021

Choose a reason for hiding this comment

mflendrich left a comment

Choose a reason for hiding this comment

mflendrich commented Jun 29, 2021

mmorel-35 commented Jun 29, 2021

mmorel-35 commented May 15, 2021 •

edited

Loading

codecov-commenter commented May 15, 2021 •

edited

Loading

mmorel-35 May 17, 2021 •

edited

Loading