Optional X-Consumer-Groups ACL Header Improvement

[jeremyjpj0916]

Summary

As discussed here : #3609

One of my biggest concerns with Kong's ACL functionality as it stands today is including the arbitrarily growing in size list of a consumers entire ACL groups as a header. In our specific implementation of Kong every route gets is own ACL group as the routes existing uuid, so we can then give consumers access to specific routes by adding that uuid to their ACL, I suspect this will be a popular pattern going forward for many users who implement Kong.

I propose a boolean that will act as a switch and enable/disable sending the consumer groups as a header, as I suspect for plenty of use cases that the arbitrary group info in the header is not necessary and as it grows in size could one day break an otherwise working proxy by increasing the header size past what a API Provider will accept (since many API providers have a 8kb header size limit by default like Apache Tomcat). I would like a predictable header size going to the API provider that is static when it comes to size generally, and by enabling this functionality on the ACL plugin we can achieve that with Kong.

Full changelog

1. Modify schema.lua to include new boolean property 'hide_groups_header' defaulted to false as to not break existing functionality
2. Modify handler.lua to account for 'hide_groups_header' boolean setting.
3. Modify /spec/03-plugins/19-acl/02-access_spec.lua for false and true testcase.

PR to Documentation: Kong/docs.konghq.com#822

Issues resolved

Fixes concern for growing Kong header size as ACL groups list grow. Also improves bytes across the wire to exclude potentially extraneous info of a consumers entire ACL group list.

[thibaultcha]

style: will you please add a trailing comma at the end of this line? This is a part of the codebase that isn't yet compliant with our formalized codestyle, but it allows to reduce the size of the diff when adding new fields in the future. Thanks!

[jeremyjpj0916]

Done! Thx.

[thibaultcha]

@jeremyjpj0916 Awesome! Thank you!

[jeremyjpj0916]

@thibaultcha Glad you took the initiative to squeeze this in so soon! Thanks a ton.

[Tieske]

Since the memory comment is only an optimization, both can go in the same PR against next imo.

(sorry for the late review, still on vacation 😄 )

[Tieske]

In the case when we're not setting the header, we should set a dummy value in the cache. We're trying to save bandwidth in the request, but we should also reduce Kong memory pressure.

so above do:

    if to_be_blocked == false then
       -- we're allowed, so go and convert 'false' to the header value, if we need it.
       -- if we don't need it, set a dummy value to save memory for potentially very long strings
       to_be_blocked = conf.hide_groups_header and "" or table_concat(consumer_groups, ", ")
     end

[jeremyjpj0916]

This seems nice as a further improvement. Since this has already been merged to master branch would the proper approach be to submit a new PR with this additional enhancement on master as well? Didn't know you could do true and "" or table of consumers_groups would cause to_be_blocked == "".

[Tieske]

this needs another PR against next, to convert existing records from nil to false. Since the schema says the default is false, it is expected to always be a boolean value, and not an absent one. To keep the data in line with the schema.

[jeremyjpj0916]

Is it problematic for this to go into the 0.14.1 minor version as is? I think in my testing of custom plugins the addition of new schema fields are treated as nil(like you mentioned) which will at least produce the desired results until converting nil to false right? For some reason initially I was wrongly thinking Kong core recognized those defaults and would take action on populating them by default in all plugins/db records as it sees fit upon successful start up, maybe a nice future Kong enhancement to automate that process. Would the next PR be what is considered a migrations run to fix this then? If so I will look over other migrations examples and whip up whats needed to get that field set to false for older acl plugin records on the next branch.

[thibaultcha]

@Tieske All in for the flag to prevent the header from being constructed.

Regarding the migration, there has been some internal discussions in the last week. We might reconsider our policy to provide migrations for schema changes. In any case, I did not want to bother Jeremy with contributing a migration just yet anyway, especially if there might not be the need for it. We should have a chat.

[thibaultcha]

@jeremyjpj0916 Yes, this is indeed scheduled to be part of 0.14.1, no problem.

If you are able to provide the subsequent improvement suggested by @Tieske by then (before next week), that’d be great, but not necessary.

[jeremyjpj0916]

@thibaultcha I'm wfh and have a few mins to tackle this before next meeting. Question is would "" be better for the setting or nil on mem 😆?

to_be_blocked = conf.hide_groups_header and "" or table_concat(consumer_groups, ", ")

Or

to_be_blocked = conf.hide_groups_header and nil or table_concat(consumer_groups, ", ")

If we want to get picky with it hah. The former creates a pointer to new space in mem while the latter already exists (If this was C eh, idk about lua).

[thibaultcha]

Strings in Lua are interned, so ”” is probably fine. But nil might be more appropriate to carry the intended meaning here; I’d probably go for nil.

[jeremyjpj0916]

👍 roger. Thx! PR for that incoming to master 2 mins.