feat: add support to validate via Kong API #502
There are no files selected for viewing
| 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 | ||||||||
|
|
||||||||
| // 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 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 | ||||||||
| ks, err := state.Get(rawState) | ||||||||
| if err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
|
|
||||||||
| if validateOnline { | ||||||||
| if errs := validateWithKong(cmd, ks, targetContent); len(errs) != 0 { | ||||||||
| 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. 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,205 @@ this command. | |||||||
| }, | ||||||||
| } | ||||||||
|
|
||||||||
| func validateEntities(ctx context.Context, obj interface{}, kongClient *kong.Client, entityType string) []error { | ||||||||
|
There was a problem hiding this comment. Most of the functions that have been added in this PR should belong to either the util package or a new |
||||||||
| entities, err := callGetAll(obj) | ||||||||
| if err != nil { | ||||||||
| return []error{err} | ||||||||
| } | ||||||||
| 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. 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. 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. 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. @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. 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. @GGabriele Please open an issue to track this problem. Can skip for now. |
||||||||
|
|
||||||||
| wsConfig := rootConfig.ForWorkspace(workspaceName) | ||||||||
| kongClient, err := utils.GetKongClient(wsConfig) | ||||||||
| allErr := []error{} | ||||||||
| if err != nil { | ||||||||
| return []error{err} | ||||||||
| } | ||||||||
|
There was a problem hiding this comment. 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, error) { | ||||||||
| // call GetAll method on entity | ||||||||
| var result reflect.Value | ||||||||
| method := reflect.ValueOf(obj).MethodByName("GetAll") | ||||||||
| if !method.IsValid() { | ||||||||
| return result, fmt.Errorf("GetAll() method not found for %v. "+ | ||||||||
| "Please file a bug with Kong Inc.", reflect.ValueOf(obj).Type()) | ||||||||
| } | ||||||||
| entities := method.Call([]reflect.Value{})[0].Interface() | ||||||||
| result = reflect.ValueOf(entities) | ||||||||
| return result, nil | ||||||||
| } | ||||||||
|
There was a problem hiding this comment. 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. 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() error { | ||||||||
| // let's make sure ASAP that all resources have the expected GetAll method | ||||||||
| dummyEmptyState, _ := state.NewKongState() | ||||||||
| if _, err := callGetAll(dummyEmptyState.Services); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.ACLGroups); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.BasicAuths); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.CACertificates); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Certificates); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Consumers); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Documents); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.HMACAuths); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.JWTAuths); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.KeyAuths); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Oauth2Creds); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Plugins); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Routes); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.SNIs); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Targets); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.Upstreams); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.RBACEndpointPermissions); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| if _, err := callGetAll(dummyEmptyState.RBACRoles); err != nil { | ||||||||
| return err | ||||||||
| } | ||||||||
| return nil | ||||||||
| } | ||||||||
|
|
||||||||
| func init() { | ||||||||
| rootCmd.AddCommand(validateCmd) | ||||||||
| validateCmd.Flags().BoolVar(&validateCmdRBACResourcesOnly, "rbac-resources-only", | ||||||||
|
|
@@ -74,4 +291,14 @@ 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.
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.") | ||||||||
| if err := ensureGetAllMethods(); err != nil { | ||||||||
| fmt.Println(err) | ||||||||
| os.Exit(1) | ||||||||
|
There was a problem hiding this comment.
Suggested change
|
||||||||
| } | ||||||||
| } | ||||||||
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.
@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 of128.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 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
--parallelismflag to the validate command.