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

Add support for plugin ordering #2657

Closed
wants to merge 1 commit into from
Closed

Add support for plugin ordering #2657

wants to merge 1 commit into from

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Jul 6, 2022

What this PR does / why we need it:
Adds plugin ordering support. Uses unreleased KTF, deck, and go-kong for now. Adds integration tests against nightly Kong images. Requesting review in advance of dependencies since I want to avoid multiple upstream releases in case change requests affect stuff in libraries.

Depends on Kong/go-kong#187, Kong/deck#710, and Kong/kubernetes-testing-framework#314

Fix #2656

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

@rainest rainest temporarily deployed to Configure ci July 6, 2022 21:47 Inactive
@rainest rainest changed the title Feat/plugin ordering Add support for plugin ordering Jul 6, 2022
@rainest rainest temporarily deployed to Configure ci July 6, 2022 21:55 Inactive
@rainest rainest temporarily deployed to Configure ci July 6, 2022 22:07 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from 86d6e22 to e7da908 Compare July 6, 2022 22:28
@rainest rainest temporarily deployed to Configure ci July 6, 2022 22:29 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from e7da908 to e479601 Compare July 6, 2022 22:35
@rainest rainest temporarily deployed to Configure ci July 6, 2022 22:35 Inactive
@rainest rainest temporarily deployed to Configure ci July 6, 2022 22:35 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 18:46 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 18:46 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 19:08 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from a1ed9ee to 7549b03 Compare July 7, 2022 21:02
@rainest rainest temporarily deployed to Configure ci July 7, 2022 21:02 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from 7549b03 to e6c3763 Compare July 7, 2022 21:12
@rainest rainest temporarily deployed to Configure ci July 7, 2022 21:12 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 21:12 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 21:12 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 21:36 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 22:53 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 22:54 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 22:54 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 22:54 Inactive
@rainest rainest temporarily deployed to Configure ci July 7, 2022 23:53 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from f02db8c to fc3a0d1 Compare July 8, 2022 17:43
@rainest rainest temporarily deployed to Configure ci July 8, 2022 17:43 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from fc3a0d1 to ed9e6f9 Compare July 8, 2022 17:47
@rainest rainest temporarily deployed to Configure ci July 8, 2022 17:47 Inactive
shaneutt
shaneutt previously approved these changes Jul 8, 2022
Copy link
Contributor

@shaneutt shaneutt left a comment

Choose a reason for hiding this comment

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

Looks solid to me, even though it says draft. I would just like to see us use the new cleanup method for the integration tests and then LGTM.

.github/workflows/test.yaml Show resolved Hide resolved
.github/workflows/test.yaml Outdated Show resolved Hide resolved
test/integration/plugin_test.go Outdated Show resolved Hide resolved
@rainest rainest force-pushed the feat/plugin-ordering branch from ed9e6f9 to 0ed231f Compare July 8, 2022 18:26
@rainest rainest temporarily deployed to Configure ci July 8, 2022 18:26 Inactive
@rainest rainest force-pushed the feat/plugin-ordering branch from 0ed231f to a538c4c Compare July 8, 2022 18:31
@rainest rainest temporarily deployed to Configure ci July 8, 2022 18:31 Inactive
@rainest rainest temporarily deployed to Configure ci July 8, 2022 20:41 Inactive
@rainest rainest temporarily deployed to Configure ci July 8, 2022 20:41 Inactive
@rainest rainest temporarily deployed to Configure ci July 8, 2022 20:41 Inactive
@rainest rainest temporarily deployed to Configure ci July 8, 2022 21:06 Inactive
@rainest
Copy link
Contributor Author

rainest commented Jul 11, 2022

Kong nightly failure on the TLSRoute example is on the Kong side. It cannot find the workspace ID of the certificate.

The upstream bug that corrects this is fixed, but at some point the Kong nightly repo stopped updating latest (or rather, it's derived from the wrong branch), and the image it was pulling was ~2m old. Newer releases are available, but you need to know the specific tag for the commit you want. This won't be fixed in the immediate future, so for now I'll just remove the OSS nightly--the Enterprise one should suffice for most purposes.

2022/07/11 17:33:25 [error] 1149#0: *9252 stream [lua] certificate.lua:33: log(): [ssl] failed to get from node cache: callback threw an error: /usr/local/share/lua/5.1/kong/db/strategies/off/init.lua:233: attempt to concatenate local 'ws_id' (a nil value)
stack traceback:
	/usr/local/share/lua/5.1/kong/db/strategies/off/init.lua:233: in function 'select'
10.244.0.1 [11/Jul/2022:17:33:25 +0000] TCP 500 0 0 0.000
	/usr/local/share/lua/5.1/kong/db/dao/init.lua:987: in function 'select'
	/usr/local/share/lua/5.1/kong/runloop/certificate.lua:134: in function </usr/local/share/lua/5.1/kong/runloop/certificate.lua:133>
	[C]: in function 'xpcall'
	/usr/local/share/lua/5.1/resty/mlcache.lua:741: in function 'get'
	/usr/local/share/lua/5.1/kong/cache/init.lua:168: in function 'find_certificate'
	/usr/local/share/lua/5.1/kong/runloop/certificate.lua:249: in function 'execute'
	/usr/local/share/lua/5.1/kong/runloop/handler.lua:1197: in function 'before'
	/usr/local/share/lua/5.1/kong/init.lua:763: in function 'ssl_certificate'
	ssl_certificate_by_lua:2: in main chunk, context: ssl_certificate_by_lua*, client: 10.244.0.1, server: unix:/kong_prefix/stream_tls_terminate.sock

Don't think the resource matters since DB-less should always be the default WS, but whatever:

10:33:25-0700 esenin $ http 192.168.16.242:8001/certificates                                          
HTTP/1.1 200 OK
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 1613
Content-Type: application/json; charset=utf-8
Date: Mon, 11 Jul 2022 17:54:56 GMT
Server: kong/2.8.0-fe20857b0
X-Kong-Admin-Latency: 0

{
    "data": [
        {
            "cert": "-----BEGIN CERTIFICATE-----\nMIIC/jCCAoSgAwIBAgIUVL6UYVDdH6peVNSOnOkCuYyhmrswCgYIKoZIzj0EAwIw\ngbQxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1T\nYW4gRnJhbmNpc2NvMRMwEQYDVQQKDApLb25nLCBJbmMuMRgwFgYDVQQLDA9UZWFt\nIEt1YmVybmV0ZXMxHjAcBgNVBAMMFXRsc3JvdXRlLmtvbmcuZXhhbXBsZTEpMCcG\nCSqGSIb3DQEJARYadGVzdEB0bHNyb3V0ZS5rb25nLmV4YW1wbGUwIBcNMjIwNjE2\nMjExMjI4WhgPMjEyMjA1MjMyMTEyMjhaMIG0MQswCQYDVQQGEwJVUzETMBEGA1UE\nCAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwNU2FuIEZyYW5jaXNjbzETMBEGA1UECgwK\nS29uZywgSW5jLjEYMBYGA1UECwwPVGVhbSBLdWJlcm5ldGVzMR4wHAYDVQQDDBV0\nbHNyb3V0ZS5rb25nLmV4YW1wbGUxKTAnBgkqhkiG9w0BCQEWGnRlc3RAdGxzcm91\ndGUua29uZy5leGFtcGxlMHYwEAYHKoZIzj0CAQYFK4EEACIDYgAEQecfzsxmPwC0\n6uNs3kyiLDb6brngM4ZtGXgwcGD393cbYmaunfBPRtxqh76RKdS9wzq4q+oB8dPs\nQKgBNhlJTr+iFH9Di7bBZFcYqx+SnNUXZ0dDNBbW4rPVTJHQvdono1MwUTAdBgNV\nHQ4EFgQU+OOVbqMcu+yXomZfnZ54LgIRNo4wHwYDVR0jBBgwFoAU+OOVbqMcu+yX\nomZfnZ54LgIRNo4wDwYDVR0TAQH/BAUwAwEB/zAKBggqhkjOPQQDAgNoADBlAjBu\nPMq+T+iTJ0yNvldYpB3BfdIhrv0EJQ9ALbB16nJwF91YV6YE7mdNP5rNVnoZ0nAC\nMQDmnIpipMawjJWpfSPSZS1/iArz8YuBroWrGFXP62lwhCUp8RZweNnrLmmb/Aek\ny3o=\n-----END CERTIFICATE-----",
            "cert_alt": null,
            "created_at": 1657560260,
            "id": "6e38f366-5589-4606-84b9-6b4f53be2b92",
            "key": "-----BEGIN PRIVATE KEY-----\nMIG2AgEAMBAGByqGSM49AgEGBSuBBAAiBIGeMIGbAgEBBDDDRndgPYZaonVuqHiu\n5uuYWI+A16BYLoUBnY0/9BL9U0s47G7LC/b05wE/7UPJEBKhZANiAARB5x/OzGY/\nALTq42zeTKIsNvpuueAzhm0ZeDBwYPf3dxtiZq6d8E9G3GqHvpEp1L3DOrir6gHx\n0+xAqAE2GUlOv6IUf0OLtsFkVxirH5Kc1RdnR0M0Ftbis9VMkdC92ic=\n-----END PRIVATE KEY-----",
            "key_alt": null,
            "snis": [
                "tlsroute.kong.example"
            ],
            "tags": null
        }
    ],
    "next": null
}

Add a new ordering field to KongPlugin and KongClusterPlugin. On
Enterprise instances, this can manipulate plugin execution order.

Add integration test runs for nightly Kong images.
@rainest rainest force-pushed the feat/plugin-ordering branch from e2089e5 to 8872119 Compare July 14, 2022 19:48
@rainest rainest closed this Jul 14, 2022
@shaneutt
Copy link
Contributor

shaneutt commented Jul 14, 2022

? New PR coming?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for plugin ordering
2 participants