Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add support for Kong Enterprise RBAC resources #276

Merged
merged 8 commits into from
Mar 3, 2021

Conversation

hbagdi
Copy link
Member

@hbagdi hbagdi commented Feb 25, 2021

This takes the patch from #271 and adds in some machinery to avoid any potential breakage for existing users.
Please see #271 (comment) for reasoning behind additional commits on top of the initial patch.
Please review commit-by-commit.

Fix #270

Tyler Rivera and others added 4 commits February 21, 2021 22:28
- diff/diff.go: fix races by adding barriers
- diff/rbac_role.go: clean up error and use Identifier() instead of ID
- file/builder.go: minor refactor for organizing code for another upcoming feature
- file/types_test.go: remove leftover debug statements
- state/rbac_role*.go: do not allow empty names in RBAC role kong's
  schema has the same validation; also fixes test cases accordingly
- file/writer.go: zeroOutID when possible
- solver/rbac_endpoint_permissions.go: use PATCH instead of POST for
  update operations because a POST violates DB schema and errors out
Exporting RBAC resources by default is a breaking change because not all
flavors of Kong have RBAC resources. Furthermore, not all users would
like to manage RBAC resources using decK.
--rbac-resources-only flag has been implemented for diff, sync,
validate and dump command.

When this flag is specified, only RBAC resources are expected and
synced. Specifying any other resources will result in an error.
Also, if RBAC resources are provided without providing this flag, decK will
error out.
@hbagdi hbagdi requested a review from a team as a code owner February 25, 2021 20:49
@hbagdi
Copy link
Member Author

hbagdi commented Feb 25, 2021

cc @tjrivera

@rainest
Copy link
Contributor

rainest commented Feb 25, 2021

Issues and points of interest found in user/black box testing:

  • Endpoint permissions with a non-existent workspace simply fail and no _workspace simply fail:
    while processing event: {Create} rbac-endpointpermission 22021df1-864f-42db-a477-74860a659c87-nonono-* failed: HTTP status 404 (message: "Workspace nonono does not exist")
    
    This is probably expected--decK normally requires you handle all transactions on a per-workspace basis. This poses a bit of a challenge since roles with permissions in multiple workspaces are supported, albeit not perfectly (the GUI doesn't really handle them effectively).
  • reset lacks the --rbac-resources-only flag. It does not reset RBAC resources when run without it.
  • Roles with no name cause a segfault: no-name.yaml.txt
  • Permissions with no workspace cause a segfault: no-workspace.yaml.txt
  • Permission deletes do not work:
    deleting rbac-endpointpermission 592e8540-25ab-4105-820e-e937506d0630-whatever-/acls
    DELETE /whatever/rbac/roles/592e8540-25ab-4105-820e-e937506d0630/endpoints/whatever//acls HTTP/1.1
    HTTP/1.1 204 No Content
    
    This "succeeds" but doesn't delete the permission. Extra slash apparently breaks it.

@rainest
Copy link
Contributor

rainest commented Feb 26, 2021

Some of the above are easy to fix: #278

  • Roles without names segfaulting isn't easy to address within the existing structure of file/builder.go. We don't have any existing pattern of logging and skipping invalid resources or returning errors when this happens, and other resources, e.g. services appear to have a similar segfault path. We could do something like this:
            for _, r := range b.targetContent.RBACRoles {
                    r := r
                    if utils.Empty(r.ID) {
    +                       if utils.Empty(r.Name) {
    +                               b.err = errors.New("RBAC role lacks name")
    +                               return
    +                       }
                            role, err := b.currentState.RBACRoles.Get(*r.Name)
    
    though that's not particularly graceful, since it just fails state building entirely and doesn't proceed onward. Functionally that's equivalent to the segfault, though it's a bit less ugly.
  • DELETEs not working is due to some odd admin API behavior. Permission admin API endpoints include the permission endpoint in the path, and the permission sometimes includes a leading slash (e.g. /acls) and sometimes does not (* is the only such endpoint AFAIK). Calls from Manager omit the leading slash, e.g. DELETE /rbac/roles/a79bdf2b-8844-424d-b6fc-97490cc6fe1d/endpoints/default/acls/*, whereas we include it DELETE /rbac/roles/a79bdf2b-8844-424d-b6fc-97490cc6fe1d/endpoints/default//acls/*. Our PATCHes also use the double slash, but for some reason, those succeed, whereas DELETEs do not (rather, they 204, but the permission remains). We can't just chop that leading slash off entirely with something like:
    @@ -18,6 +19,8 @@ func rbacEndpointPermissionFromStruct(arg diff.Event) *state.RBACEndpointPermiss
                    panic("unexpected type, expected *state.RBACEndpointPermission")
            }
    
    +       trimmed := strings.TrimLeft(*ep.Endpoint, "/")
    +       ep.Endpoint = &trimmed
            return ep
     }
    
    since that makes PATCHEs start 404ing--they require the //. DELETEs succeed, though they do throw a spurious while post processing event: entity not found after. POSTs succeed, since Kong accepts a body like {"workspace":"*","endpoint":"wat","actions":"delete,create,update,read"} (it automatically appends the leading /). The discrepancy in the verb behaviors suggests we'd need to handle this in go-kong with endpoint-specific logic. We apparently already have that for GETs, but not for DELETEs.

Edit: okay, the PATCH behavior appears to be because PATCH cares about the endpoint in the body only. PATCHing with {"workspace":"*","endpoint":"wat","actions":"delete,create,update,read"} will not work, but {"workspace":"*","endpoint":"/wat","actions":"delete,create,update,read"} will, regardless of whether your URL includes the double slash. 🤔

@hbagdi
Copy link
Member Author

hbagdi commented Feb 26, 2021

Thanks for the great feedback @rainest!

Endpoint permissions with a non-existent workspace simply fail and no _workspace simply fail

As you said, we can't do anything about this.

Roles with no name cause a segfault

Permissions with no workspace cause a segfault

We can take care of this using #278 (comment)

Permission deletes do not work

This is solvable. We need to change the endpoint when it is passed on to go-kong and not in the FromStruct method. That will delete the permission in Kong and not cause the postProcess error.

Travis Raines added 4 commits February 27, 2021 21:07
Trim leading slashes from RBAC endpoint permissions' endpoint before
passing them to go-kong when deleting.

Including the leading slash makes go-kong build a URL with a double
slash, which will not actually delete the permission.
Add the --rbac-resources-only flag to the reset command, and delete all
RBAC roles in the state passed to reset. Deleting roles also deletes
their associated permissions.
@hbagdi
Copy link
Member Author

hbagdi commented Feb 27, 2021

@tjrivera Would you mind taking this PR for a spin and letting us know if it works for your environment and use-case?
@rainest Any other comments?

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, other than the now-fixed issues I mentioned previously, everything worked as I'd expect. Good to go from my perspective.

@tjrivera
Copy link
Contributor

tjrivera commented Mar 1, 2021

I was able to build and test this branch today. Everything works as expected. Thanks, @hbagdi, and @rainest for the help in getting this over the finish line!

@hbagdi
Copy link
Member Author

hbagdi commented Mar 3, 2021

Amazing, thanks for all your hard work and making this possible, @tjrivera!

@hbagdi hbagdi merged commit 12116b4 into main Mar 3, 2021
@hbagdi hbagdi deleted the feat/enterprise-rbac branch March 3, 2021 08:46
@hbagdi
Copy link
Member Author

hbagdi commented Mar 3, 2021

@tjrivera FYI, we will have a release of decK with a few other features in a couple of weeks and this will be included then.

@tjrivera
Copy link
Contributor

tjrivera commented Mar 3, 2021

Awesome! great to see! Thanks @hbagdi and @rainest!

AntoineJac pushed a commit that referenced this pull request Jan 23, 2024
feat: add support for Kong Enterprise RBAC resources
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Manage RBAC Entities
3 participants