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

fix: add missing protocols field to plugins #9525

Merged
merged 2 commits into from
Oct 31, 2022

Conversation

GGabriele
Copy link
Contributor

Summary

All plugin entities are expected to have a protocols schema field, but some plugin schemas are missing it. From a functional point of view, this doesn't have any impact, since the protocols field is still injected in the overall schema. But when plugin schemas are retrieved via the Admin API using the /schemas/plugins/<name> endpoint, then the "original" schema is returned. For example:

$ http :8001/schemas/plugins/prometheus
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 354
Content-Type: application/json; charset=utf-8
Date: Tue, 11 Oct 2022 06:42:47 GMT
Server: kong/3.0.0
X-Kong-Admin-Latency: 6

{
    "fields": [
        {
            "config": {
                "fields": [
                    {
                        "per_consumer": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "status_code_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "latency_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "bandwidth_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "upstream_health_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    }
                ],
                "required": true,
                "type": "record"
            }
        }
    ]
}

This is problematic for decK, which uses this endpoint to inject schema defaults before doing a deployment, which can cause unnecessary/misleading diffs:

$ cat kong.yaml
_format_version: "3.0"
plugins:
  - name: prometheus

$ deck sync
updating plugin prometheus (global)  {
   "config": {
     "bandwidth_metrics": false,
     "latency_metrics": false,
     "per_consumer": false,
     "status_code_metrics": false,
     "upstream_health_metrics": false
   },
   "enabled": true,
   "id": "93aece4b-b813-4d9b-af28-f4c1c874f128",
   "name": "prometheus",
-  "protocols": [
-    "grpc",
-    "grpcs",
-    "http",
-    "https"
-  ]
 }

Summary:
  Created: 0
  Updated: 1
  Deleted: 0

Full changelog

  • add missing protocols field to plugin schemas

@GGabriele GGabriele requested a review from a team as a code owner October 11, 2022 07:09
@CLAassistant
Copy link

CLAassistant commented Oct 11, 2022

CLA assistant check
All committers have signed the CLA.

@hbagdi
Copy link
Member

hbagdi commented Oct 11, 2022

@bungle @gszr Is there a way to add an assertion in our test suite to ensure that something like this doesn't happen again (at least for bundled plugins)?

@hbagdi
Copy link
Member

hbagdi commented Oct 25, 2022

@flrgh With your new powers, can you help us review this PR?
@GGabriele Please work with @flrgh to get this over the finish line.

@flrgh flrgh force-pushed the fix/plugins-missing-protocols branch from 0176833 to d7ead09 Compare October 28, 2022 17:23
Copy link
Contributor

@flrgh flrgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GGabriele code changes LGTM, thanks 👍 can you please add a CHANGELOG entry for this? We'll be good to merge once that's done.

@GGabriele GGabriele force-pushed the fix/plugins-missing-protocols branch from d7ead09 to 2cf97aa Compare October 31, 2022 12:03
All plugin entities are expected to have a `protocols` schema field,
but some plugin schemas are missing it. From a functional point
of view, this doesn't have any impact, since the `protocols` field
is still injected in the overall schema. But when plugin schemas
are retrieved via the Admin API using the `/schemas/plugins/<name>`
endpoint, then the "original" schema is returned. For example:

```
$ http :8001/schemas/plugins/prometheus
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 354
Content-Type: application/json; charset=utf-8
Date: Tue, 11 Oct 2022 06:42:47 GMT
Server: kong/3.0.0
X-Kong-Admin-Latency: 6

{
    "fields": [
        {
            "config": {
                "fields": [
                    {
                        "per_consumer": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "status_code_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "latency_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "bandwidth_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    },
                    {
                        "upstream_health_metrics": {
                            "default": false,
                            "type": "boolean"
                        }
                    }
                ],
                "required": true,
                "type": "record"
            }
        }
    ]
}
```

This is problematic for decK, which uses this endpoint to
inject schema defaults before doing a deployment, which can
cause unnecessary/misleading diffs:

```
$ cat kong.yaml
_format_version: "3.0"
plugins:
  - name: prometheus

$ deck sync
updating plugin prometheus (global)  {
   "config": {
     "bandwidth_metrics": false,
     "latency_metrics": false,
     "per_consumer": false,
     "status_code_metrics": false,
     "upstream_health_metrics": false
   },
   "enabled": true,
   "id": "93aece4b-b813-4d9b-af28-f4c1c874f128",
   "name": "prometheus",
-  "protocols": [
-    "grpc",
-    "grpcs",
-    "http",
-    "https"
-  ]
 }

Summary:
  Created: 0
  Updated: 1
  Deleted: 0
```
@GGabriele
Copy link
Contributor Author

GGabriele commented Oct 31, 2022

@GGabriele code changes LGTM, thanks 👍 can you please add a CHANGELOG entry for this? We'll be good to merge once that's done.

Thanks for the review!
I just pushed the Changelog update too.

@flrgh flrgh merged commit 56cffd0 into Kong:master Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants