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

authorize and proxy refuse to start if policy is empty #190

Closed
victornoel opened this issue Jun 21, 2019 · 5 comments · Fixed by #200
Closed

authorize and proxy refuse to start if policy is empty #190

victornoel opened this issue Jun 21, 2019 · 5 comments · Fixed by #200
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@victornoel
Copy link

Describe the bug

authorize and proxy refuse to start if policy is empty.

Expected behavior

I would expect pomerium to start but not serve anything.

Environment:

  • Pomerium version (retrieve with pomerium --version): v0.0.5+a405379
@desimone
Copy link
Contributor

desimone commented Jul 1, 2019

When policy and route configurations were static, I think it made more sense to treat empty policy as a configuration error. If only because, I can't come up with a use-case for having a policy-less instance and could see the alternative as confusing.

However, now that we support hot-reloadable policy, I think it's worth taking another look at allowing pomerium to start, and be updated, with an empty policy. I haven't tested it yet, but I would not want pomerium to crash because it was given an update empty or invalid policy.

@victornoel is this the use-case you had in mind? I'm curious where this "bit" you.

/cc @travisgroth

@desimone desimone added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 1, 2019
@travisgroth
Copy link
Contributor

travisgroth commented Jul 1, 2019

I would not want pomerium to crash because it was given an update empty or invalid policy.

Agreed. I believe it would be fine and simply not have any routes. I vaguely recall considering this but it should be double checked.

Removing the validation check should be easy, assuming the rest of the code works in that state. I'm also thinking policy route count should be logged / instrumented to aid in debugging.

@travisgroth
Copy link
Contributor

FWIW, I just verified we're stable if you update the config to have no policy. We just 404.

So, I think this is easy to support. We just remove the check for an empty policy structure at initialization.

As a pattern, I think this is fine as long as we make it clear when you've got no policies. I can put this PR together.

@victornoel
Copy link
Author

@desimone you perfectly understood my use case: the deployment lifecycle of my policy configuration is not exactly the same as the deployment of pomerium so it can happens that sometimes the policy is empty. It isn't meant to be the case at all time, but it can happen. For example I will deploy a new cluster with pomerium along with some other infrastructure pods, then later someone else is going to deploy an application and update the policy configuration.

@travisgroth thx also it's great your PR introduce a log message when the policy is updated, it will ease diagnostics in some cases :)

@travisgroth
Copy link
Contributor

Makes sense to me. Thanks @victornoel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants