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

Validation of OpenTelemetry plugin's "config.resource_attributes" field is too strict #10654

Closed
1 task done
seh opened this issue Apr 11, 2023 · 23 comments
Closed
1 task done
Assignees
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... plugins/opentelemetry stale

Comments

@seh
Copy link

seh commented Apr 11, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

3.2.2

Current Behavior

Following the documentation for the OpenTelemetry plugin, writing a KongPlugin manifest to be translated by the Kong ingress controller (version 2.9.2), I attempted to set a few attributes in the "config.resource_attributes" field, which is of type "map," per the schema. If any of the attribute names include a period—such as "service.name," or "k8s.namespace.name" per the OpenTelemetry semantic conventions—then the ingress controller's validating Webhook rejects the manifest as follows:

admission webhook “validations.kong.konghq.com” denied the request: plugin failed schema validation: schema violation (config.resource_attributes: expected a string)

This comes from a manifest like the following:

apiVersion: configuration.konghq.com/v1
kind: KongPlugin
metadata:
  name: opentelemetry-to-collector
  namespace: &ns some-app
plugin: opentelemetry
config: 
  endpoint: http://opentelemetry-collector.some-ns.svc.cluster.local:4318/v1/traces
  resource_attributes:
    k8s.namespace.name: *ns

I tried a few variations, placing the "k8s.namespace.name" mapping key name within double quotation marks, and inserting a backslash before the periods, but neither changed the outcome.

If I remove the periods and use, say, "k8snamespacename" as the resource attribute name, Kong's validation procedure accepts the manifest, and the ingress controller programs the proxies accordingly.

Expected Behavior

Kong's validation procedure should admit just about any string here for the attribute name, as the OpenTelemetry specification is still vague on the syntactic constraints for such names.

Steps To Reproduce

  1. (Optionally) Enable the OpenTelemetery plugin within Kong's configuration.
  2. Within Kubernetes, enable Kong's validating admission Webhook via a ValidatingWebhookConfiguration.
  3. Attempt to create a KongPlugin object by applying a manifest like the one shown earlier, via kubectl apply --filename <name> --dry-run=server or kubectl apply --filename <name> --server-side=true.
  4. Observe that the validating Webhook rejects the manifest due to the "config.resource_attributes" mapping entry.

Anything else?

I first mentioned this problem in the "kong" channel of the "Kubernetes" Slack workspace.

This plugin entered Kong's code base in #8826 by @mayocream.

@randmonkey
Copy link
Contributor

@seh I used exactly the same Kong ingress controller and Kong version (KIC 2.9.2, Kong 3.2.2) and same manifest

apiVersion: configuration.konghq.com/v1
kind: KongPlugin
metadata:
  name: opentelemetry-to-collector
  namespace: default
plugin: opentelemetry
config: 
  endpoint: http://opentelemetry-collector.some-ns.svc.cluster.local:4318/v1/traces
  resource_attributes:
    k8s.namespace.name: default

And I could successfully create the plugin. Would you please confirm again the versions of KIC and Kong, and also the manifest?

@seh
Copy link
Author

seh commented Apr 28, 2023

As stated earlier, I'm using Kong proxy version 3.2.2 and ingress controller version 2.9.2.

Here's another manifest:

KongPlugin manifest
apiVersion: configuration.konghq.com/v1
kind: KongPlugin
metadata:
  name: opentelemetry-to-collector
  namespace: default
plugin: opentelemetry
config: 
  endpoint: http://opentelemetry-collector.some-ns.svc.cluster.local:4318/v1/traces
  resource_attributes:
    k8s.namespace.name: default

Attempting to apply that manifest yields the following failure reported by kubectl:

Error from server: error when creating "/tmp/kong-plugin.yaml":
  admission webhook "validations.kong.konghq.com" denied the request:
    plugin failed schema validation:
      schema violation (config.resource_attributes: expected a string)

Asking for the server to validate in via --dry-run=server provokes the same response from the validating Webhook.

@seh
Copy link
Author

seh commented Apr 28, 2023

@randmonkey, do you have a ValidatingWebhookConfiguration in place to validate creation requests for KongPlugin objects?

@reneeckstein
Copy link

Hi. I have the exact same problem on Kong 3.2.1 and (after just upgrading) also on Kong 3.3.0 without using Kong Ingress Controller.

Even the example from the docs already failing for me
https://docs.konghq.com/hub/kong-inc/opentelemetry/#configure-the-opentelemetry-plugin

curl -X POST http://localhost:8001/plugins \
    --data "name=opentelemetry"  \
    --data "config.endpoint=http://some-service-name.some-namespace.svc.cluster.local:4318/v1/traces" \
    --data "config.resource_attributes.service.name=kong-dev"

Error:
{"message":"schema violation (config.resource_attributes: expected a string)","code":2,"fields":{"config":{"resource_attributes":"expected a string"}},"name":"schema violation"}

@reneeckstein
Copy link

Hi @seh I just found a way to set it with decK

Plugin Config in decK

- config:
    batch_flush_delay: 3
    batch_span_count: 200
    connect_timeout: 1000
    endpoint: http://jaeger-collector.some-namespace.svc.cluster.local:4318/v1/traces
    headers: null
    read_timeout: 5000
    resource_attributes:
      service.name: kong-dev
    send_timeout: 5000
  enabled: true
  name: opentelemetry
  protocols:
  - grpc
  - grpcs
  - http
  - https

image

@jschmid1 jschmid1 added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Jun 27, 2023
@stale
Copy link

stale bot commented Jul 11, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@seh
Copy link
Author

seh commented Jul 12, 2023

This issue is still not fixed.

@ms2008
Copy link
Contributor

ms2008 commented Jul 19, 2023

@seh The issue is sort of conditionally fixed. That is, when you send a JSON request it works (after this PR #11091), but when you send a form data request it doesn't work because the variable name in the form data contains the . symbol, which is recognized by Kong as nested keys, and then normalize dotted keys in objects which will cause schema violation.

@seh
Copy link
Author

seh commented Jul 19, 2023

Thank you for the detail, but this still strikes me as unnecessarily pedantic: a distinction without a difference. To us users, this is still broken, with no apparent way for us to "fix it" by doing something different.

If the Kubernetes validating admission Webhook is sending HTTP form data and it should not, then that fix belongs in the Webhook. Do you think that we should migrate this issue—or file a related one—over to Kong/kubernetes-ingress-controller? It would help to hear what you think would work better instead.

@ms2008
Copy link
Contributor

ms2008 commented Jul 21, 2023

If the Kubernetes validating admission Webhook is sending HTTP form data and it should not, then that fix belongs in the Webhook.

I think Webhook sends a JSON request, can @randmonkey confirm?

Do you think that we should migrate this issue—or file a related one—over to Kong/kubernetes-ingress-controller?

I don't think it's necessary, because this issue is only triggered if you enable the KIC's admission webhook feature, and it won't happen if you disable the admission Webhook. It's essentially a Kong gateway issue.

If you're using a version of Kong gateway after the PR I mentioned above, then the problem is actually fixed.

@randmonkey
Copy link
Contributor

@ms2008 @seh ALL requests sent from KIC are filled with JSON body, because they are sent by Kong/go-kong:
https://github.com/Kong/go-kong/blob/main/kong/request.go#L14-59

@dlouzan
Copy link
Contributor

dlouzan commented Aug 9, 2023

@ms2008 We are also experiencing this with the file-log plugin, am I right to think the fix will be part of the upcoming 3.4 release? https://github.com/Kong/kong/commits/3.4.0. The commit above is at least part of the tag history, but there's no changelog yet.

@dlouzan
Copy link
Contributor

dlouzan commented Aug 9, 2023

As a temporary fix, we could disable the admission controller with:

helm upgrade ingress kong
    ...
    --set ingressController.admissionWebhook.enabled=false

@ms2008
Copy link
Contributor

ms2008 commented Aug 10, 2023

@dlouzan Yes, that fix has been included in 3.4, the exact commit is here 866a15c and you can see that the changelog has been updated as well!

As a temporary fix, we could disable the admission controller

That's right, this only affects the webhook, so temporarily disabling the webhook is a valid workaround.

@samugi
Copy link
Member

samugi commented Aug 16, 2023

@seh @dlouzan 3.4 was released, could you confirm whether you can still reporoduce the issue on that version?

@dlouzan
Copy link
Contributor

dlouzan commented Aug 16, 2023

@samugi Sure, we will comment back here when we update our systems (probably next week).

@dlouzan
Copy link
Contributor

dlouzan commented Aug 16, 2023

@samugi But I'm realizing, we won't probably be able to test it properly, as I think the helm chart has not yet updated the default kong version to 3.4, isn't it? We tend to wait until that is updated and not keep a custom version for the values.

@stale
Copy link

stale bot commented Sep 16, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 16, 2023
@seh
Copy link
Author

seh commented Sep 16, 2023

Unfortunately, I am no longer equipped to test the fix, as I am no longer using Kong.

@stale stale bot removed the stale label Sep 16, 2023
@dlouzan
Copy link
Contributor

dlouzan commented Sep 17, 2023

At the time of our latest system update, the helm chart did not still incorporate a version that included this fix. We can give feedback later this month.

@github-actions
Copy link
Contributor

This issue is marked as stale because it has been open for 14 days with no activity.

@github-actions github-actions bot added the stale label Oct 12, 2023
@github-actions
Copy link
Contributor

Dear contributor,

We are automatically closing this issue because it has not seen any activity for three weeks.
We're sorry that your issue could not be resolved. If any new information comes up that could
help resolving it, please feel free to reopen it.

Your contribution is greatly appreciated!

Please have a look
our pledge to the community
for more information.

Sincerely,
Your Kong Gateway team

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 19, 2023
@dlouzan
Copy link
Contributor

dlouzan commented Oct 30, 2023

With the most recent helm chart which uses kong 3.4+, this seems to be working properly now 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... plugins/opentelemetry stale
Projects
None yet
Development

No branches or pull requests

8 participants