fix: marshall and unmarshall actions on endpoint permissions #148

GGabriele · 2022-03-17T12:43:48Z

For endpoint permissions's actions, Kong returns a list with GET requests, but it expects a
comma-separated string with POST/PUT requests.

This fixes the marshalling for this entity (reverting #131)

Sample script attempting to configure actions as a list:

$ echo '
id: 82bfb84b-d659-4ac1-ab29-94576f838753
name: workspace-super-admin
comment: "Full access to all endpoints in the workspace"
' | y2j |  http PUT ":8001/workspace1/rbac/roles/82bfb84b-d659-4ac1-ab29-94576f838753"

$ echo '
workspace: workspace1
endpoint: /rbac/*/*/*
actions:
    - delete
    - create
    - read
    - update
negative: true
' | y2j | http POST ":8001/workspace1/rbac/roles/82bfb84b-d659-4ac1-ab29-94576f838753/endpoints"

HTTP/1.1 200 OK
{
    "comment": "Full access to all endpoints in the workspace",
    "created_at": 1647520491,
    "id": "82bfb84b-d659-4ac1-ab29-94576f838753",
    "is_default": false,
    "name": "workspace-super-admin"
}


HTTP/1.1 201 Created
{
    "actions": [],
    "created_at": 1647520491,
    "endpoint": "/rbac/*/*/*",
    "negative": true,
    "role": {
        "id": "82bfb84b-d659-4ac1-ab29-94576f838753"
    },
    "workspace": "workspace1"
}

The actions field is ignored. While it works if the actions field is a string:

$ echo '
workspace: workspace1
endpoint: /rbac/*/*/*
actions: delete,create,read,update
negative: true
' | y2j | http POST ":8001/workspace1/rbac/roles/82bfb84b-d659-4ac1-ab29-94576f838753/endpoints"
HTTP/1.1 201 Created
{
    "actions": [
        "read",
        "delete",
        "create",
        "update"
    ],
    "created_at": 1647520694,
    "endpoint": "/rbac/*/*/*",
    "negative": true,
    "role": {
        "id": "82bfb84b-d659-4ac1-ab29-94576f838753"
    },
    "workspace": "workspace1"
}

codecov-commenter · 2022-03-17T12:45:13Z

Codecov Report

Attention: Patch coverage is 66.66667% with 13 lines in your changes missing coverage. Please review.

Project coverage is 54.14%. Comparing base (bd63e50) to head (6ae5da8).
Report is 292 commits behind head on main.

Files with missing lines	Patch %	Lines
kong/endpoint_permission_service.go	41.66%	4 Missing and 3 partials ⚠️
kong/test_utils.go	60.00%	4 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
+ Coverage   45.38%   54.14%   +8.76%     
==========================================
  Files          44       44              
  Lines        3962     3991      +29     
==========================================
+ Hits         1798     2161     +363     
+ Misses       1827     1374     -453     
- Partials      337      456     +119

Flag	Coverage Δ
2.0.5	`39.51% <12.82%> (-0.17%)`	⬇️
2.1.4	`39.33% <12.82%> (-0.17%)`	⬇️
2.2.2	`39.33% <12.82%> (-0.17%)`	⬇️
2.3.3	`39.33% <12.82%> (-0.17%)`	⬇️
2.4.0	`39.33% <12.82%> (-0.17%)`	⬇️
2.5.1	`39.33% <12.82%> (-0.17%)`	⬇️
2.6.0	`39.33% <12.82%> (-0.17%)`	⬇️
2.7.0	`39.33% <12.82%> (-0.17%)`	⬇️
2.8.0	`39.33% <12.82%> (-0.17%)`	⬇️
community	`39.51% <12.82%> (-0.17%)`	⬇️
enterprise	`52.79% <66.66%> (+7.84%)`	⬆️
enterprise-1.5.0.11	`52.79% <66.66%> (+7.84%)`	⬆️
enterprise-2.1.4.6	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.2.1.3	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.3.3.4	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.4.1.3	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.5.1.2	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.6.0.2	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.7.0.0	`51.71% <66.66%> (+7.84%)`	⬆️
enterprise-2.8.0.0	`51.71% <66.66%> (+7.84%)`	⬆️
integration	`54.14% <66.66%> (+8.76%)`	⬆️

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

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

shaneutt

Looks good overall, but given the context that we're fixing something we broke before it seems like we should be updating or adding tests which will protect this against future regressions. What are your thoughts?

GGabriele · 2022-03-17T14:26:23Z

Totally! Not sure why, but it seems that we are skipping tests on RBAC here.

Since now we have a "proper" testing pipeline in decK, what I plan to do is to keep adding more and more test cases to give us a good coverage. Like, this.

rainest · 2022-03-21T23:03:20Z

It skips them if RBAC is not available, which we expect for community. It should be on for Enterprise, but for some reason

go-kong/.ci/setup_kong.sh

Lines 63 to 66 in bd63e50

    
           export KONG_LICENSE_PATH=/tmp/license.json 
        
           export KONG_PASSWORD=kong 
        
           export KONG_ENFORCE_RBAC=on 
        
           export KONG_PORTAL=on

is apparently having no effect, despite not being under any conditional.

Dunno when/why that stopped working, but I suppose there's no reason to not just set them in the Action environment instead; they're all static anyway... Except that doesn't appear to work either.

If it did,

go-kong/kong/rbac_user_service_test.go

Lines 120 to 123 in bd63e50

    
           Actions: []*string{ 
        
           	String("create"), 
        
           	String("read"), 
        
           },

would test at least part of this.

rainest · 2022-03-21T23:45:45Z

IDK why those variables are getting ignored (they do show up in env output run before the Kong commands), but setting them on the invocation line instead of exporting them works, so whatever.

However fixing this exposes that there were several (mostly) unrelated broken tests lurking around: /~https://github.com/Kong/go-kong/runs/5636336474?check_suite_focus=true

GGabriele · 2022-03-22T07:42:53Z

Also, those variable were set even before your commit: /~https://github.com/Kong/go-kong/blob/main/.ci/setup_kong.sh#L65-L66

How do we want to proceed here? Add these vars directly in the invocation and fix those failing tests?

Kong returns a list of actions with GET requests, but it expects a comma-separated string with POST/PUT requests. This fixes the marshalling for this entity.

Instead of exporting Enterprise values, set them inline with the appropriate command. Kong wasn't seeing the exported variables previously, and wasn't actually running tests that require them.

Apply the endpoint path transform (prepend '/' if the path is '*') to all endpoint permission calls, not just Get(). Update the endpoint permission test to create a client for the test endpoint workspace and use it for all operations on that endpoint.

Provide entity type for permission that requires it. Expand default entity permissions to all actions, to not actually limit the admin's ability to interact with objects. We care that we can interact with permissions; enforcing them is the Gateway's domain.

Run the role tests in dedicated workspaces. Otherwise, they can interfere with one another or other tests. We do not want to delete the default super-admin permission.

Compare an endpoint permissions actions across the posted object and the retrieved object to ensure they are the same set. Kong's API uses a CSV string for the GET, so we must transform it.

Reduce the expected role count, as this test no longer operates in the workspace that contains the default roles.

Skip the consumer list and credential list on Enterprise. The presence of admins (which wrap hidden consumers and credentials) affects those resources' pagination. The tests require very specific pagination behavior based on the set of consumers they create, and we cannot isolate the admin in its own test, as it's used in all Enterprise tests.

rainest · 2022-03-22T21:34:21Z

How do we want to proceed here? Add these vars directly in the invocation and fix those failing tests?

Yeah--I don't really much care what the setup script looks like so long as it works, and inline does. I'm curious why that's the case (sudo would be my first guess, but sudo does inherit the environment of its caller by default, which a run of env confirms, so not sure what's up there), but not to the extent that I'll dig into it given a working alternative.

With that and a bunch of new changes unrelated to the original PR, tests pass, add (and actually run) a regression test for this, and fixes any tests that were broken.

430956c tests the original fix condition, as I understand it.

The rest of the changes fix broken tests that were hidden by Enterprise tests not actually running for the most part. Other than c35a1dd, they do not change library code, only test code.

Most fixes are unremarkable; 6ae5da8 is the exception. The implementation of admin consumers and credentials is weird, and apparently changes the way those resources paginate even though the created resources don't exist from the perspective of the admin API. We had acknowledged as much before but apparently didn't actually fix it there and didn't notice because of the skips.

Checking for specific pagination behavior isn't something anyone should need to do in practice--all you should care about as a library user is that you can pass in a next value and get the expected page. We should consider reworking our tests to not require specific pagination behavior, but IMO it's fairly low-value: this endpoint exists in community doesn't change its behavior on Enterprise, so there's no pressing reason to test it on Enterprise. The specific pages change depending on your consumer set, but that's expected--right now our test requires a specific consumer set, and it arguably shouldn't.

GGabriele requested a review from a team as a code owner March 17, 2022 12:43

GGabriele mentioned this pull request Mar 17, 2022

fix: add custom marshaller for FRBACEndpointPermission Kong/deck#619

Merged

shaneutt reviewed Mar 17, 2022

View reviewed changes

GGabriele and others added 6 commits March 22, 2022 13:21

fix: marshall and unmarshall actions on endpoint permissions

cc0505b

Kong returns a list of actions with GET requests, but it expects a comma-separated string with POST/PUT requests. This fixes the marshalling for this entity.

tests: set Enterprise variables inline

8a4af6d

Instead of exporting Enterprise values, set them inline with the appropriate command. Kong wasn't seeing the exported variables previously, and wasn't actually running tests that require them.

fix(tests) workspace role tests

4dbe7d3

Run the role tests in dedicated workspaces. Otherwise, they can interfere with one another or other tests. We do not want to delete the default super-admin permission.

chore(tests) assert workspace creation succeeded

c1557f2

rainest force-pushed the rbac_actions branch from 08dd55f to c1557f2 Compare March 22, 2022 20:23

rainest added 3 commits March 22, 2022 13:28

fix(tests) use correct config string

9b6ed00

test(rbac) ensure actions round-trip

430956c

Compare an endpoint permissions actions across the posted object and the retrieved object to ensure they are the same set. Kong's API uses a CSV string for the GET, so we must transform it.

chore(tests) change expected count

6dc55a3

Reduce the expected role count, as this test no longer operates in the workspace that contains the default roles.

rainest force-pushed the rbac_actions branch from 9691ad7 to e0aafd7 Compare March 22, 2022 20:55

rainest force-pushed the rbac_actions branch from e0aafd7 to 6ae5da8 Compare March 22, 2022 21:25

rainest requested a review from shaneutt March 22, 2022 21:34

shaneutt approved these changes Mar 25, 2022

View reviewed changes

rainest merged commit c921825 into main Mar 25, 2022

rainest deleted the rbac_actions branch March 25, 2022 16:44

rainest mentioned this pull request Jun 7, 2022

ACL tests fail on Enterprise #176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: marshall and unmarshall actions on endpoint permissions #148

fix: marshall and unmarshall actions on endpoint permissions #148

GGabriele commented Mar 17, 2022

codecov-commenter commented Mar 17, 2022 •

edited

Loading

shaneutt left a comment

GGabriele commented Mar 17, 2022

rainest commented Mar 21, 2022

rainest commented Mar 21, 2022

GGabriele commented Mar 22, 2022

rainest commented Mar 22, 2022

fix: marshall and unmarshall actions on endpoint permissions #148

fix: marshall and unmarshall actions on endpoint permissions #148

Conversation

GGabriele commented Mar 17, 2022

codecov-commenter commented Mar 17, 2022 • edited Loading

Codecov Report

shaneutt left a comment

Choose a reason for hiding this comment

GGabriele commented Mar 17, 2022

rainest commented Mar 21, 2022

rainest commented Mar 21, 2022

GGabriele commented Mar 22, 2022

rainest commented Mar 22, 2022

codecov-commenter commented Mar 17, 2022 •

edited

Loading