feat: add support to validate via Kong API #502

GGabriele · 2021-10-05T11:52:05Z

Hi! In order to get more familiarity with the tool and the codebase, I decided to give it a try and work on the proposal discussed in #190

Sample usage:

$ deck validate --help
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.

When ran with the --use-kong-api flag, it also uses the 'validate' endpoint in Kong
to validate entities configuration.

Usage:
  deck validate [flags]

Flags:
  -h, --help                  help for validate
      --rbac-resources-only   indicate that the state file(s) contains RBAC resources only (Kong Enterprise only).
  -s, --state strings         file(s) containing Kong's configuration.
                              This flag can be specified multiple times for multiple files.
                              Use '-' to read from stdin. (default [kong.yaml])
      --use-kong-api          perform schema validation against Kong API.

Sample input:

services:
- name: svc1
  host: mockbin.org:80
  tags:
  - team-svc1
  routes:
  - name: r1
    https_redirect_status_code: 301
    paths:
    - /r1
certificates:
- id: 13c562a1-191c-4464-9b18-e5222b46035b
  cert: |
    -----BEGIN CERTIFICATE-----
    -----END CERTIFICATE-----
  key: |
    -----BEGIN PRIVATE KEY-----
    -----END PRIVATE KEY-----
  tags:
  - cloudops-managed
  snis:
  - name: demo1.example.com:123
  - name: demo2.example.com
  - name: demo3.example.com
plugins:
- name: prometheus
  enabled: true
  protocols:
  - httpss
  - https

Sample output:

$ deck validate --use-kong-api
services: HTTP status 400 (message: "schema violation (host: must not have a port)")
certificates: HTTP status 400 (message: "2 schema violations (cert: invalid certificate: x509.new: asn1/tasn_dec.c:309:error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error; key: invalid key: pkey.new:load_key: asn1/tasn_dec.c:309:error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error)")
plugins: HTTP status 400 (message: "schema violation (protocols.1: expected one of: grpc, grpcs, http, https, tcp, tls, udp)")
snis: HTTP status 400 (message: "schema violation (name: must not have a port)")

Let me know what you think! :)

hbagdi · 2021-10-05T16:48:01Z

So the goal of #190 is to do validation of data within the yaml file.

For example, the following is a valid input as per deck validate :

services:
- name: svc1
  host: mockbin.org:80
  tags:
  - team-svc1

But if you try to do deck sync, you get the following:

Error: 1 errors occurred:
        while processing event: {Create} service svc1 failed: HTTP status 400 (message: "schema violation (host: must not have a port)")

The goal here is to take the entities in the file and then validate them against Kong's Admin API.
See comments for more direction.

cmd/common.go

cmd/validate.go

GGabriele · 2021-10-06T07:54:39Z

@hbagdi thanks for the feedbacks and tips! I just pushed some changes.

Sample input:

services:
- name: svc1
  host: mockbin.org:80
  tags:
  - team-svc1
  routes:
  - name: r1
    https_redirect_status_code: 301
    paths:
    - /r1
certificates:
- id: 13c562a1-191c-4464-9b18-e5222b46035b
  cert: |
    -----BEGIN CERTIFICATE-----
    -----END CERTIFICATE-----
  key: |
    -----BEGIN PRIVATE KEY-----
    -----END PRIVATE KEY-----
  tags:
  - cloudops-managed
  snis:
  - name: demo1.example.com:123
  - name: demo2.example.com
  - name: demo3.example.com
plugins:
- name: prometheus
  enabled: true
  protocols:
  - httpss
  - https

Sample output:

$ deck validate --use-kong-api
services: HTTP status 400 (message: "schema violation (host: must not have a port)")
certificates: HTTP status 400 (message: "2 schema violations (cert: invalid certificate: x509.new: asn1/tasn_dec.c:309:error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error; key: invalid key: pkey.new:load_key: asn1/tasn_dec.c:309:error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error)")
plugins: HTTP status 400 (message: "schema violation (protocols.1: expected one of: grpc, grpcs, http, https, tcp, tls, udp)")
snis: HTTP status 400 (message: "schema violation (name: must not have a port)")

hbagdi

The approach is correct here. We know need to polish the code to get to merge.

cmd/common.go

main.go

cmd/validate.go

hbagdi · 2021-10-06T16:10:26Z

cmd/validate.go

+	validateEntities(ctx, ks.Routes, kongClient, "routes")
+	validateEntities(ctx, ks.SNIs, kongClient, "snis")
+	validateEntities(ctx, ks.Targets, kongClient, "targets")
+	validateEntities(ctx, ks.Upstreams, kongClient, "upstreams")


RBAC resources are missing here. Can you include those?

Ops, missed this...what is/are the db entities associated to RBAC resources? I couldn't find them :\

Roles and endpoint permissions (there are some others, but we lack support for them).

deck/dump/dump.go

Lines 221 to 240 in 02208b2

func getEnterpriseRBACConfiguration(ctx context.Context, group *errgroup.Group,

client *kong.Client, state *utils.KongRawState) {

group.Go(func() error {

roles, err := GetAllRBACRoles(ctx, client)

if err != nil {

return fmt.Errorf("roles: %w", err)

}

state.RBACRoles = roles

return nil

})

group.Go(func() error {

eps, err := GetAllRBACREndpointPermissions(ctx, client)

if err != nil {

return fmt.Errorf("eps: %w", err)

}

state.RBACEndpointPermissions = eps

return nil

})

}

is probably the best jumping-off point to find the associated functions and client structs.

https://docs.konghq.com/enterprise/2.5.x/admin-api/rbac/reference/#add-a-role
https://docs.konghq.com/enterprise/2.5.x/admin-api/rbac/reference/#add-a-role-endpoint-permission

Thanks for the pointers!

cmd/validate.go

hbagdi · 2021-10-06T16:54:25Z

cmd/validate.go

+			defer wg.Done()
+			output := validate(ctx, values.Index(i).Interface(), kongClient, entityType)
+			if output != "" {
+				fmt.Println(output)


Don't print to stdout but instead collect all of these as errors and bubble up all the way and then print them in the RunE fn.

I'm not 100% sure my last change is what you were envisioning, but let me know otherwise!

hbagdi · 2021-10-06T16:56:39Z

cmd/validate.go

+func validateEntities(ctx context.Context, obj interface{}, kongClient *kong.Client, entityType string) {
+	// call GetAll on entity
+	method := reflect.ValueOf(obj).MethodByName("GetAll")
+	entities := method.Call([]reflect.Value{})[0].Interface()


Since there are no tests, we will need to figure out a way to catch an error here.
Can you use init() function to invoke code like this to ensure that it doesn't error/panic?

Not sure if I got you here, can you elaborate a bit please?

We can also do a bit more of checks like this:

method := reflect.ValueOf(obj).MethodByName("GetAll") if !method.IsValid() { // bla } entities := method.Call([]reflect.Value{})[0].Interface() values := reflect.ValueOf(entities) if values.Kind() != reflect.Slice { // bla }

The goal here is to statically ensure that all these state structs have the GetAll method (cause otherwise the code will panic at runtime).

One way to do that is to call all these reflect functions within an init function and then keep the references to the method in an array/map. Now, if there is a panic, it will happen on every call to main(). The likelihood of us catching that is much higher this way (until we have extensive testing happening for this project).

I guess you mean something like this?

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) } func validateGetAllMethod() { dummyEmptyState, _ := state.NewKongState() callGetAll(dummyEmptyState.Services) callGetAll(dummyEmptyState.ACLGroups) ... } func init() { rootCmd.AddCommand(validateCmd) ... validateGetAllMethod() }

I haven't found a nice way to store the method reference and call it later at validation time though :\ (without having to create a proper state at init time)

(without having to create a proper state at init time)

Why not do that?

So Something like this:

var store := map[string]somestruct{}

func init(){
// get a reference to the right method, throw a panic if the method doesn't exist
method:=getMethod()
store[entityType]= somestruct{ method:method, other: releavant-info}
}

And then at runtime, use the store to call the methods?

Yeah initially I modelled exactly that, but this is going to implicitly add validation on the configuration file for all commands, right? I mean, if that's fine to you, that's fine to me :P

Also, in order to render the state, I was reusing the code of the initial validate..this would mean that the current validate behaviour will become implicit at runtime, and maybe the --online mode proposed here should become the "new default" for validate.

Or am I missing something?

hbagdi · 2021-10-07T18:08:14Z

cmd/validate.go

 	"github.com/spf13/cobra"
 )

 var (
 	validateCmdKongStateFile     []string
 	validateCmdRBACResourcesOnly bool
+	validateOnline               bool
+	validateOnlineErrors         []error


Please avoid a global state like this. Instead, change the function signatures to return []error where needed.

hbagdi · 2021-10-07T18:11:09Z

cmd/validate.go

+func validateEntities(ctx context.Context, obj interface{}, kongClient *kong.Client, entityType string) {
+	// call GetAll on entity
+	method := reflect.ValueOf(obj).MethodByName("GetAll")
+	entities := method.Call([]reflect.Value{})[0].Interface()


The goal here is to statically ensure that all these state structs have the GetAll method (cause otherwise the code will panic at runtime).

One way to do that is to call all these reflect functions within an init function and then keep the references to the method in an array/map. Now, if there is a panic, it will happen on every call to main(). The likelihood of us catching that is much higher this way (until we have extensive testing happening for this project).

hbagdi · 2021-10-07T18:11:48Z

cmd/validate.go

+		go func(i int) {
+			defer wg.Done()
+			if err := validate(ctx, values.Index(i).Interface(), kongClient, entityType); err != nil {
+				validateOnlineErrors = append(validateOnlineErrors, err)


Please returns errors from this function instead of collecting these in a global variable.

hbagdi · 2021-10-11T16:15:19Z

cmd/validate.go

+		return err
+	}
+	return nil
+}


Get rid off this function and call validateEntity from validateEntities directly.

hbagdi · 2021-10-11T16:21:12Z

cmd/validate.go

+	return reflect.ValueOf(entities)
+}
+
+func validateGetAllMethod() {


Suggested change

func validateGetAllMethod() {

// 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() {

hbagdi · 2021-10-11T16:22:28Z

cmd/validate.go

+		if validateOnline {
+			if err := validateWithKong(cmd, ks); err != nil {
+				for _, e := range err {
+					cprint.DeletePrintln(e)


Do a print without any colors. I don't think we use colors for errors anywhere else in the code-base.

hbagdi · 2021-10-11T16:23:13Z

cmd/validate.go

-No communication takes places between decK and Kong during the execution of
-this command.
+
+When ran with the --online flag, it also uses the 'validate' endpoint in Kong


Suggested change

When ran with the --online flag, it also uses the 'validate' endpoint in Kong

No communication takes places between decK and Kong during the execution of

this command unless --online flag is used.

hbagdi · 2021-10-11T16:23:50Z

cmd/validate.go

 		if err != nil {
 			return err
 		}

+		if validateOnline {
+			if err := validateWithKong(cmd, ks); err != nil {


Suggested change

if err := validateWithKong(cmd, ks); err != nil {

if errs := validateWithKong(cmd, ks); errs != nil {

For readability

cmd/validate.go

GGabriele · 2021-10-12T09:56:02Z

cmd/validate.go

 )

+var maxConcurrency = 100


@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?

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

cmd/validate.go

hbagdi

One small addition and we are ready to merge. Good work.

hbagdi · 2021-10-13T20:35:02Z

cmd/validate.go

+	allErr := []error{}
+	if err != nil {
+		return []error{err}
+	}


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.

hbagdi · 2021-10-13T20:35:57Z

We will hold for a review from @rainest for merge as well.
Travis can help us ensure that the final patch works against our enterprise stack or not.

rainest · 2021-10-13T21:50:35Z

Fully implementing the --rbac-resources-only flag and --workspace should be the only additional things we need for Enterprise, correct? I've linked info for the former in the open comment thread for that feature.

For --workspace, the implementation should look like the implementation for other commands, e.g.

deck/cmd/dump.go

Lines 154 to 156 in 02208b2

    
           dumpCmd.Flags().StringVarP(&dumpWorkspace, "workspace", "w", 
        
           	"", "dump configuration of a specific Workspace"+ 
        
           		"(Kong Enterprise only).")

to start. It's just a client configuration, so further work beyond taking the flag and setting that workspace when creating the client shouldn't be necessary.

--workspace we mostly need for less-privileged users who can't access /schemas/<whatever>/validate outside the workspace they have permission to. While the endpoint functions the same in all workspaces, users without permission to the default workspace won't be able to access it unless the validation client is workspaced.

decK's RBAC token handling is implemented globally, so nothing further is needed there: https://docs.konghq.com/deck/1.8.x/guides/kong-enterprise/#rbac

…connectivity

GGabriele · 2021-10-14T07:56:11Z

cmd/common.go

@@ -58,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)


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

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

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

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

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"

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

@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.

cmd/validate.go

hbagdi · 2021-10-19T18:25:21Z

cmd/common.go

@@ -51,6 +65,15 @@ func workspaceExists(ctx context.Context, config utils.KongClientConfig, workspa
 	return exists, nil
 }

+func getWorkspaceName(workspace string, targetContent *file.Content) string {


Suggested change

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

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

More self-documenting code.

hbagdi · 2021-10-19T18:31:01Z

cmd/validate.go

+	method := reflect.ValueOf(obj).MethodByName("GetAll")
+	entities := method.Call([]reflect.Value{})[0].Interface()
+	return reflect.ValueOf(entities)
+}


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.

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!

hbagdi · 2021-10-19T18:34:16Z

cmd/validate.go

+	}
+	if !workspaceExists {
+		return []error{fmt.Errorf("workspace doesn't exist: %s", workspaceName)}
+	}


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

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.

@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.

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.

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

hbagdi · 2022-01-14T19:47:52Z

cmd/validate.go

 )

+var maxConcurrency = 100


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

hbagdi · 2022-01-14T19:51:54Z

cmd/validate.go

+				}
+				os.Exit(1)
+			}
+		}


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

hbagdi · 2022-01-14T19:55:14Z

cmd/validate.go

@@ -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.")


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.")

hbagdi · 2022-01-14T19:56:56Z

cmd/validate.go

@@ -66,6 +84,205 @@ this command.
 	},
 }

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


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.

hbagdi · 2022-01-14T20:01:30Z

cmd/validate.go

+	}
+	if !workspaceExists {
+		return []error{fmt.Errorf("workspace doesn't exist: %s", workspaceName)}
+	}


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

hbagdi · 2022-01-14T20:03:11Z

cmd/validate.go

+	if err != nil {
+		return []error{err}
+	}
+	if !workspaceExists {


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.

codecov-commenter · 2022-01-15T13:34:39Z

Codecov Report

Merging #502 (9c68a3b) into main (8643ba5) will increase coverage by 0.89%.
The diff coverage is 13.11%.

@@            Coverage Diff             @@
##             main     #502      +/-   ##
==========================================
+ Coverage   50.42%   51.31%   +0.89%     
==========================================
  Files          72       73       +1     
  Lines        7856     7857       +1     
==========================================
+ Hits         3961     4032      +71     
+ Misses       3546     3456      -90     
- Partials      349      369      +20

Impacted Files	Coverage Δ
cmd/common.go	`6.22% <0.00%> (+6.22%)`	⬆️
utils/utils.go	`63.33% <0.00%> (-36.67%)`	⬇️
cmd/validate.go	`16.19% <15.68%> (-3.81%)`	⬇️
utils/types.go	`21.55% <0.00%> (-3.71%)`	⬇️
file/builder.go	`55.29% <0.00%> (-0.20%)`	⬇️
diff/diff.go	`0.00% <0.00%> (ø)`
diff/order.go	`79.16% <0.00%> (ø)`
cmd/root.go	`55.49% <0.00%> (+0.49%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8643ba5...9c68a3b. Read the comment docs.

GGabriele · 2022-01-15T13:37:47Z

@hbagdi I believe I addressed all your comments. Let me know what you think or if I missed something!

hbagdi · 2022-01-18T23:33:06Z

utils/utils.go

+		return result, fmt.Errorf("GetAll() method not found for %v. "+
+			"Please file a bug with Kong Inc", reflect.ValueOf(obj).Type())


Suggested change

return result, fmt.Errorf("GetAll() method not found for %v. "+

"Please file a bug with Kong Inc", reflect.ValueOf(obj).Type())

return result, fmt.Errorf("GetAll() method not found for type '%v'. "+

"Please file a bug with Kong Inc", reflect.ValueOf(obj).Type())

hbagdi · 2022-01-18T23:34:28Z

cmd/validate.go

+		fmt.Println(err)
+		os.Exit(1)


Suggested change

fmt.Println(err)

os.Exit(1)

panic(err.Error())

hbagdi · 2022-01-18T23:38:18Z

validate/validate.go

+	ctx context.Context,
+	ks *state.KongState,
+	client *kong.Client,
+	parallelism int,
+	rbacResourcesOnly bool,


Instead create a ValidaterOpt struct and then take that struct as input.

type ValidatorOpts struct { . . } func NewValidator(opt ValidatorOpt) { . . }

hbagdi · 2022-01-18T23:39:04Z

validate/validate.go

+func (v ErrorsWrapper) Error() string {
+	var errStr string
+	for _, e := range v.Errors {
+		errStr += fmt.Sprintf("%s\n", e.Error())


Do not add a print after the last error in the array.

Fix #190

Validate via Kong API

231e0e7

GGabriele requested a review from a team as a code owner October 5, 2021 11:52

hbagdi reviewed Oct 5, 2021

View reviewed changes

cmd/common.go Outdated Show resolved Hide resolved

hbagdi reviewed Oct 5, 2021

View reviewed changes

cmd/validate.go Outdated Show resolved Hide resolved

rework

b91c844

hbagdi reviewed Oct 6, 2021

View reviewed changes

addressing comments

ef450c4

hbagdi reviewed Oct 7, 2021

View reviewed changes

GGabriele added 2 commits October 8, 2021 09:36

addressing more comments

4136209

adding comment

422f066

hbagdi reviewed Oct 11, 2021

View reviewed changes

minor changes + limiting concurrency

bc31676

GGabriele commented Oct 12, 2021

View reviewed changes

remove validate function

e25214e

hbagdi reviewed Oct 12, 2021

View reviewed changes

cmd/validate.go Outdated Show resolved Hide resolved

Add validation for missing rbac entities

a4c603e

hbagdi reviewed Oct 13, 2021

View reviewed changes

GGabriele added 3 commits October 14, 2021 08:34

Fetch kong version before making other requests to make sure we have …

9c3ec69

…connectivity

implementing --rbac-resources-only and --workspace flags

94ed516

fix error check

ddeb657

GGabriele commented Oct 14, 2021

View reviewed changes

cmd/validate.go Outdated Show resolved Hide resolved

switch back to use ExistsByName

7523bfd

hbagdi reviewed Oct 19, 2021

View reviewed changes

GGabriele added 2 commits October 21, 2021 14:32

Rename workspace to workspaceFlag in getWorkspaceName function

a6b0a11

Improve handling of missing GetAll method issues

34fb225

hbagdi assigned GGabriele Jan 5, 2022

hbagdi requested changes Jan 14, 2022

View reviewed changes

GGabriele added 4 commits January 15, 2022 11:48

Add --parallelism flag

3e13393

add errors wrapper

6319328

reorganize code

536ba04

do not run workspace related code if no workspace is submitted

809e7d3

GGabriele mentioned this pull request Jan 15, 2022

feat: make it possible to validate configuration without the existence of a specific workspace #563

Open

lint

9c68a3b

hbagdi reviewed Jan 18, 2022

View reviewed changes

Addressing comments

69135ea

hbagdi approved these changes Jan 19, 2022

View reviewed changes

hbagdi merged commit cac49c4 into Kong:main Jan 19, 2022

AntoineJac pushed a commit that referenced this pull request Jan 23, 2024

feat: add support to validate via Kong API (#502)

7f03717

Fix #190

	func getEnterpriseRBACConfiguration(ctx context.Context, group *errgroup.Group,
	client kong.Client, state utils.KongRawState) {
	group.Go(func() error {
	roles, err := GetAllRBACRoles(ctx, client)
	if err != nil {
	return fmt.Errorf("roles: %w", err)
	}
	state.RBACRoles = roles
	return nil
	})

	group.Go(func() error {
	eps, err := GetAllRBACREndpointPermissions(ctx, client)
	if err != nil {
	return fmt.Errorf("eps: %w", err)
	}
	state.RBACEndpointPermissions = eps
	return nil
	})
	}

-func validateGetAllMethod() {
+// 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() {

	When ran with the --online flag, it also uses the 'validate' endpoint in Kong
	No communication takes places between decK and Kong during the execution of
	this command unless --online flag is used.

	if err := validateWithKong(cmd, ks); err != nil {
	if errs := validateWithKong(cmd, ks); errs != nil {

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

	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.")

		return result, fmt.Errorf("GetAll() method not found for %v. "+
		"Please file a bug with Kong Inc", reflect.ValueOf(obj).Type())

feat: add support to validate via Kong API #502

feat: add support to validate via Kong API #502

Conversation

GGabriele commented Oct 5, 2021 • edited Loading

hbagdi commented Oct 5, 2021

GGabriele commented Oct 6, 2021

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbagdi commented Oct 13, 2021

rainest commented Oct 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 15, 2022

Codecov Report

GGabriele commented Jan 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GGabriele commented Oct 5, 2021 •

edited

Loading