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 all 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
20 changes: 11 additions & 9 deletions cmd/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ func workspaceExists(ctx context.Context, config utils.KongClientConfig, workspa
return exists, nil
}

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

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

Expand All @@ -65,16 +74,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
131 changes: 128 additions & 3 deletions cmd/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,25 +5,30 @@ import (

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

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

// 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 +55,16 @@ 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); len(errs) != 0 {
return validate.ErrorsWrapper{Errors: errs}
}
}
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 +76,107 @@ this command.
},
}

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)}
}

workspaceName := validateWorkspace
if validateWorkspace != "" {
// check if workspace exists
workspaceName := getWorkspaceName(validateWorkspace, targetContent)
workspaceExists, err := workspaceExists(ctx, rootConfig, workspaceName)
if err != nil {
return []error{err}
}
if !workspaceExists {
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)
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.


opts := validate.ValidatorOpts{
Ctx: ctx,
State: ks,
Client: kongClient,
Parallelism: validateParallelism,
RBACResourcesOnly: validateCmdRBACResourcesOnly,
}
validator := validate.NewValidator(opts)
return validator.Validate()
}

// 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 := utils.CallGetAll(dummyEmptyState.Services); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.ACLGroups); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.BasicAuths); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.CACertificates); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Certificates); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Consumers); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Documents); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.HMACAuths); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.JWTAuths); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.KeyAuths); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Oauth2Creds); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Plugins); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Routes); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.SNIs); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Targets); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.Upstreams); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.RBACEndpointPermissions); err != nil {
return err
}
if _, err := utils.CallGetAll(dummyEmptyState.RBACRoles); err != nil {
return err
}
return nil
}

func init() {
rootCmd.AddCommand(validateCmd)
validateCmd.Flags().BoolVar(&validateCmdRBACResourcesOnly, "rbac-resources-only",
Expand All @@ -74,4 +185,18 @@ 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 validations against Kong API. When this flag is used, validation is done\n"+
"via communication with Kong. This increases the time for validation but catches \n"+
"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.")
validateCmd.Flags().IntVar(&validateParallelism, "parallelism",
10, "Maximum number of concurrent requests to Kong.")

if err := ensureGetAllMethods(); err != nil {
panic(err.Error())
}
}
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
14 changes: 14 additions & 0 deletions utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/url"
"os"
"path/filepath"
"reflect"
"regexp"
"strings"
)
Expand Down Expand Up @@ -52,3 +53,16 @@ func NameToFilename(name string) string {
func FilenameToName(filename string) string {
return strings.ReplaceAll(filename, url.PathEscape(string(os.PathSeparator)), string(os.PathSeparator))
}

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 type '%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
}
Loading