support enabling webvalve in non-local, production-like envs #34

samandmoore · 2019-09-16T20:44:44Z

this changeset makes it so that webvalve has the following behavior:

when the env is test/development

webvalve is enabled and always runs in intercepting mode where
services are intercepted by default

when the env is NOT test/development, e.g. production

webvalve is disabled unless WEBVALVE_ENABLED=1/t/true
when WEBVALVE_ENABLED is truthy, webvalve is enabled in allowing mode
where all traffic is allowed by default
$SERVICE_ENABLED=0/f/false can be used to switch a service into intercepting
mode, i.e. enable the fake service
when WEBVALVE_ENABLED is truthy and
WEBVALVE_SERVICE_ENABLED_DEFAULT=0/f/false then webvalve is enabled in
intercepting mode where all traffic is intercepted by default
$SERVICE_ENABLED=1/t/true can be used to switch a service into
allowing mode, i.e. allow the traffic to that service to go through to
the internet

/domain @smudge @jmileham @klesse413 @effron
/no-platform

nanda-prbot · 2019-09-16T20:44:48Z

Needs somebody from @smudge, @jmileham, @klesse413, and @effron to claim domain review

Use the shovel operator to claim, e.g.:

@myname << domain && platform

lib/webvalve/fake_service_config.rb

smudge · 2019-09-16T20:59:07Z

lib/webvalve/fake_service_config.rb

-      WebValve::ENABLED_VALUES.include?(ENV["#{service_name.to_s.upcase}_ENABLED"])
+    def service_enabled_in_env?(default:)
+      value = ENV.fetch("#{service_name.to_s.upcase}_ENABLED", default).to_s
+      WebValve::ENABLED_VALUES.include?(value)


ENABLED_VALUES doesn't actually include true (boolean) -- only strings -- %w(1 t true)

Are we missing test coverage for the production env + webvalve_enabled=1 case?

i'm converting the true boolean to a string on line 52, which is why this works. i believe we have the proper coverage.

but i can add the non-string version for sanity.

smudge · 2019-09-16T21:00:40Z

<< domain TAFN -- my thought is that WebValve::ENABLED_VALUES.include?(true) will currently return false but I can't figure out where in the matrix of possible test cases that's gonna cause an issue.

nanda-prbot · 2019-09-16T21:00:48Z

@samandmoore needs to incorporate feedback from @smudge. Bump when done.

samandmoore · 2019-09-16T21:23:26Z

spec/webvalve/fake_service_config_spec.rb

-        it 'returns true' do
-          expect(subject.should_intercept?).to eq true
+        it 'still returns false by default' do
+          expect(subject.should_intercept?).to eq false


this test is RAILS_ENV=production, WEBVALVE_ENABLED=1

samandmoore · 2019-09-16T21:26:48Z

my thought is that WebValve::ENABLED_VALUES.include?(true) will currently return false but I can't figure out where in the matrix of possible test cases that's gonna cause an issue.

we always convert to string before comparing to ENABLED_VALUES entries, so we should be fine. we do this also because we're typically reading the value from the ENV which only yields strings

samandmoore · 2019-09-16T21:26:54Z

bump

nanda-prbot · 2019-09-16T21:27:00Z

Needs @smudge to provide domain review

When you finish a round of review, be sure to say you've finished or sign off on the PR, e.g.:

TAFN or DomainLGTM

If you're too busy to review, unclaim the PR, e.g.:

@myname >> domain

smudge · 2019-09-16T21:28:36Z

Oh got it, I just missed the to_s, I guess because I'm old and my eyesight is failing.

~~domain LGTM 🎉~~ (edited by @samandmoore because the code totally changed 😄)

nanda-prbot · 2019-09-16T21:28:38Z

Approved! 👌 💫 ⭐

samandmoore · 2019-09-17T23:12:17Z

still trying to figure out if we like this approach vs a slightly different one.

samandmoore · 2019-10-02T01:21:09Z

lib/webvalve/manager.rb

+
+    # @api private
+    def allowing?
+      !in_always_intercepting_env? && DISABLED_VALUES.include?(ENV['WEBVALVE_ENABLED'])


The more I look at this the more I want to rename this env variable to WEBVALVE_MODE and have it behave accordingly:

unset means off, webvalve doesn't even load

'allowing', webvalve loads and defaults to allowing internet connections

'intercepting', webvalve loads and defaults to disallowing internet connections

But, uh, I dunno. Gonna make a decision this week and see how it goes in real codebases with real use cases.

Unset vs Allowing feels conceptually very tricky, because in production they should behave identically (like, regardless of underlying stuff getting loaded or not). Like, I get where you're going with it, but unless you make a 3rd option called 'off' and make it explicitly togglable (like WEBVALVE_MODE=off) there's the potential for some surprises.

I still sort of stand by the logic that if you don't want this gem "on" in production (in the sense that it is loaded at runtime) then you should put it in the test/dev groups of the Gemfile like other non-prod dependencies. And that should be the README's default recommendation. Most devs get that soon as you put a gem like that in the production group, all bets are off, and I think that's okay...

What if the options are

WEBVALVE_ENABLED=1 intercepts all services by default and FAKE_[SERVICE]_ENABLED=0 selectively toggles the fakes off (enabling the real services), otherwise

WEBVALVE_ENABLED=0 (or, simply, unset) intercepts no services, and then FAKE_[SERVICE]_ENABLED=1 can be used to intercept select services.

WEBVALVE_ENABLED ends up meaning more "default all services to enabled"

WEBVALVE_ENABLED can default to 1 in dev/test -- and then prod-like envs that we want to totally isolate from real services can also set to 1.

(I see that you wrote an ADR on this so feel free to take or leave these comments -- I'm just trying to get these thoughts down while I have them.)

i actually wrote the ADR so that we could have this conversation a bit more concretely, so keep the thoughts coming 😄

i tried to make my peace with the unset vs 0 meaning different things problem by saying that it would only impact folks trying to use the more advanced behavior that the tools supports.

2. WEBVALVE_ENABLED=0 (or, simply, unset) intercepts no services, and then FAKE_[SERVICE]_ENABLED=1 can be used to intercept select services.

the concern i have with this approach is that i wanted enabling webvalve in a production-like environment to require a sort of 2 keys strategy. i.e. i want you to have to say "i'd like to use webvalve" and "i'd like webalve to enable fake this service"

the main motivation for that, for me, is safety. i don't know if it's wise to allow someone to enable a fake service with just the one toggle.

Maybe let's ditch WEBVALVE_ENABLED as an overloaded concept and go back to WEBVALVE_ENABLED=0 meaning it's fully off, nothing activated, and WEBVALVE_ENABLED=1 means it loads and enables RAILS_ENV-specific default behaviors.

In dev/test this means enabling all fakes, but in production the two-key process is this:

"i'd like to use webvalve"

WEBVALVE_ENABLED=1

"i'd like webvalve to enable fakes in this service"

WEBVALVE_SERVICE_ENABLED_DEFAULT=0 (this defaults to 1 in prod, 0 elsewhere)
or
[SERVICE]_ENABLED=0 (for select services you want to fake)

And then, optionally, we can eventually solve the confusion around "[service]_enabled" meaning the real service, not the fake service, by renaming these things and inverting their meaning:

WEBVALVE_FAKE_SERVICE_ENABLED_DEFAULT=1
FAKE_[SERVICE]_ENABLED=1

And we're okay with that first thing being super verbose because it's only for power users who have read all the docs 😅

implemented!

this changes the configuration of webvalve_enabled=1 for production-like environments to better align with our intuitions. So what if the defaults look like this: | | Dev/Test | Prod-like | | -----|-------- | ------------- | | `WEBVALVE_ENABLED` | true | false | | `[SERVICE]_ENABLED` | false | true | That way even if you set `WEBVALVE_ENABLED` in stage/prod/demo, you have to opt services _in_ to being faked instead of opting them out.

…disabled

…LED_DEFAULT this changeset makes it so that webvalve has the following behavior: when the env is test/development * webvalve is enabled and always runs in intercepting mode where services are intercepted by default when the env is NOT test/development, e.g. production * webvalve is disabled unless WEBVALVE_ENABLED=1/t/true * when WEBVALVE_ENABLED is truthy, webvalve is enabled in allowing mode where all traffic is allowed by default * $SERVICE_ENABLED=0/f/false can be used to switch a service into intercepting mode, i.e. enable the fake service * when WEBVALVE_ENABLED is truthy and WEBVALVE_SERVICE_ENABLED_DEFAULT=0/f/false then webvalve is enabled in intercepting mode where all traffic is intercepted by default * $SERVICE_ENABLED=1/t/true can be used to switch a service into allowing mode, i.e. allow the traffic to that service to go through to the internet

samandmoore · 2019-10-31T21:15:12Z

bump @smudge

wanna give this a thorough read? i believe it now provides our agreed upon implementation. 🕺

smudge · 2019-10-31T21:32:30Z

lib/webvalve/fake_service_config.rb

+    end
+
+    def explicitly_disabled?
+      value_from_env.present? && WebValve::DISABLED_VALUES.include?(value_from_env.to_s)


If I pass SERVICE_ENABLED=TRUE, the way this is written, both explicitly_enabled? and explicitly_disabled? will return false, right?

This might be why ActiveModel only includes FALSE_VALUES -- everything else (aside from nil and "") means true. https://github.com/rails/rails/blob/master/activemodel/lib/active_model/type/boolean.rb#L17

i have some explicit tests somewhere in here that show that a value like "this" is not considered truthy and i think the same for falsey for these methods. i'm happy to change that behavior, but what i was going for here is that the env variable being set doesn't do anything unless it's properly set to a true-like value or a false-like value. thoughts?

Got it -- so we say that SERVICE_ENABLED=something_invalid is neither explicitly enabled, nor explicitly disabled -- I buy it, but it doesn't feel like a super necessary distinction (like, as an end-developer I wouldn't be surprised if SERVICE_ENABLED=banana got cast to true) and my more general inclination would be to bool-cast ENV vars the same way everywhere and reduce the local complexity in basically any gem I write. (Like, normally I'd rely on ActiveModel's Boolean type, but that's not available in every lib. I wish Ruby had a built-in shorthand for boolean ENV vars.)

Yea. I'm down to change course in this PR. So what casts to false? Only "", nil, "f", "false", false, "0", and 0?

Yup, seems reasonable. Since this is only stringy I don't think you need false and 0, unless you are writing a more generic caster.

smudge · 2019-11-01T15:19:37Z

lib/webvalve/manager.rb

+      ENABLED_VALUES.include?(ENV['WEBVALVE_ENABLED'])
+    end
+
+    def services_enabled_by_default?


I wonder if in some earlier routine it would make sense to do something like this:

def set_defaults! if Webvalve.env.in?(ALWAYS_ENABLED_ENVS) if ENV.key? 'WEBVALVE_SERVICE_ENABLED_DEFAULT' WebValve.logger.warn SERVICE_ENABLED_DEFAULT_WARNING end if ENV.key? 'WEBVALVE_ENABLED' WebValve.logger.warn WEBVALVE_ENABLED_WARNING end ENV['WEBVALVE_ENABLED'] = '1' ENV['WEBVALVE_SERVICE_ENABLED_DEFAULT'] = '0' else ENV['WEBVALVE_ENABLED'] ||= '0' ENV['WEBVALVE_SERVICE_ENABLED_DEFAULT'] ||= '1' end end

And then inside of Manager there doesn't need to be a concept of "always" or "explicit" - it can more naively trust that the two ENV vars are set and mean what they say they mean:

def enabled? ENABLED_VALUES.include?(ENV.fetch('WEBVALVE_ENABLED')) end def services_enabled_by_default? ENABLED_VALUES.include?(ENV.fetch('WEBVALVE_SERVICE_ENABLED_DEFAULT')) end def allowing? enabled? && services_enabled_by_default? end def intercepting? enabled? && !services_enabled_by_default? end

smudge · 2019-11-01T15:27:17Z

Yep, this feels great -- I made one suggestion here that, at the very least, could reduce the testing burden on the Manager class, because you could set and test default behaviors independently, and then trust that in ALWAYS_ENABLED_ENVS we've overridden the inputs to mean what they claim to mean.

Buuuut I don't want to keep blocking you on this, especially since the interface itself shouldn't need to change if we decide to do any such refactoring later on. So feel free to take or leave those implementation detail suggestions.

domain LGTM 🎉

nanda-prbot · 2019-11-01T15:27:27Z

Approved! 🎸 🎏 🌟

nanda-prbot added the domain-not-claimed label Sep 16, 2019

smudge reviewed Sep 16, 2019

View reviewed changes

lib/webvalve/fake_service_config.rb Outdated Show resolved Hide resolved

smudge reviewed Sep 16, 2019

View reviewed changes

nanda-prbot removed the domain-not-claimed label Sep 16, 2019

samandmoore force-pushed the sam-production-faking branch from 7e870ec to ddd8202 Compare September 16, 2019 21:22

samandmoore commented Sep 16, 2019

View reviewed changes

nanda-prbot added the approved label Sep 16, 2019

samandmoore changed the title ~~default services to enabled with webvalve_enabled=1 in production~~ [no-merge] default services to enabled with webvalve_enabled=1 in production Sep 17, 2019

samandmoore commented Oct 2, 2019

View reviewed changes

samandmoore force-pushed the sam-production-faking branch from 46d70a7 to 74fc863 Compare October 16, 2019 19:35

samandmoore added 11 commits October 31, 2019 12:16

move the to_s so it's more obvious. update changelog and version

fc49f1e

fix typo

36a6381

clarify test language a bit

3f10dc6

okay, here's a better approach

1468bbd

in on+allowing mode, only install webmock if at least one service is …

1183e2c

…disabled

update manager specs

63b5309

update fake service config specs

e778e8e

add exhaustive specs for the envs

20a6091

okay, test coverage, complete?

19eb0d4

fix disabled reference

0752a61

samandmoore added 2 commits October 31, 2019 12:16

slight readme tweak

4e8fd68

add ADR for new features

04d2aaf

samandmoore force-pushed the sam-production-faking branch from 74fc863 to 04d2aaf Compare October 31, 2019 16:17

samandmoore added 2 commits October 31, 2019 17:01

update the ADR

2bd0c8b

samandmoore added 2 commits October 31, 2019 17:17

update changelog

db4947c

update readme

820e982

smudge reviewed Oct 31, 2019

View reviewed changes

delete notes

c51648b

smudge reviewed Nov 1, 2019

View reviewed changes

This was referenced Nov 4, 2019

refactor activation logic / defaults to simplify Manager class #37

Open

use ActiveModel-style FALSE_VALUES approach for ENV variables #38

Open

samandmoore changed the title ~~[no-merge] default services to enabled with webvalve_enabled=1 in production~~ support enabling webvalve in non-local, production-like envs Nov 4, 2019

samandmoore merged commit bb2dbcd into master Nov 4, 2019

samandmoore deleted the sam-production-faking branch November 4, 2019 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support enabling webvalve in non-local, production-like envs #34

support enabling webvalve in non-local, production-like envs #34

samandmoore commented Sep 16, 2019 •

edited

Loading

nanda-prbot commented Sep 16, 2019

smudge Sep 16, 2019

samandmoore Sep 16, 2019

samandmoore Sep 16, 2019

smudge commented Sep 16, 2019

nanda-prbot commented Sep 16, 2019

samandmoore Sep 16, 2019

samandmoore commented Sep 16, 2019

samandmoore commented Sep 16, 2019

nanda-prbot commented Sep 16, 2019

smudge commented Sep 16, 2019 •

edited by samandmoore

Loading

nanda-prbot commented Sep 16, 2019

samandmoore commented Sep 17, 2019

samandmoore Oct 2, 2019

smudge Oct 16, 2019

smudge Oct 16, 2019

smudge Oct 16, 2019 •

edited

Loading

smudge Oct 16, 2019

samandmoore Oct 17, 2019

samandmoore Oct 17, 2019

smudge Oct 17, 2019 •

edited

Loading

smudge Oct 17, 2019

samandmoore Oct 31, 2019

samandmoore commented Oct 31, 2019

smudge Oct 31, 2019

samandmoore Oct 31, 2019

smudge Oct 31, 2019

samandmoore Oct 31, 2019

smudge Nov 1, 2019

smudge Nov 1, 2019 •

edited

Loading

smudge commented Nov 1, 2019

nanda-prbot commented Nov 1, 2019

support enabling webvalve in non-local, production-like envs #34

support enabling webvalve in non-local, production-like envs #34

Conversation

samandmoore commented Sep 16, 2019 • edited Loading

nanda-prbot commented Sep 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge commented Sep 16, 2019

nanda-prbot commented Sep 16, 2019

Choose a reason for hiding this comment

samandmoore commented Sep 16, 2019

samandmoore commented Sep 16, 2019

nanda-prbot commented Sep 16, 2019

smudge commented Sep 16, 2019 • edited by samandmoore Loading

nanda-prbot commented Sep 16, 2019

samandmoore commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge Oct 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge Oct 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samandmoore commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smudge Nov 1, 2019 • edited Loading

Choose a reason for hiding this comment

smudge commented Nov 1, 2019

nanda-prbot commented Nov 1, 2019

samandmoore commented Sep 16, 2019 •

edited

Loading

smudge commented Sep 16, 2019 •

edited by samandmoore

Loading

smudge Oct 16, 2019 •

edited

Loading

smudge Oct 17, 2019 •

edited

Loading

smudge Nov 1, 2019 •

edited

Loading