feat: interact with plugins from service endpoint for least privilege

[achoimet]

Hello,

Is it possible to add those methods to be able to CREATE/UPDATE/DELETE plugins from the service endpoints ? In this case we can enable least privilege with the kong community edition.

[CLAassistant]

All committers have signed the CLA.

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

antoine.choimet seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[rainest]

Can you clarify what scenario you're in that needs this? I suppose this would allow service plugin creation with certain RBAC rules, but that shouldn't be a concern in the community edition.

That aside we'd probably want to handle this either with a parameter or wrapper functions (probably the latter, since it'd allow preserving the existing Create() signature) so as to not duplicate a bunch of the plugin creation code.

[achoimet]

The use case is that we have a tool to drive the creation and configuration of request termination plugin. For this tool we created a dedicated consumer, this consumer has a dedicated group and only a set of accessible paths.

We don't want this consumer to be able to interact with a plugin at global level (/plugins), only with a targeted service.
Since Kong is able to do plugin configutation at service level, we were thinking to be a good idea to reflect that on your great library, to respect the principle of least privilege.

Yes ok for the implementation as a wrapper, so I give the path as a parameter of Create() function ?

[achoimet]

Hello @rainest, since the endpoint/query path is defined inside the function, and when I want to add a plugin at service level I need to add /services/SERVICE_NAME before /plugins, it seems not possible to me to wrap the functions to do this behaviour ? Am I wrong ?
To not repeat the code, I could add a parameter but yes the signature need to be changed everywhere..

For example for Create :

func (s *PluginService) Create(ctx context.Context,
	plugin *Plugin, serviceIDorName *string,
) (*Plugin, error) {
	queryPath := "/plugins"
	method := "POST"
	if plugin.ID != nil {
		queryPath = queryPath + "/" + *plugin.ID
		method = "PUT"
	}
	if serviceIDorName != nil {
		queryPath = "/services/" + *serviceIDorName + queryPath
	}
	req, err := s.client.NewRequest(method, queryPath, nil, plugin)
	if err != nil {
		return nil, err
	}

	var createdPlugin Plugin
	_, err = s.client.Do(ctx, req, &createdPlugin)
	if err != nil {
		return nil, err
	}
	return &createdPlugin, nil
}

I can also add methods in the service_service.go, AddPlugin RemovePlugin UpdatePlugin maybe it will be more elegant.
edit : added to service if it makes more sense

[rainest]

You'd leave the Create() signature as is, but modify it and CreateForService() to use a helper that accepts an endpoint. Rather than modifying CreateForService() to use Create(), you're modifying both Create() and CreateForService()` to use a new function that handles their shared logic.

The exported functions determine the endpoint and perform any validation they need, and then pass the endpoint/plugin along to the helper. Roughly:

func (s *PluginService) CreateForService(ctx context.Context,
	serviceIDorName *string, plugin *Plugin,
) (*Plugin, error) {
	// these remain here because they're specific to this endpoint
	if isEmptyString(plugin.ID) {
		return nil, fmt.Errorf("ID cannot be nil for Update operation")
	}
	if isEmptyString(serviceIDorName) {
		return nil, fmt.Errorf("serviceIDorName cannot be nil")
	}

	return s.create(ctx, plugin, fmt.Sprintf("/services/%v/plugins", *serviceIDorName), "POST")
 
func (s *PluginService) create(ctx context.Context, plugin *Plugin, endpoint, method string) (*Plugin, error) {
	req, err := s.client.NewRequest("POST", endpoint, nil, plugin)
	if err != nil {
		return nil, err
	}

	var createdPlugin Plugin
	_, err = s.client.Do(ctx, req, &createdPlugin)
	if err != nil {
		return nil, err
	}
	return &createdPlugin, nil

With that approach, Create() should be able to keep its existing signature, it'll just need a small refactor to pass the appropriate method and endpoint for that method on to create().

[achoimet]

Thanks for your reply @rainest , I tried to respond to your expectations and I created a sendRequest method common to the methods Create / CreateForService etc.

Is it okay for you ?

[rainest]

Ah, true, I suppose you can just use that for everything. It looks like something has broken on the List function, so that may require its own function--the Expected nil, but got: &kong.ListOpt{Size:2, Offset:"WyI2N2UwZjNhZC0xMTExLTQwYzEtYTMyOS05MjFhMTdlMTUyZmQiXQ", Tags:[]*string(nil), MatchAllTags:false} failure looks like the shared one probably can't handle responses with multiple pages. There's also a linter error for one of the errors being unchecked.

Conceptually though, that looks good, so it should be good to go once the errors are addressed (note that you'll also need to sign the CLA).

[achoimet]

@rainest Ok I fixed the test and fixed the two lint errors. The CLA seems now signed.

[rainest]

CI seems unhappy because third-party PRs can't upload covergage info, which is fine, but I thought we had something in place to skip that when it was unavailable. Checking around to see why that's not the case.

[achoimet]

@rainest Any news? Do I need to do something from my side?

[codecov-commenter]

Codecov Report

Merging #192 (c2bbbd7) into main (1ef06ec) will decrease coverage by 13.69%.
The diff coverage is 49.15%.

@@             Coverage Diff             @@
##             main     #192       +/-   ##
===========================================
- Coverage   53.47%   39.78%   -13.70%     
===========================================
  Files          44       44               
  Lines        3888     3924       +36     
===========================================
- Hits         2079     1561      -518     
- Misses       1353     2067      +714     
+ Partials      456      296      -160     

Flag	Coverage Δ
2.0.5	39.78% <49.15%> (+0.01%)	⬆️
2.1.4	39.62% <49.15%> (+0.01%)	⬆️
2.2.2	39.62% <49.15%> (+0.01%)	⬆️
2.3.3	39.62% <49.15%> (+0.01%)	⬆️
2.4.0	39.62% <49.15%> (+0.01%)	⬆️
2.5.1	39.62% <49.15%> (+0.01%)	⬆️
2.6.0	39.62% <49.15%> (+0.01%)	⬆️
2.7.0	39.62% <49.15%> (+0.01%)	⬆️
2.8.0	39.62% <49.15%> (+0.01%)	⬆️
community	39.78% <49.15%> (+0.01%)	⬆️
enterprise	?
enterprise-1.5.0.11	?
enterprise-2.1.4.6	?
enterprise-2.2.1.3	?
enterprise-2.3.3.4	?
enterprise-2.4.1.3	?
enterprise-2.5.1.2	?
enterprise-2.6.0.2	?
enterprise-2.7.0.0	?
enterprise-2.8.0.0	?
integration	39.78% <49.15%> (-13.70%)	⬇️

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

Impacted Files	Coverage Δ
kong/plugin_service.go	51.54% <49.15%> (-2.26%)	⬇️
kong/mtls_auth_service.go	0.00% <0.00%> (-62.50%)	⬇️
kong/developer_service.go	0.00% <0.00%> (-54.87%)	⬇️
kong/workspace_service.go	0.00% <0.00%> (-51.21%)	⬇️
kong/rbac_role_service.go	0.00% <0.00%> (-48.94%)	⬇️
kong/developer_role_service.go	0.00% <0.00%> (-46.67%)	⬇️
kong/entity_permission_service.go	0.00% <0.00%> (-40.66%)	⬇️
kong/rbac_user_service.go	0.00% <0.00%> (-34.89%)	⬇️
kong/types.go	7.04% <0.00%> (-30.99%)	⬇️
kong/endpoint_permission_service.go	0.00% <0.00%> (-26.42%)	⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[rainest]

@achoimet it looks like CodeCov was just having issues the other day; integration tests that don't require a license are now completing fine. It does look like there are still a few linter errors around an incorrect error check that you do need to fix.

[achoimet]

@achoimet it looks like CodeCov was just having issues the other day; integration tests that don't require a license are now completing fine. It does look like there are still a few linter errors around an incorrect error check that you do need to fix.

Ok, linters issues are fixed.

[rainest]

Ah, now we had inconvenient timing and need to fix #195 on our side 🤦