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 consumer IDs in plugins #71

Merged
merged 4 commits into from
Jun 5, 2017

Conversation

tophercullen
Copy link

Fixes #70

@CyExy
Copy link
Contributor

CyExy commented May 22, 2017

Thanks for the PR! Sorry for the delay, I'll check it out when I get back to the office that should happen be in couple of days.

@sarraz1
Copy link

sarraz1 commented May 24, 2017

Hello, I'm going to test and review your fix.

@sarraz1
Copy link

sarraz1 commented May 31, 2017

A problems is that when I dump the configuration, consumer_id is lost.

Note that if consumer_id was present, the dumped file could not be used in the case the consumers have to be recreated (consumer_id would be regenerated and so the plugins configurations would be false). Nevertheless you must add consumer_id. Else the applied configuration from the dump would not be the initial one and it's better to have an consumer_id error to fix instead having a bad configuration.

@tophercullen
Copy link
Author

As far as being lost, I reproduced that locally. adding consumer_id to the plugin dump fixes that nicely.

As far as the consumer_id existence, that touches on some key features that are missing, but required for multi-environment management. That is beyond the scope for this PR. However, I'm working through how to solve that exactly

@CyExy
Copy link
Contributor

CyExy commented Jun 1, 2017

Thanks for the PR.

Using the consumer ID can be problematic because they are generated when the consumer is added to Kong.

Maybe adding consumer specific plugins alongside consumer or reference a consumer using the username?

@tophercullen
Copy link
Author

tophercullen commented Jun 1, 2017

The random nature of the consumer ID is different problem I'm working on (see #75) but relies on the plugins being able to handle consumer_ids. A consumer specific plugin would just mean mapping the api name to its api id instead of a mapping a user to its consumer_id. Realistically, both should probably be viable.

Given that passing any attribute is currently allowed when defining plugins, I see two options:

  • Make the plugin aware and handle consumer_ids appropriately (this pr)
  • disallowed the consumer_id attribute in plugins since they can't handle them (a different pr).

@sarraz1
Copy link

sarraz1 commented Jun 1, 2017

I think that reference the consumer by username in plugins is the better solution.

Else and supposing consumers were created first, we can generate kongfig files using kong rest api to find the consumer_id.

Tools like ansible with usage of template file can help to wrap kongfig and implement such a mechanism.

@tophercullen
Copy link
Author

tophercullen commented Jun 1, 2017

I think you are misunderstanding. This PR (or similar functionality) is still needed even if a username is provided, and then mapped, or some other resource (like ansible) does the mapping. The plugin functions will still need to be able to compare plugins with consumer_ids since that is what is fundamentally being compared.

If not, then specifically disallowing the use of consumer_id attributes is needed. Else the same problems, like #68 and #70, are going to keep popping up.

I'm working on a PR for doing a username mapping, but it requires this functionality as there is no other support for consumer plugins currently. Only global or api without consumer components.

@sarraz1
Copy link

sarraz1 commented Jun 1, 2017

But I have never said the opposite, this discussion is based on a remark and I agree that your PR is not directly questionned and consumers id is fundamental in kong plugins. I did not study the problem in détails but I guess we should improve kong rate limiting plugin by adding a method such find_consumer_by_username_or_id.

Nevertheless in your working PR request about username, be aware to not diverge from kong "evolving" rest api. In my sense, that's the kongfig file respect of kong api specifications which makes kongfig reliable.

@sarraz1
Copy link

sarraz1 commented Jun 4, 2017

My tests have been passed successfuly.
A lot a combination of rate-limiting api plugins and global plugins (with and without consumer_id).
It should work on the other plugins too.

#70 and #68 are solved, nice work !

sarraz1
sarraz1 approved these changes Jun 4, 2017
Copy link
Contributor

@CyExy CyExy left a comment

Choose a reason for hiding this comment

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

Thanks, great work.

I do believe that a config file should contain all that is needed for a setup. Or the least exporting config from one Kong instance should apply to another.

Referencing consumer ids by nature cannot happen before they are created, and because of that before this can be released the support for referencing consumers by username needs to be added.

I'm going to merge this into feature/development branch so I could add integration tests etc

@CyExy CyExy changed the base branch from master to feat/consumer-refs June 5, 2017 10:57
@CyExy CyExy merged commit f6010dc into mybuilder:feat/consumer-refs Jun 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants