deis / router

Edge router for Deis Workflow
https://deis.com
MIT License
80 stars 57 forks source link

feat(model): allow configuration through a configmap. #270

Closed ultimateboy closed 8 years ago

ultimateboy commented 8 years ago

While this does function as desired, this PR is still a WIP - mainly needing updated docs.

deis-bot commented 8 years ago

@kmala, @arschles and @krancour are potential reviewers of this pull request based on my analysis of git blame information. Thanks @ultimateboy!

krancour commented 8 years ago

I've actually been wanting to do this. I'll take a look.

ultimateboy commented 8 years ago

A sample configmap:

apiVersion: v1
kind: ConfigMap
metadata:
  name: deis-router
  namespace: deis
  labels:
    heritage: deis
data:
  nginx.errorLogLevel: "info"
  nginx.maxWorkerConnections: "767"
codecov-io commented 8 years ago

Current coverage is 45.53% (diff: 19.23%)

Merging #270 into master will decrease coverage by 0.86%

@@             master       #270   diff @@
==========================================
  Files             3          3          
  Lines           306        325    +19   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            142        148     +6   
- Misses          160        171    +11   
- Partials          4          6     +2   

Powered by Codecov. Last update 9a0d3c8...26becac

krancour commented 8 years ago

I've made some line comments that I think should be addressed. Those aside, I want to make a more general comment as well.

I previously said:

I've actually been wanting to do this.

I'm now second guessing my own impulse. ConfigMaps are sexy. They seem like the right way™ to configure things, but annotations also make a high degree of sense here and seem pretty defensible as well-- if routable services are configured through annotations, why not configure the router in the same way? It's consistent.

I realize that (mostly for the sake of backward compatibility), this new feature of managing router config through a ConfigMap doesn't preclude the the possibility of doing so with annotations. What it does do, however, is increase complexity for an operator who's hunting down an issue by adding multiple sources of configuration. It also would add complexity to the documentation. (Since ConfigMap keys disallow the / character, as you have noted, the router.deis.io/ prefix on keys needs to be dropped. These seems to add a complex new dimension to configuration, the explanation for which will be too easily overlooked in the documentation, which will doubtless give some operators difficulties that we will need to assist them with.)

So...

I'm not saying we shouldn't do this. I'm not. But, if we were to do this, I think we need to have a really compelling answer for why. What is the tangible benefit of doing so?

jchauncey commented 8 years ago

I want to weigh in on this PR but I feel like this needs to be part of a larger architectural discussion. Can we schedule a 30 minute meeting to discuss the details and lay everything out? And try to come to a consensus.

krancour commented 8 years ago

@jchauncey absolutely.