-
Notifications
You must be signed in to change notification settings - Fork 129
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
Changes from 10 commits
231e0e7
b91c844
ef450c4
4136209
422f066
bc31676
e25214e
a4c603e
9c3ec69
94ed516
ddeb657
7523bfd
a6b0a11
34fb225
3e13393
6319328
536ba04
809e7d3
9c68a3b
69135ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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("") | ||||||
|
@@ -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) | ||||||
if err != nil { | ||||||
return false, fmt.Errorf("checking if workspace exists: %w", err) | ||||||
} | ||||||
return exists, nil | ||||||
} | ||||||
|
||||||
func getWorkspaceName(workspace string, targetContent *file.Content) string { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
More self-documenting code. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||
|
||||||
|
@@ -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 | ||||||
|
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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Another option would be to have a default value but make it configurable with a CLI flag. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's make it configurable from the get-go. Add a |
||||||
|
||||||
// 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 { | ||||||
|
@@ -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) | ||||||
} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
return nil | ||||||
}, | ||||||
PreRunE: func(cmd *cobra.Command, args []string) error { | ||||||
|
@@ -66,6 +84,159 @@ this command. | |||||
}, | ||||||
} | ||||||
|
||||||
func validateEntities(ctx context.Context, obj interface{}, kongClient *kong.Client, entityType string) []error { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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 { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? |
||||||
return []error{fmt.Errorf("workspace doesn't exist: %s", workspaceName)} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Users may try and just always supply There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So What we can do is simply overwriting the user input from
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a call to |
||||||
|
||||||
// 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) | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). This is not a blocker for merging. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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):
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", | ||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
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() | ||||||
} |
There was a problem hiding this comment.
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 ateamA
workspace in Kong, but this method was still returningfalse
when checking it.I think the
Exists
method is the one we are looking for, but wanted to check it with you.There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 useExistsByName()
:Exists()
is only still around to avoid breaking legacy code and becauseExistsByName()
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()
:There was a problem hiding this comment.
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 useExistsByName()
and following your steps doesn't work for me.I see the backend failing here:
Kong version is:
Meanwhile, I reverted the
ExistsByName
changeThere was a problem hiding this comment.
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.