Repository navigation
Accept only the evidence each element reads, and none by default - #63
Draft
jwrosewell wants to merge 8 commits into
Draft
jwrosewell wants to merge 8 commits into
jwrosewell wants to merge 8 commits into
Conversation
…, the JSON bundler and the set header element read
… that accepts every key, and test what is no longer sent
…e sends only what the service lists
…est engine's results
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For #52.
The problem
The elements
PipelineBuilderadds by default, and the cloud engines, accepted every evidence key, so a pipeline accepted every header,If-None-Matchincluded, whatever its engines read. The client-side endpoints' ETag and Vary header (#59) are built from what the pipeline accepts, so Vary named every header, and only #59's list of revalidation headers kept the ETag of a revalidation the same. A cloud pipeline also sent every header and cookie in the evidence to the cloud service, whether or not the service lists it.SequenceElementEvidenceKeyFilterandJavaScriptBuilderEvidenceKeyFilterdefinedfilter(), which nothing calls, so the baseEvidenceKeyFilter.filter_evidence_key(), which returns true, answered for them (sequenceelement.py, javascriptbuilder.py).SetHeaderElement,JSONBundlerElementandCloudEnginedo not overrideget_evidence_key_filter(), so they tookFlowElement's, which accepts every key (flowelement.py). Three elements the pipeline builder adds accept every evidence key #52 named the first three. The JSON bundler is the same case, and the builder adds it with the JavaScript builder.CloudEngineis the base ofDeviceDetectionCloud,HardwareProfileCloud,IpIntelligenceCloudandLocationCloud, so every cloud pipeline accepted every key through them.The change
As James decided on 7 November, every element accepts only the evidence that can change its results.
query.session-idandquery.sequence. The result changes as the sequence grows, so they belong in the ETag.query.keys,header.hostandheader.protocol, tested withkey.startswith("query.")where it was"query" in key, whichheader.x-queryalso passed.CloudEnginereads only the cloud request engine's results, so it accepts nothing, and so do the four cloud engines built on it. The .NET cloud engines already accept nothing (DeviceDetectionCloudEngine.cs line 74).filter_evidence_key, so they are called.FlowElement.get_evidence_key_filter(), the default for an element that does not give its own filter, accepts no evidence where it accepted every key. specifications#103 says a base Flow Element's default MUST accept no Evidence. .NET and Java already make every element give its own filter, and Go's default accepts none.The JavaScript builder reads the query values through its own filter (
javascriptbuilder.pyline 401) and the host and protocol directly (lines 363 and 372), all of which it still accepts, and it used only thequery.keys of what that filter returned, so the script it writes is unchanged. The sequence element reads only its two keys.A cloud pipeline now keeps the evidence the cloud request engine accepts, being the keys the service lists at its
evidencekeysendpoint, and the keys the sequence element and the JavaScript builder read. The cloud request engine sends the evidence the pipeline keeps, so it no longer sends the service other headers and cookies. The live service listed 138 keys on 8 November, 25 of them cookies.Four cloud request engine tests relied on the old behaviour, because the stand-in service lists
query.User-Agentalone and they gave other evidence. They now add a test element that accepts every key (tests/classes/accepteverykeyelement.py).How to verify
Each red run is
rcat 0507eef, or this branch before the fix, with only the tests added, and each green run is this branch with the same temporary workflow.If-None-Match, and with Set the client-side cache headers, with an ETag that is the same in every process and none after errors #59 merged in the Vary header named every header the request carried.fiftyone_pipeline_core289 passed on the branch and 301 with Set the client-side cache headers, with an ETag that is the same in every process and none after errors #59 merged in, withVary: Host, User-Agentand the revalidation answered 304.fiftyone_pipeline_engines9,fiftyone_pipeline_engines_fiftyone34 andfiftyone_pipeline_translation19 passed, and the examples' offline web tests 11, or 18 with Set the client-side cache headers, with an ETag that is the same in every process and none after errors #59.test_evidence_precedence_2,_4and_5andtest_reserved_evidence_is_left_out_of_the_body(run 42425200931), for the reason above. With the test element (ab865bb), the newtest_evidence_no_element_accepts_is_not_sentfailed onrc(run 42426589244) and all 32 passed on the branch (run 42426594361).nightly-pull-request.yml, run as a dry run from a temporary branch, built 6cb8928, this branch's head, merged withrc(run 42471441691). All 15 build and test jobs passed, on Ubuntu, Windows and macOS with Python 3.10 to 3.14.The new tests are these.
tests/test_default_elements_accept.pyinfiftyone_pipeline_core.test_a_default_pipeline_accepts_only_what_its_elements_read, a default pipeline with an element acceptingheader.user-agent, does not acceptheader.if-none-match,header.x-test,header.x-query,header.cookie,cookie.queryorserver.client-ip, and accepts the user agent, the two sequence keys, aquery.key,header.hostandheader.protocol.test_each_element_accepts_only_what_it_readschecks each of the four elements.test_processing_sets_aside_the_evidence_no_element_readschecks thatprocess()leavesheader.if-none-matchout of the evidence.test_only_the_headers_the_elements_read_are_named_for_varychecks that of a request's headers onlyHostandUser-Agentare ones the pipeline accepts, which is what Set the client-side cache headers, with an ETag that is the same in every process and none after errors #59's Vary names. With Set the client-side cache headers, with an ETag that is the same in every process and none after errors #59 merged in, a temporary test checked the Vary header itself, and that a revalidation got a 304.test_evidence_no_element_accepts_is_not_sentintests/test_credentials.pychecks that a header and a cookie the stand-in service does not list are not sent to it.tests/test_cloudengine_evidence.pyinfiftyone_pipeline_cloudrequestengine.test_a_cloud_engine_accepts_no_evidencechecks query, header, cookie, sequence and server keys.test_a_cloud_pipeline_sends_only_the_evidence_the_service_listschecks that with a cloud engine in the pipeline the user agent is sent and the header and cookie the service does not list are not.An element that declares no evidence
Every element in this repository gives its own filter, apart from test elements and the cloud engines, which take
CloudEngine's. No test changed for the new default. With the tests alone (06a5826), the two new tests intest_default_elements_accept.pyfailed and every other test passed (run 42471232966). With the change (6cb8928), all passed (run 42471233048). Both runs coveredfiftyone_pipeline_core(291 tests),fiftyone_pipeline_engines(9),fiftyone_pipeline_engines_fiftyone(34),fiftyone_pipeline_translation(19), the four cloud request engine test files that need no key (39) andfiftyone_ipintelligence_translation(26), on Ubuntu with Python 3.12.test_an_element_that_declares_nothing_accepts_nothingchecks the default rejects every key.test_an_element_that_declares_nothing_widens_nothingchecks that adding such an element to a pipeline does not make it accept more.A customer's own element that relied on the default no longer receives evidence that no other element accepts, and it now has to give its own filter, as the release note says. #59 keeps its list of revalidation headers, as an element of a customer's own can still accept them.
Release note
Other drafts on the same files
None of the open drafts changes these files. #59 and #61 change
web.pyandevidence.pybeside them.What remains
fiftyone_pipeline_coreandfiftyone_pipeline_cloudrequestenginechange, so both are released, and so are the packages that depend on them.