feat: Add support for multiple ADM endpoints #346

jrconlin · 2022-01-19T00:30:45Z

ADM requires different endpoint data for mobile vs non-mobile tile
requests.

This patch also changes AdmSettings to AdmFilterSettings since
that's more explicit about what they are.

Issue: CONSVC-1562

ADM requires different endpoint data for mobile vs non-mobile tile requests. This patch also changes `AdmSettings` to `AdmFilterSettings` since that's more explicit about what they are. Issue: #345, CONSVC-1562

src/settings.rs

pjenvey · 2022-01-22T00:09:24Z

src/adm/settings.rs

+    /// Return the information for a mobile connection
+    pub fn as_mobile(settings: &Settings) -> Self {
+        let default = Self::as_default(settings);
+        AdmPse {


Instead of cloning the bits to create new AdmPses every time we could construct them once at startup and store them on ServerState. I don't care much about these extra clones though so this as is is ok.

src/adm/settings.rs

src/adm/tiles.rs

ncloudioj · 2022-01-25T19:25:35Z

src/web/user_agent.rs

+
+    /// Determine if the device is a mobile phone based on either the form factor or OS.
+    pub fn is_mobile(&self) -> bool {
+        self.form_factor == FormFactor::Phone


@jrconlin Just confirmed with the mobile team, we do have mobile traffic on "tablets", and they wanted Contile to respond to tile requests from there. So let's include FormFactor::Tablet here as well.

@ncloudioj Should we run this by adM as well?

Yeah, we will confirm this with them in our next meeting.

src/web/user_agent.rs

Firefox on iPad returns the default desktop UA (apparently per Apple directive).

src/web/user_agent.rs

* allow for out-of-area tiles to receive empty 200 lists (#290) * removed `position` from response (current PR) * fixed response for iPad (current PR)

jrconlin · 2022-01-27T00:10:49Z

Now blocking on mozilla-services/contile-integration-tests#147

hackebrot

Hi team! 👋🏻

This is my understanding of the feature request:

Send requests for tiles for mobile clients to a different partner API endpoint
Make that URL configurable via CONTILE_ADM_MOBILE_ENDPOINT_URL

Unrelated to the partner mobile API, we’re also removing the deprecated settings sub1 and partner_id. We're adding adm_mobile_partner_id and adm_mobile_sub1 for the mobile API.

Does that sound correct to you?

src/adm/settings.rs

jrconlin · 2022-01-27T15:50:16Z

This is correct.
This patch contains a number of fixes and changes which are products of the recent mobile endpoint changes. These normally get rolled up into the final commit message, but I will list them here for clarity.

Add handling to send "mobile" devices to the new ADM mobile endpoint, with appropriate partner_id, and sub1
Add settings arguments for the new mobile specific arguments.
Remove the obsolete sub1 and partner_id values (since we can no longer warn on their use.)
"Correctly" identify firefox on iPad UserAgent string (which uses the default macos desktop UA per guidance from the mobile team and Apple)
Remove the "position" field from outbound data since it is not used by User Agents and is complicating deployment. (Issue The "postion" field is not always populated in the response of Contile #347)
Adjust integration testing to account for above.

…mobile-endpoint

Changes made. Please re-review.

hackebrot · 2022-02-03T10:09:34Z

I merged main and resolved the merge conflicts.

…mobile-endpoint

…ntile into feat/mobile-endpoint

pjenvey

LGTM just needs signoff from @hackebrot

jrconlin · 2022-02-04T23:05:05Z

I'd also appreciate a drive by from @ncloudioj to make sure that the ADM calls are correct

hackebrot

Hi @jrconlin @pjenvey! 👋🏻

I've added my comments inline. I recommend using distinct values for partner parameters to make it easier to debug issues. Otherwise the changes look reasonable to me.

I appreciate you adding the comments to the integration test files. That's helpful! 👍🏻

hackebrot · 2022-02-07T14:36:54Z

integration-tests/docker-compose.yml

      CONTILE_ADM_QUERY_TILE_COUNT: 5
      CONTILE_ADM_SETTINGS: /tmp/contile/adm_settings.json
+      CONTILE_ADM_SUB1: test


Suggested change

CONTILE_ADM_SUB1: test

CONTILE_ADM_SUB1: sub1_test

I suppose. Although, we aren't really checking the parameters in the mock parent script.

hackebrot · 2022-02-07T14:37:10Z

integration-tests/docker-compose.yml

      CONTILE_ADM_QUERY_TILE_COUNT: 5
      CONTILE_ADM_SETTINGS: /tmp/contile/adm_settings.json
+      CONTILE_ADM_SUB1: test
+      CONTILE_ADM_PARTNER_ID: test


Suggested change

CONTILE_ADM_PARTNER_ID: test

CONTILE_ADM_PARTNER_ID: partner_id_test

hackebrot · 2022-02-07T14:37:29Z

integration-tests/docker-compose.init_error.yml

@@ -3,6 +3,8 @@ version: "3"
 services:
  contile:
    environment:
+      CONTILE_ADM_SUB1: test


Suggested change

CONTILE_ADM_SUB1: test

CONTILE_ADM_SUB1: sub1_test

hackebrot · 2022-02-07T14:39:46Z

integration-tests/docker-compose.init_error.yml

@@ -3,6 +3,8 @@ version: "3"
 services:
  contile:
    environment:
+      CONTILE_ADM_SUB1: test
+      CONTILE_ADM_PARTNER_ID: test


Suggested change

CONTILE_ADM_PARTNER_ID: test

CONTILE_ADM_PARTNER_ID: partner_id_test

hackebrot · 2022-02-07T14:53:43Z

integration-tests/volumes/client/scenarios.yml

@@ -141,7 +133,7 @@ scenarios:
            errno: 520
            error: 'An error occurred while trying to request data'

-  - name: error_tablet_ios_reqwest_error
+  - name: error_tablet_ios_reqwest


Hmm wondering if we should revert that name change.

The error in the web application is a reqwest error. 🤔

integration-tests/volumes/client/scenarios.yml

hackebrot · 2022-02-07T14:57:34Z

integration-tests/volumes/client/scenarios_204.yml

@@ -1,6 +1,9 @@
 scenarios:
  - name: success_204_No_Content_exluded_region
    description: Test that Contile returns a 204 No Content for excluded regions
+    # This test uses the Environment Flag "CONTILE_EXCLUDED_COUNTRIES_200" (specified
+    # in the `docker-compose.204.yml` file) and checks that Contile returns a 204 with
+    # no content if a request is made to an excluded country location.


Thank you for adding this comment! 📝

Suggested change

# no content if a request is made to an excluded country location.

# no content if a request is made from an excluded country location.

hackebrot · 2022-02-07T15:14:30Z

src/web/user_agent.rs

-
+    let mut wresult = Parser::new().parse(ua).unwrap_or_default();
+
+    // NOTE: Firefox on iPads report back the "desktop" UA


It's Safari on Desktop isn't it?

Suggested change

// NOTE: Firefox on iPads report back the "desktop" UA

// NOTE: Firefox on iPads report back the Safari "desktop" UA

hackebrot · 2022-02-07T15:19:59Z

src/web/test.rs

    let tile2 = &tiles[1];
    assert_eq!(tile2["name"], "Los Pollos Hermanos");
-    assert_eq!(tile2["position"], 2);


Do we have any unit test coverage for position not being included in the response content? We could invert this and the above change to check for that.

Don't the strict integration tests handle this? Adding a unit test for this feels like trying to prove a negative.

Yes, that is correct. If Contile returns position and an expected response content in scenarios.yml for a 200 OK scenario doesn't have position the comparison of Python dicts would fail.

hackebrot

LGTM! 👍🏻

Granted that @ncloudioj approves of these partner related changes, I'm happy for you to merge this pull request. 📱

chore: Update to edition 2021

8a1ccca

jrconlin requested review from pjenvey and ncloudioj January 19, 2022 00:30

feat: Add support for multiple ADM endpoints

8d3015a

ADM requires different endpoint data for mobile vs non-mobile tile requests. This patch also changes `AdmSettings` to `AdmFilterSettings` since that's more explicit about what they are. Issue: #345, CONSVC-1562

jrconlin force-pushed the feat/mobile-endpoint branch from fcf2fbb to 8d3015a Compare January 19, 2022 00:34

jrconlin added 2 commits January 19, 2022 16:14

f int test

a7f5688

f use default values if no mobile value present

bc8b4ee

pjenvey suggested changes Jan 22, 2022

View reviewed changes

f r's

ce9b5e5

pjenvey previously approved these changes Jan 25, 2022

View reviewed changes

jrconlin added 2 commits January 25, 2022 08:12

f Update to address RUSTSEC-2021-0131, RUSTSEC-2022-0006

d1fae94

f remove fixed CVE from audit.toml

5013be6

jrconlin dismissed pjenvey’s stale review via 5013be6 January 25, 2022 16:14

jrconlin requested a review from pjenvey January 25, 2022 16:14

ncloudioj reviewed Jan 25, 2022

View reviewed changes

f add tablet to mobile types

d95c4a6

pjenvey reviewed Jan 25, 2022

View reviewed changes

src/web/user_agent.rs Outdated Show resolved Hide resolved

jrconlin added 2 commits January 25, 2022 15:20

f correctly(?) identify iPad firefox UA

63fa1d2

Firefox on iPad returns the default desktop UA (apparently per Apple directive).

f cargo update

4b1f83a

pjenvey suggested changes Jan 26, 2022

View reviewed changes

src/web/user_agent.rs Outdated Show resolved Hide resolved

jrconlin added 4 commits January 25, 2022 16:42

f r's

3894bdc

f fix integration tests for tablet.

da012bd

f remove position field

1d3e3ad

f fix up integration testing

e22a970

* allow for out-of-area tiles to receive empty 200 lists (#290) * removed `position` from response (current PR) * fixed response for iPad (current PR)

jrconlin requested a review from hackebrot January 27, 2022 00:06

hackebrot suggested changes Jan 27, 2022

View reviewed changes

hackebrot reviewed Jan 27, 2022

View reviewed changes

src/adm/settings.rs Outdated Show resolved Hide resolved

f r's

7bfc226

jrconlin added 2 commits February 1, 2022 15:25

f update contile-integration-tests to latest version

b479b99

f fix docker

9b65beb

jrconlin requested review from hackebrot and pjenvey February 2, 2022 00:20

pjenvey previously approved these changes Feb 2, 2022

View reviewed changes

jrconlin added 3 commits February 2, 2022 07:41

f cargo update

70580e1

f fix macos UA to not be windows

0f3844f

Merge branch 'main' of github.com:mozilla-services/contile into feat/…

63739c9

…mobile-endpoint

jrconlin dismissed pjenvey’s stale review via 63739c9 February 2, 2022 17:31

jrconlin requested a review from pjenvey February 2, 2022 23:58

pjenvey previously approved these changes Feb 3, 2022

View reviewed changes

Merge branch 'main' into feat/mobile-endpoint

3bd8b12

jrconlin mentioned this pull request Feb 3, 2022

bug: report error from image library #349

Merged

jrconlin added 3 commits February 3, 2022 17:22

Merge branch 'main' into feat/mobile-endpoint

b5586d8

Merge branch 'main' of github.com:mozilla-services/contile into feat/…

3c74d91

…mobile-endpoint

Merge branch 'feat/mobile-endpoint' of github.com:mozilla-services/co…

689b26d

…ntile into feat/mobile-endpoint

jrconlin dismissed pjenvey’s stale review via 689b26d February 4, 2022 22:43

pjenvey previously approved these changes Feb 4, 2022

View reviewed changes

hackebrot suggested changes Feb 7, 2022

View reviewed changes

f r's

478fa01

jrconlin dismissed pjenvey’s stale review via 478fa01 February 7, 2022 16:33

jrconlin requested a review from hackebrot February 7, 2022 17:24

pjenvey approved these changes Feb 7, 2022

View reviewed changes

hackebrot approved these changes Feb 8, 2022

View reviewed changes

jrconlin merged commit 80d7dca into main Feb 8, 2022

jrconlin deleted the feat/mobile-endpoint branch February 8, 2022 18:39

jrconlin mentioned this pull request Feb 8, 2022

Support an alternative adm_endpoint_url for the mobile form_factor #345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for multiple ADM endpoints #346

feat: Add support for multiple ADM endpoints #346

jrconlin commented Jan 19, 2022

pjenvey Jan 22, 2022

ncloudioj Jan 25, 2022

pjenvey Jan 25, 2022 •

edited

Loading

ncloudioj Jan 25, 2022

jrconlin commented Jan 27, 2022

hackebrot left a comment •

edited

Loading

jrconlin commented Jan 27, 2022

hackebrot commented Feb 3, 2022

pjenvey left a comment

jrconlin commented Feb 4, 2022

hackebrot left a comment

hackebrot Feb 7, 2022

jrconlin Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

hackebrot Feb 7, 2022

jrconlin Feb 7, 2022

hackebrot Feb 8, 2022

hackebrot left a comment

	CONTILE_ADM_PARTNER_ID: test
	CONTILE_ADM_PARTNER_ID: partner_id_test

	# no content if a request is made to an excluded country location.
	# no content if a request is made from an excluded country location.


		let mut wresult = Parser::new().parse(ua).unwrap_or_default();

		// NOTE: Firefox on iPads report back the "desktop" UA

feat: Add support for multiple ADM endpoints #346

feat: Add support for multiple ADM endpoints #346

Conversation

jrconlin commented Jan 19, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjenvey Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrconlin commented Jan 27, 2022

hackebrot left a comment • edited Loading

Choose a reason for hiding this comment

jrconlin commented Jan 27, 2022

hackebrot commented Feb 3, 2022

pjenvey left a comment

Choose a reason for hiding this comment

jrconlin commented Feb 4, 2022

hackebrot left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackebrot left a comment

Choose a reason for hiding this comment

pjenvey Jan 25, 2022 •

edited

Loading

hackebrot left a comment •

edited

Loading