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

Include details in the Konnect sync error #5274

Closed
1 of 3 tasks
mflendrich opened this issue Dec 4, 2023 · 3 comments · Fixed by #5453
Closed
1 of 3 tasks

Include details in the Konnect sync error #5274

mflendrich opened this issue Dec 4, 2023 · 3 comments · Fixed by #5453
Assignees
Labels
release/highlight This part of the release is worth bragging about.
Milestone

Comments

@mflendrich
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

Problem Statement

When performing deck sync to Konnect, in the event of a resource failing validation on the Konnect end, the log message from

return "", fmt.Errorf("performing update for %s failed: %w", client.AdminAPIClient().BaseRootURL(), err)
looks like this

kong-controller-fefefefe-abcde time="2023-12-04T08:10:30Z" level=warning msg="Failed pushing configuration to Konnect" error="performing update for https://us.kic.api.konghq.com/kic/api/runtime_groups/fefefefe-fefe-fefe-fefe-fefefefefefe failed: 1 errors occurred:\n\twhile processing event: {Create} jwt-auth my_jwt_secret for consumer example-user failed: HTTP status 400 (message: \"validation error\")\n"

where validation error is not a useful message to the user.

The Admin API side (at least in Konnect's implementation) provides more meaningful detail in the details field of the response:

{"code":3,"message":"validation error","details":[{"@type":"type.googleapis.com/kong.admin.model.v1.ErrorDetail","type":"ERROR_TYPE_FIELD","field":"value.rsa_public_key","messages":["'-----BEGIN RSA PUBLIC KEY-----\\nMIIREDACTED...\\n-----END RSA PUBLIC KEY-----' is not valid 'pem-pkix-encoded-public-key'"]}]}

Proposed Solution

Include the details field in the log entry.

Additional information

I haven't verified if the Lua implementation populates details as well (but I believe it should) - the implementation should treat the details field as optional and not crash if it's not present :)

Acceptance Criteria

  • in the event of HTTP 4xx received from Konnect during sync, the details field from the error response is included in the message to the user
  • stretch, optional (but likely to be implemented because of a shared code path): the same but for DB mode sync to Lua CP
@mflendrich mflendrich added this to the KIC v3.1.x milestone Dec 4, 2023
@rainest
Copy link
Contributor

rainest commented Dec 7, 2023

https:/Kong/go-database-reconciler/blob/deb74352abb17010b1fb8524dd6c0a3519055cb7/pkg/konnect/error.go#L11-L39 appears to be the culprit. It unmarshals into a struct with only a Message field (TIL apparently go will try and match fields without JSON unmarshal tags?).

@rainest
Copy link
Contributor

rainest commented Dec 7, 2023

Not sure what exactly we can expect in details. Upstream that's apparently from Koko GRPC details, which is effectively a gRPC status.

Those details can contain arbitrary data with a type indicator, but looking at the preceding code it looks like they should always be a collection of this ErrorDetail structs.

Dunno how exactly that translates into the HTTP response body though--naively I'd assume you do get JSON array of those structs, but I don't have a ready actual HTTP response in front of me to say for sure.

While the private koko repo is the actual source of truth about those types, the types currently in the public repo don't appear to have diverged at present, so I've linked to them.

@mflendrich mflendrich added the release/highlight This part of the release is worth bragging about. label Dec 8, 2023
@randmonkey randmonkey assigned randmonkey and unassigned randmonkey Dec 11, 2023
@randmonkey
Copy link
Contributor

randmonkey commented Jan 11, 2024

Asked koko team and got the feedback from them that details field is guaranteed to exist and have a []interface{} schema, but the schema of internal items is not fixed:

so, the presence of details on error should be guaranteed, but there is no fixed schema for its item because it depends on the type of error

After checking the code of diff.Syncer, the CRUD of entity (no matter to Kong gateway or to Konnect) will call the CRUD methods in go-kong, and finally call client.Do to send request and get response:
(For example, creating a service)
https:/Kong/go-database-reconciler/blob/7e0411f83b2d626756077c789f176f740294b7a3/pkg/types/service.go#L30-L33
-->
https:/Kong/go-kong/blob/b34bcbb6c66e3d01719a1d148e559fb1d9c58132/kong/service_service.go#L34
-->
https:/Kong/go-kong/blob/b34bcbb6c66e3d01719a1d148e559fb1d9c58132/kong/service_service.go#L53

So we need to add parsing of details field of response in the client.Do method in go-kong and then update the go-kong packeage in go-database-reconciler.
Created issue Kong/go-kong#398 in go-kong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release/highlight This part of the release is worth bragging about.
Projects
None yet
3 participants