Align env var with RFC #1938
Align env var with RFC #1938
Conversation
|
This change looks reasonable:
|
Duh, I confused that with another PR (#1937). There should be specs indeed on that one, hence why I left it as a draft and planned to add some. |
lloeki
force-pushed
the
appsec-change-rules-env-var
branch
from
9bc2ba6
to
24c9547
Compare
lloeki
force-pushed
the
appsec-change-rules-env-var
branch
from
24c9547
to
c1be54b
Compare
There was a problem hiding this comment.
I had a lot of tiny suggestions and was prepared to press the approve button but then found that in the appsec/configuration/settings_spec.rb we have quite a lot of duplication when compared to extensions_spec.rb so...
I'm curious to know more about that one.
I don't think it's a blocker, so if you tell me "I really want it like this", I can live with it, but, yeah, it gave me bit of pause.
P.s.: You may want to rebase on top of current master, I fixed the CI issues with Ruby 2.3.
| end | ||
| end | ||
|
|
||
| describe Datadog::AppSec::Configuration::Settings do |
There was a problem hiding this comment.
Minor: This is repeated from the top-level describe :)
There was a problem hiding this comment.
duh, I claim brain bug on my side!
|
|
||
| let(:dsl) { Datadog::AppSec::Configuration::DSL.new } | ||
|
|
||
| after { settings.send(:reset!) } |
There was a problem hiding this comment.
This doesn't seem to do anything? The settings on the class seem all to be instance-level data, and RSpec creates a new instance for each test case so this should not make a difference.
| describe '#enabled' do | ||
| subject(:enabled) { settings.enabled } | ||
| it { is_expected.to eq(true) } | ||
| end |
There was a problem hiding this comment.
This confused me a lot!
The default on Datadog::AppSec::Configuration::Settings class is to have this as false so it took me a while to realize that in datadog/appsec/spec_helper.rb there was a allow(ENV).to receive(:[]).with('DD_APPSEC_ENABLED').and_return('true').
This doesn't seem to match the rest of the tests -- they all check the defaults in DEFAULTS and then separately test the env-updated behaviors -- so should this one be updated to also test that the default is false?
There was a problem hiding this comment.
Indeed this feels weird to have it set in spec_helper. I actually did not think much of the value being true instead of false...
| require 'datadog/appsec/spec_helper' | ||
|
|
||
| RSpec.describe Datadog::AppSec::Configuration::Settings do | ||
| shared_context 'registry with integration' do |
There was a problem hiding this comment.
Minor: Since this "registry" is actually not shared (it only gets included at the top-level, once), we could just just remove the shared_context and make these lets be part of the top-level describe and have the same effect ;)
There was a problem hiding this comment.
I kept it because I feel it should be reusable by other specs, but it probably doesn't belong in this file anymore.
| describe '#ruleset=' do | ||
| subject(:ruleset_) { settings.merge(dsl.tap { |c| c.ruleset = :risky }) } | ||
| it { expect { ruleset_ }.to change { settings.ruleset }.from(:recommended).to(:risky) } | ||
| end |
There was a problem hiding this comment.
The behavior of this method seems somewhat... weird. From the code, it seems like you can set it to :recommended, :risky, '/or/a/string/with/a/path/to/a/json/file'.
Am I getting this correct? Would it be worth documenting this as a comment somewhere? For instance, I'm unclear about the edge cases -- can you do DD_APPSEC_RULES=risky as well?
There was a problem hiding this comment.
Indeed, this setting is settable to symbols to use one of the vendored presets, but the env var does not allow selection of these rulesets yet. This (and some other differences in env var vs dsl) will be addressed in a separate PR but is out of the scope of this one.
| describe '#waf_timeout' do | ||
| subject(:waf_timeout) { settings.waf_timeout } | ||
| it { is_expected.to eq(5000) } | ||
| end | ||
|
|
||
| describe '#waf_timeout=' do | ||
| subject(:waf_timeout_) { settings.merge(dsl.tap { |c| c.waf_timeout = 3 }) } | ||
| it { expect { waf_timeout_ }.to change { settings.waf_timeout }.from(5000).to(3) } | ||
| end |
There was a problem hiding this comment.
Minor: Have you considered documenting in the method name that this value is in microseconds e.g.waf_timeout_microseconds?
There was a problem hiding this comment.
This setter should accept durations with units (as strings like '1s'), like its sibling env var does. The env var is defined across libs as being DD_APPSEC_WAF_TIMEOUT and I did not want to introduce inconsistency. That said, it should be documented in the getter/setter docstring.
| describe '#trace_rate_limit' do | ||
| subject(:trace_rate_limit) { settings.trace_rate_limit } | ||
| it { is_expected.to eq(100) } | ||
| end | ||
|
|
||
| describe '#trace_rate_limit=' do | ||
| subject(:trace_rate_limit_) { settings.merge(dsl.tap { |c| c.trace_rate_limit = 2 }) } | ||
| it { expect { trace_rate_limit_ }.to change { settings.trace_rate_limit }.from(100).to(2) } | ||
| end |
There was a problem hiding this comment.
Minor: A bit like waf_timeout above, would it be worth stating in the method name that this is a trace_rate_limit_per_second or something?
There was a problem hiding this comment.
It's actually trace_rate_limit_per_second_per_concurrency_unit (or _per_thread, or _per_fiber since it lives in a fiber local). Again I'd rather have proper docstrings explaining the unit+behaviour of the feature (even though this one is not considered public API)
| before do | ||
| allow(ENV).to receive(:[]).with('DD_APPSEC_ENABLED').and_return('1') | ||
| end |
There was a problem hiding this comment.
Minor: Usually to modify env vars in the dd-trace-rb specs we end up using ClimateControl, e.g.
around { |example| ClimateControl.modify('DD_PROFILING_NO_EXTENSION' => nil) { example.run } }
There was a problem hiding this comment.
Oh, I did not know that and found references to a similar approach in the specs, so did not look further.
TBH I'm not too sure of the added value of ClimateControl (is that mainly about this synchronize? Are tests ever run multithreaded?) and kind of find it less readable as well as being unable to assert whether ENV has received :[] for a given key.
There was a problem hiding this comment.
One thing I think is an advantage is when debugging tests. When I was looking at the DD_APPSEC_ENABLED tests I was getting pretty confused where the env var was coming from because I was printing ENV and did not see it there, but ENV['DD_APPSEC_ENABLED'] returned a value.
But... I don't feel too strongly, just thought I'd mention it :)
| describe '#enabled=' do | ||
| subject(:enabled_) { settings.merge(dsl.tap { |c| c.enabled = false }) } | ||
| it { expect { enabled_ }.to change { settings.enabled }.from(true).to(false) } | ||
| end |
There was a problem hiding this comment.
Minor: This seems somewhat redundant. Since none of the setters depend on the env var, I don't think it's worth repeating them again, I suggest simplifying this.
There was a problem hiding this comment.
The tested behaviour here is that the env var should:
a) have less priority than the DSL setters (final value)
b) still be picked up before the DSL kicks in (change, notably from a non-default to a non-default)
This was actually useful to test because I triggered a small bug with it (that affected only specs, not real life apps) when this example was in spec/datadog/appsec/extensions_spec.rb.
|
|
||
| describe '#enabled=' do | ||
| subject(:enabled_) { settings.enabled = false } | ||
| it do | ||
| expect { enabled_ }.to change { settings.enabled }.from(true).to(false) | ||
| end | ||
| it { expect { enabled_ }.to change { settings.enabled }.from(true).to(false) } | ||
| end | ||
|
|
There was a problem hiding this comment.
⬆️ I can't comment on the exact lines above, but this whole spec seems weird.
This is not a spec for Datadog::Core::Configuration::Settings as the describe claims, this is a spec for Settings.new.appsec.
And... it quite repeats the new spec/datadog/appsec/configuration/settings_spec.rb. My suggestion would be to avoid doing so -- by having this spec check that Settings.new.appsec correctly delegates to a Datadog::AppSec::Configuration::Settings but not actually testing it through here.
That, or perhaps removing the Datadog::AppSec::Configuration::Settings and testing it completely through here. Having a spec that is almost copy-pasted from the other seems weird...?
There was a problem hiding this comment.
this whole spec seems weird
Yes it is. @marcotc added the extension spec to ensure that propagation of these values happens correctly but the subject being Settings.new.appsec defeats the purpose. I chose to implement a spec for Datadog::AppSec::Configuration::Settings because it felt more logical/unit to have these examples on the instance itself instead of going through and depending on a bigger chunk of code.
Interestingly enough the bug I mentioned in another comment is that reset! doesn't quite work because there's some memoization happening somewhere in or around that bigger chunk of code that escapes reset attempts and adding more test cases makes them fail randomly depending on ordering for reasons that only happen in specs.
My suggestion would be to avoid doing so -- by having this spec check that Settings.new.appsec correctly delegates to a Datadog::AppSec::Configuration::Settings but not actually testing it through here.
Agreed.
There was a problem hiding this comment.
As discussed during the 2022-03-22 team meeting, there's still quite a bit of iteration that it would be nice to see on the tests, but the production code change we're quite happy with, so I'm approving this PR, any extra test changes will be done on a separate PR, to unblock release of beta2.
Current RFC-defined env vars are:
DD_APPSEC_ENABLEDDD_APPSEC_RULES# This one is currently not compliant, which this PR addressesDD_APPSEC_WAF_TIMEOUT