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 to validate via Kong API #502

Merged
merged 20 commits into from
Jan 19, 2022
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 26 additions & 10 deletions cmd/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,20 @@ var (
assumeYes bool
)

func validateEntity(ctx context.Context, kongClient *kong.Client, entityType string, entity interface{}) (bool, error) {
errWrap := "validate entity '%s': %s"
endpoint := fmt.Sprintf("/schemas/%s/validate", entityType)
req, err := kongClient.NewRequest(http.MethodPost, endpoint, nil, entity)
if err != nil {
return false, fmt.Errorf(errWrap, entityType, err)
}
resp, err := kongClient.Do(ctx, req, nil)
if err != nil {
return false, fmt.Errorf(errWrap, entityType, err)
}
return resp.StatusCode == http.StatusOK, nil
}

// workspaceExists checks if workspace exists in Kong.
func workspaceExists(ctx context.Context, config utils.KongClientConfig, workspaceName string) (bool, error) {
rootConfig := config.ForWorkspace("")
Expand All @@ -44,13 +58,22 @@ func workspaceExists(ctx context.Context, config utils.KongClientConfig, workspa
return false, err
}

exists, err := rootClient.Workspaces.ExistsByName(ctx, &workspaceName)
exists, err := rootClient.Workspaces.Exists(ctx, &workspaceName)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rainest I think the ExistsByName method is not behaving as we expected here. I did create a teamA workspace in Kong, but this method was still returning false when checking it.

I think the Exists method is the one we are looking for, but wanted to check it with you.

$ http :8001/workspaces
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 391
Content-Type: application/json; charset=utf-8
Date: Thu, 14 Oct 2021 07:54:08 GMT
Server: kong/2.5.1
X-Kong-Admin-Latency: 1

{
    "data": [
        {
            "comment": null,
            "config": {},
            "created_at": 1634196302,
            "id": "a8cf1f50-e9bd-40a6-a3ec-7477abca297a",
            "meta": {},
            "name": "teamA"
        },
        {
            "comment": null,
            "config": {},
            "created_at": 1633083360,
            "id": "b16c7d76-e917-47b2-9449-489be71b13cf",
            "meta": {},
            "name": "default"
        }
    ],
    "next": null
}

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep this out of scope from this PR. Can you open an issue to track this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since it uses workspaceExists as well, should we leave out from this PR the implementation of the --workspace flag entirely then?

Copy link
Member

Choose a reason for hiding this comment

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

yes, let's do that. We can have a follow-up PR to add support for workspaces. cc @rainest

Copy link
Contributor

Choose a reason for hiding this comment

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

We don't want to change this back to Exists(). Exists() only works if you have access to the default workspace. Most workspace existence checks should use ExistsByName(): Exists() is only still around to avoid breaking legacy code and because ExistsByName() cannot take IDs as input.

If you're passing the name in you should be fine. Can you double-check that the workspace is in fact available? If so, and you're getting errors, can you show us what you're seeing in your admin API access log?

For me, after changing it back to ExistsByName():

11:20:57-0700 yagody $ /tmp/bdeck validate --online
validate entity 'services': HTTP status 400 (message: "schema violation (host: must not have a port)")

11:20:59-0700 yagody $ /tmp/bdeck validate --online --workspace foo
Warning: Workspace 'foo' specified via --workspace flag is different from workspace '' found in state file(s).
workspace doesn't exist: foo

11:21:15-0700 yagody $ http localhost:8001/workspaces name=foo
HTTP/1.1 201 Created

11:22:15-0700 yagody $ /tmp/bdeck validate --online --workspace foo
Warning: Workspace 'foo' specified via --workspace flag is different from workspace '' found in state file(s).
validate entity 'services': HTTP status 400 (message: "schema violation (host: must not have a port)")
# tail -3 /kong/servroot/logs/admin_access.log 
172.23.0.1 - - [18/Oct/2021:18:22:20 +0000] "GET /foo/workspaces/foo HTTP/1.1" 200 692 "-" "Go-http-client/1.1"
172.23.0.1 - - [18/Oct/2021:18:22:20 +0000] "POST /foo/schemas/services/validate HTTP/1.1" 400 135 "-" "Go-http-client/1.1"
172.23.0.1 - - [18/Oct/2021:18:22:20 +0000] "POST /foo/schemas/routes/validate HTTP/1.1" 200 42 "-" "Go-http-client/1.1"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe something is wrong with my environment (running it from docker-kong/compose)? I switched back to use ExistsByName() and following your steps doesn't work for me.

$ deck validate --online --workspace randomWS
Warning: Workspace 'randomWS' specified via --workspace flag is different from workspace '' found in state file(s).
workspace doesn't exist: randomWS
exit status 1

$ http localhost:8001/workspaces name=randomWS
HTTP/1.1 201 Created

$ deck validate --online --workspace randomWS
Warning: Workspace 'randomWS' specified via --workspace flag is different from workspace '' found in state file(s).
workspace doesn't exist: randomWS
exit status 1

I see the backend failing here:

kong_1                | 192.168.112.1 - - [19/Oct/2021:12:50:36 +0000] "POST /workspaces HTTP/1.1" 201 124 "-" "HTTPie/2.4.0"
kong_1                | 192.168.112.1 - - [19/Oct/2021:12:50:40 +0000] "GET / HTTP/1.1" 200 9967 "-" "Go-http-client/1.1"
kong_1                | 192.168.112.1 - - [19/Oct/2021:12:50:40 +0000] "GET /randomWS/workspaces/randomWS HTTP/1.1" 404 23 "-" "Go-http-client/1.1"

Kong version is:

$ http localhost:8001 | jq '.version'
"2.5.1"

Meanwhile, I reverted the ExistsByName change

Copy link
Contributor

Choose a reason for hiding this comment

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

@hbagdi are you able to replicate this 404 with ExistsByName()? I can't fathom what would cause this in Gabriele's environment, but we do want to keep this as-is, else it'll require access to default.

Absent any obvious cause, I tried to see if this happened with workspaces whose name contained capitals or if it was something that happened in free mode, but neither results in a 404 for me.

The endpoint in question doesn't have any special logic in the API Lua source, it's just auto-generated. There could be something that explains it in the workspace entity lookup code, but I can't say where to look without replication. Asked FTT but they may not have any ideas either.

If you can't replicate, I'd recommend we leave this with ExistsByName() and chalk up the observed issue as a fluke pending reports from others. If you can replicate, I suppose we then get to go dig through Lua source to see exactly where the lookup fails.

Upstream tests of the function are in place and don't fail, so as far as we know it works in general and this issue has to be some edge case we don't account for. If we can figure out what that is, we should address it in the upstream implementation.

if err != nil {
return false, fmt.Errorf("checking if workspace exists: %w", err)
}
return exists, nil
}

func getWorkspaceName(workspace string, targetContent *file.Content) string {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func getWorkspaceName(workspace string, targetContent *file.Content) string {
func getWorkspaceName(workspaceFlag string, targetContent *file.Content) string {

More self-documenting code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done :)

if workspace != targetContent.Workspace && workspace != "" {
cprint.DeletePrintf("Warning: Workspace '%v' specified via --workspace flag is "+
"different from workspace '%v' found in state file(s).\n", workspace, targetContent.Workspace)
return workspace
}
return targetContent.Workspace
}

func syncMain(ctx context.Context, filenames []string, dry bool, parallelism,
delay int, workspace string) error {

Expand All @@ -65,16 +88,9 @@ func syncMain(ctx context.Context, filenames []string, dry bool, parallelism,
return err
}

var wsConfig utils.KongClientConfig
var workspaceName string
// prepare to read the current state from Kong
if workspace != targetContent.Workspace && workspace != "" {
cprint.DeletePrintf("Warning: Workspace '%v' specified via --workspace flag is "+
"different from workspace '%v' found in state file(s).\n", workspace, targetContent.Workspace)
workspaceName = workspace
} else {
workspaceName = targetContent.Workspace
}
var wsConfig utils.KongClientConfig
workspaceName := getWorkspaceName(workspace, targetContent)
wsConfig = rootConfig.ForWorkspace(workspaceName)

// load Kong version after workspace
Expand Down
184 changes: 181 additions & 3 deletions cmd/validate.go
Original file line number Diff line number Diff line change
@@ -1,29 +1,39 @@
package cmd

import (
"context"
"fmt"
"os"
"reflect"
"sync"

"github.com/kong/deck/file"
"github.com/kong/deck/state"
"github.com/kong/deck/utils"
"github.com/kong/go-kong/kong"
"github.com/spf13/cobra"
)

var (
validateCmdKongStateFile []string
validateCmdRBACResourcesOnly bool
validateOnline bool
validateWorkspace string
)

var maxConcurrency = 100
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hbagdi what value you think would be most appropriate here? For now I pseudo-randomly set it to 100, which is slightly lower than the nginx default of 128.

Another option would be to have a default value but make it configurable with a CLI flag.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Let's make it configurable from the get-go. Add a --parallelism flag to the validate command.


// validateCmd represents the diff command
var validateCmd = &cobra.Command{
Use: "validate",
Short: "Validate the state file",
Long: `The validate command reads the state file and ensures validity.

It reads all the specified state files and reports YAML/JSON
parsing issues. It also checks for foreign relationships
and alerts if there are broken relationships, or missing links present.

No communication takes places between decK and Kong during the execution of
this command.
this command unless --online flag is used.
`,
Args: validateNoArgs,
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -50,11 +60,19 @@ this command.
return err
}
// this catches foreign relation errors
_, err = state.Get(rawState)
ks, err := state.Get(rawState)
if err != nil {
return err
}

if validateOnline {
if errs := validateWithKong(cmd, ks, targetContent); errs != nil {
for _, e := range errs {
fmt.Println(e)
hbagdi marked this conversation as resolved.
Show resolved Hide resolved
}
os.Exit(1)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

My bad, we don't print errors, we return the error from this command and cobra prints and exists appropriately.
You will need to implement an error type that can hold multiple errors and print those.
Similar to t

return nil
},
PreRunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -66,6 +84,159 @@ this command.
},
}

func validateEntities(ctx context.Context, obj interface{}, kongClient *kong.Client, entityType string) []error {
Copy link
Member

Choose a reason for hiding this comment

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

Most of the functions that have been added in this PR should belong to either the util package or a new kong/validation package. cmd package must not have such business logic within itself.
Please move these functions.

entities := callGetAll(obj)
errors := []error{}

// create a buffer of channels. Creation of new coroutines
// are allowed only if the buffer is not full.
chanBuff := make(chan struct{}, maxConcurrency)

var wg sync.WaitGroup
wg.Add(entities.Len())
hbagdi marked this conversation as resolved.
Show resolved Hide resolved
// each coroutine will append on a slice of errors.
// since slices are not thread-safe, let's add a mutex
// to handle access to the slice.
mu := &sync.Mutex{}
for i := 0; i < entities.Len(); i++ {
// reserve a slot
chanBuff <- struct{}{}
go func(i int) {
defer wg.Done()
// release a slot when completed
defer func() { <-chanBuff }()
_, err := validateEntity(ctx, kongClient, entityType, entities.Index(i).Interface())
if err != nil {
mu.Lock()
errors = append(errors, err)
mu.Unlock()
}
}(i)
}
wg.Wait()
return errors
}

func validateWithKong(cmd *cobra.Command, ks *state.KongState, targetContent *file.Content) []error {
ctx := cmd.Context()
// make sure we are able to connect to Kong
_, err := fetchKongVersion(ctx, rootConfig)
if err != nil {
return []error{fmt.Errorf("couldn't fetch Kong version: %w", err)}
}

// check if workspace exists
rainest marked this conversation as resolved.
Show resolved Hide resolved
workspaceName := getWorkspaceName(validateWorkspace, targetContent)
workspaceExists, err := workspaceExists(ctx, rootConfig, workspaceName)
if err != nil {
return []error{err}
}
if !workspaceExists {
Copy link
Member

Choose a reason for hiding this comment

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

What if decK is used against the open-source version of Kong (which has no concept of workspace)?
All workspace specific code must be executed only when the user supplies a workspace.

return []error{fmt.Errorf("workspace doesn't exist: %s", workspaceName)}
}
Copy link
Member

Choose a reason for hiding this comment

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

My one last product review comment: it must be possible for the user to validate their configuration without the existence of a workspace. It is usual for our users to validate their configuration before applying. This is a blocker for merging.
cc @rainest can help figure out alternative solutions

Copy link
Contributor

Choose a reason for hiding this comment

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

Some workspace has to exist for you to check :)

You could have this fall back to trying against default if the workspace requested isn't available. It may fail, as you may not have permissions--using --workspace here implies you don't, since otherwise you could omit the flag and check against default fine.

Users may try and just always supply --workspace if that's their intended target for lack of understanding that it's only needed here to work around RBAC issues, since that's a fairly nuanced point.

Copy link
Member

Choose a reason for hiding this comment

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

@rainest Can we open a Github issue with the relevant context to track this problem? We must improve this later if not in this PR.
The PR can be merged after that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So deck already assumes the default workspace always exists: https://github.com/Kong/deck/blob/main/cmd/common.go#L31-L35

What we can do is simply overwriting the user input from --workspace and set it to the default while printing a warning:

	if !workspaceExists {
		cprint.DeletePrintf("Warning: Workspace '%v' specified via --workspace flag "+
			"doesn't exist. Validating against default workspace instead.\n", workspaceName)
		workspaceName = ""
	}
$ deck validate --online
validate entity 'services': HTTP status 400 (message: "schema violation (host: must not have a port)")
$ deck validate --online --workspace randomWS
Warning: Workspace 'randomWS' specified via --workspace flag is different from workspace '' found in state file(s).
Warning: Workspace 'randomWS' specified via --workspace flag doesn't exist. Validating against default workspace instead.
validate entity 'services': HTTP status 400 (message: "schema violation (host: must not have a port)")

If that is fine to you, I can push the changes, so that we don't have to open a different issue and tackle it at a different time.

Copy link
Member

Choose a reason for hiding this comment

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

@GGabriele Please open an issue to track this problem. Can skip for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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


wsConfig := rootConfig.ForWorkspace(workspaceName)
kongClient, err := utils.GetKongClient(wsConfig)
allErr := []error{}
if err != nil {
return []error{err}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a call to fetchKongVersion using this client here? See cmd/ping.go for example.
This is to ensure that there is connectivity (network-level + authn) with Kong before we start throwing requests.


// validate RBAC resources first.
if err := validateEntities(ctx, ks.RBACEndpointPermissions, kongClient, "rbac-endpointpermission"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.RBACRoles, kongClient, "rbac-role"); err != nil {
allErr = append(allErr, err...)
}
if validateCmdRBACResourcesOnly {
return allErr
}

if err := validateEntities(ctx, ks.Services, kongClient, "services"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.ACLGroups, kongClient, "acls"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.BasicAuths, kongClient, "basicauth_credentials"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.CACertificates, kongClient, "ca_certificates"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Certificates, kongClient, "certificates"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Consumers, kongClient, "consumers"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Documents, kongClient, "documents"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.HMACAuths, kongClient, "hmacauth_credentials"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.JWTAuths, kongClient, "jwt_secrets"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.KeyAuths, kongClient, "keyauth_credentials"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Oauth2Creds, kongClient, "oauth2_credentials"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Plugins, kongClient, "plugins"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Routes, kongClient, "routes"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.SNIs, kongClient, "snis"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Targets, kongClient, "targets"); err != nil {
allErr = append(allErr, err...)
}
if err := validateEntities(ctx, ks.Upstreams, kongClient, "upstreams"); err != nil {
allErr = append(allErr, err...)
}
return allErr
}

func callGetAll(obj interface{}) reflect.Value {
// call GetAll method on entity
method := reflect.ValueOf(obj).MethodByName("GetAll")
entities := method.Call([]reflect.Value{})[0].Interface()
return reflect.ValueOf(entities)
}
Copy link
Member

Choose a reason for hiding this comment

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

A further improvement to this function would be to add error as the second return value and check that in ensureGetAllMethod. An error should be returned if there is an error in looking up the method or calling the method (if any).
In the init() then, print the error and exit rather than a panic. This is to make the UX a bit better.
Printing a helpful error and exiting is a better user experience than a CLI that does a stack dump on the user.
Even better, an error that is actionable. If the method lookup fails, then inform the user in the output that they should file a bug with Kong Inc.

This is not a blocker for merging.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I totally agree an obscure fail will not help users. I implemented this (example by forcing a fail):

$ deck validate --online
GetAll() method not found for *state.ServicesCollection. Please file a bug with Kong Inc.
exit status 1

Let me know what you think!


// ensureGetAllMethod ensures at init time that `GetAll()` method exists on the relevant structs.
// If the method doesn't exist, the code will panic. This increases the likelihood of catching such an
// error during manual testing.
func ensureGetAllMethods() {
// let's make sure ASAP that all resources have the expected GetAll method
dummyEmptyState, _ := state.NewKongState()
callGetAll(dummyEmptyState.Services)
callGetAll(dummyEmptyState.ACLGroups)
callGetAll(dummyEmptyState.BasicAuths)
callGetAll(dummyEmptyState.CACertificates)
callGetAll(dummyEmptyState.Certificates)
callGetAll(dummyEmptyState.Consumers)
callGetAll(dummyEmptyState.Documents)
callGetAll(dummyEmptyState.HMACAuths)
callGetAll(dummyEmptyState.JWTAuths)
callGetAll(dummyEmptyState.KeyAuths)
callGetAll(dummyEmptyState.Oauth2Creds)
callGetAll(dummyEmptyState.Plugins)
callGetAll(dummyEmptyState.Routes)
callGetAll(dummyEmptyState.SNIs)
callGetAll(dummyEmptyState.Targets)
callGetAll(dummyEmptyState.Upstreams)
hbagdi marked this conversation as resolved.
Show resolved Hide resolved
callGetAll(dummyEmptyState.RBACEndpointPermissions)
callGetAll(dummyEmptyState.RBACRoles)
}

func init() {
rootCmd.AddCommand(validateCmd)
validateCmd.Flags().BoolVar(&validateCmdRBACResourcesOnly, "rbac-resources-only",
Expand All @@ -74,4 +245,11 @@ func init() {
"state", "s", []string{"kong.yaml"}, "file(s) containing Kong's configuration.\n"+
"This flag can be specified multiple times for multiple files.\n"+
"Use '-' to read from stdin.")
validateCmd.Flags().BoolVar(&validateOnline, "online",
false, "perform schema validation against Kong API.")
GGabriele marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
false, "perform schema validation against Kong API.")
false, "perform validations against Kong API. When this flag is used, validation is done via communication with Kong. This increases the time for validation but catches significant errors. No resource is created in Kong.")

validateCmd.Flags().StringVarP(&validateWorkspace, "workspace", "w",
"", "validate configuration of a specific workspace "+
"(Kong Enterprise only).\n"+
"This takes precedence over _workspace fields in state files.")
ensureGetAllMethods()
}
1 change: 1 addition & 0 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ func registerSignalHandler() context.Context {
signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)

go func() {
defer signal.Stop(sigs)
GGabriele marked this conversation as resolved.
Show resolved Hide resolved
sig := <-sigs
fmt.Println("received", sig, ", terminating...")
cancel()
Expand Down