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

Logical/Auth plugin config should be saned #3180

Closed
jefferai opened this issue Aug 16, 2017 · 4 comments
Closed

Logical/Auth plugin config should be saned #3180

jefferai opened this issue Aug 16, 2017 · 4 comments
Assignees
Milestone

Comments

@jefferai
Copy link
Member

When auth plugins were added, rather than add a config map to the API, plugin_name was added as its own parameter, creating a discrepancy with secret mounts. Secret mounts should get a plugin_name, and auth mounts a config map, so that both support the same workflow (but without backwards compatibility).

@jefferai jefferai added this to the next-release milestone Aug 16, 2017
@calvn
Copy link
Member

calvn commented Aug 16, 2017

Should we encourage usage towards one or the other (e.g. mark one of them as deprecated)?

@jefferai
Copy link
Member Author

I think we should look at likely usage patterns. To some extent I'm not sure whether the things that are in the config map now are necessarily easier to have there than pulling up to the top level. I think the original idea was that the config map would hold configuration for the backends but this went away by having config endpoints within the backends that can be ACL'd. So it's possible the right thing is to keep supporting this but change to simply moving these things up a level out of the map, unless we think we're going to be piling on more of these over time.

@calvn
Copy link
Member

calvn commented Aug 17, 2017

I see. Looking at it from the api.MountInput perspective, having things at the top level looks more like what the CLI accepts (i.e. you don't provide plugin-name, max-lease-ttl, etc. within a --config param, but directly instead), but less like what vault.MountEntry accepts (since it transposes those fields back into MountConfig).

The way I see it is that api.MountInput acts as the interface between the actual CLI input and MountEntry under core, and it makes perfect sense since that's what the api package is supposed to do. I guess the question is, should we lean towards having it look more like how the CLI behaves, or how it's laid out in core (i.e. MountEntry.Config).

@calvn
Copy link
Member

calvn commented Aug 17, 2017

Actually the CLI is more of a thin wrapper around the API, so the key interaction is between the API and the underlying core functionality. In that case I'd say that MountInput should assimilate the structure of MountEntry, but I agree that we should probably keep both for now and observe usage patterns until a further conclusion can be drawn.

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

No branches or pull requests

2 participants