Closed Bug 1365089 Opened 7 years ago Closed 7 years ago

Add releng build files containing Leanplum SDK private app tokens

Categories

(Firefox Build System :: Android Studio and Gradle Integration, enhancement, P1)

Product:
Firefox Build System
Component:
Android Studio and Gradle Integration
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: nalexander, Assigned: nalexander)

References

Details

(Whiteboard: [LP_M1])

Attachments

(4 files, 2 obsolete files)

Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.
59 bytes, text/x-review-board-request
chmanchester
: review+
Details
Bug 1365089 - Follow-up bits after updating the secrets
59 bytes, text/x-review-board-request
Details
Bug 1365089 - Pre: Support setting default keyfile locations in moz.configure.
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1365089 - Follow-up: Don't require Leanplum SDK secrets for repacks.
59 bytes, text/x-review-board-request
nthomas
: review+
Details 
This is the Leanplum SDK equivalent to https://bugzilla.mozilla.org/show_bug.cgi?id=1152871.
nevin: we need three Leanplum SDK tokens:

* one for release;
* one for beta;
* one for developers.

The last one we will check into the tree when we land the code, like mobile/android/base/adjust-sdk-sandbox.token but the first two is private.  Just get them ready and we'll transmit them securely to releng (via encrypted email, IIRC).
Flags: needinfo?(cnevinchen)
coop: can you co-ordinate this, like you did for https://bugzilla.mozilla.org/show_bug.cgi?id=1152871?
Flags: needinfo?(coop)
Depends on: 1365060
Comment on attachment 8867923 [details]
Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.

https://reviewboard.mozilla.org/r/139456/#review142806

::: commit-message-8d576:14
(Diff revision 1)
> +I don't know how to set a default or fallback keyfile at this time,
> +but we probably want one, since this is intended to be enabled in
> +Nightly very quickly.  We might turn it on only in official Nightly
> +builds to avoid having local developers need to change their
> +mozconfigs.  Any guidance on setting a fallback keyfile appreciated!

We should be able to use `imply_option` to set `--with-leanplum-sdk-keyfile` based on the milestone.
Attachment #8867923 - Flags: review?(cmanchester) → review+
imply_option doesn't allow use override.
(In reply to Mike Hommey [:glandium] from comment #5)
> imply_option doesn't allow use override.

glandium: can you suggest how to achieve what I want, which is that MOZ_ANDROID_MMA implies a default --with-leanplum-sdk-keyfile=/mobile/android/base/leanplum-sdk.token, but --with-leanplum-sdk-keyfile can be overridden by mozconfigs in automation?
Flags: needinfo?(mh+mozilla)
Change the functions in build/moz.configure/keyfiles.configure to allow to pass a default?
Flags: needinfo?(mh+mozilla)
(In reply to Nick Alexander :nalexander from comment #2)
> coop: can you co-ordinate this, like you did for
> https://bugzilla.mozilla.org/show_bug.cgi?id=1152871?

Kim should be able to have buildduty tackle this.
Flags: needinfo?(coop) → needinfo?(kmoir)
opened bug 1365372 for the buildduty work
Depends on: 1365372
Flags: needinfo?(kmoir)
If you need the app_id and access_key for each channel. Please feel free to let me know.
Flags: needinfo?(cnevinchen)
Comment on attachment 8868657 [details]
Bug 1365089 - Pre: Support setting default keyfile locations in moz.configure.

https://reviewboard.mozilla.org/r/140242/#review144280

Tests should be added to python/mozbuild/mozbuild/test/configure/test_checks_configure.py with this change.
Attachment #8868657 - Flags: review?(cmanchester)
Comment on attachment 8868657 [details]
Bug 1365089 - Pre: Support setting default keyfile locations in moz.configure.

https://reviewboard.mozilla.org/r/140242/#review145248
Attachment #8868657 - Flags: review?(cmanchester) → review+
Priority: -- → P1
Wesley: Nevin: I need you folks to co-ordinate with releng build duty and our marketing team (or Leanplum directly) to get the SDK client ID and token files onto the buildbots before this can land.

The Release key needs to go into

/builds/leanplum-sdk.token

The Beta key needs to go into 

/builds/leanplum-sdk-beta.token

I think Nevin wanted to keep the Nightly key private, so it needs to go into

/builds/leanplum-sdk-nightly.token

Then we want one more key that is public for developers to use, which I will check into the tree before landing the patch.  Let me know if you need me to help with that.
Hi chmanchester
Should I send the API key to you?

Thanks!
Flags: needinfo?(cmanchester)
(In reply to Nick Alexander :nalexander from comment #17)
> Wesley: Nevin: I need you folks to co-ordinate with releng build duty and
> our marketing team (or Leanplum directly) to get the SDK client ID and token
> files onto the buildbots before this can land.
> 
> The Release key needs to go into
> 
> /builds/leanplum-sdk.token
> 
> The Beta key needs to go into 
> 
> /builds/leanplum-sdk-beta.token
> 
> I think Nevin wanted to keep the Nightly key private, so it needs to go into
> 
> /builds/leanplum-sdk-nightly.token
> 
Above are correct. And nightly's access_key/app_id should be the same as beta, Production will have another app_id and access_key

> Then we want one more key that is public for developers to use, which I will
> check into the tree before landing the patch.  Let me know if you need me to
> help with that.

Hi Nick
The develpment app_id and access_key should only be available for Fennec team internally. Can you teach me how to do that?
I don't want the dev key to be put in m-c. Thank you!
Flags: needinfo?(nalexander)
(In reply to Nevin Chen [:nechen] from comment #20)
> (In reply to Nick Alexander :nalexander from comment #17)
> > Wesley: Nevin: I need you folks to co-ordinate with releng build duty and
> > our marketing team (or Leanplum directly) to get the SDK client ID and token
> > files onto the buildbots before this can land.
> > 
> > The Release key needs to go into
> > 
> > /builds/leanplum-sdk.token
> > 
> > The Beta key needs to go into 
> > 
> > /builds/leanplum-sdk-beta.token
> > 
> > I think Nevin wanted to keep the Nightly key private, so it needs to go into
> > 
> > /builds/leanplum-sdk-nightly.token
> > 
> Above are correct. And nightly's access_key/app_id should be the same as
> beta, Production will have another app_id and access_key

We've learned the hard way that every channel should have its own key: the code shipping is different so the configuration needs to be different too.

> > Then we want one more key that is public for developers to use, which I will
> > check into the tree before landing the patch.  Let me know if you need me to
> > help with that.
> 
> Hi Nick
> The develpment app_id and access_key should only be available for Fennec
> team internally. Can you teach me how to do that?
> I don't want the dev key to be put in m-c. Thank you!

I thought you wanted Leanplum to be enabled for all developers, rather than opt-in for developers (like Adjust is).  Why have it enabled but with a bogus key?  Let's just do the same as we did for Adjust and have it off for developers and opt-in when Fennec team folks who have the private development key want it.
Flags: needinfo?(nalexander)
(In reply to Nevin Chen [:nechen] from comment #19)
> Hi chmanchester
> Should I send the API key to you?
> 
> Thanks!

I'm sorry, I'm not the right person for this. Maybe Nick can redirect.
Flags: needinfo?(cmanchester) → needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #21)
> We've learned the hard way that every channel should have its own key: the
> code shipping is different so the configuration needs to be different too.
Got it. Thanks!

> I thought you wanted Leanplum to be enabled for all developers, rather than
> opt-in for developers (like Adjust is).  Why have it enabled but with a
> bogus key?  Let's just do the same as we did for Adjust and have it off for
> developers and opt-in when Fennec team folks who have the private
> development key want it.

Sorry for the confussion. Because Leanplum will charge us on how many users register. And limit of the number of events is 600. So the access key/app_id should be restricted. Thank you for the understand. 

Btw, does this means your patch about "mobile/android/config/mozconfigs/common" also need to be modified?

I guess on after I resolve my problems in https://bugzilla.mozilla.org/show_bug.cgi?id=1365372#c13 , I should create three screets like you mentioned comment 17?

Thank you!
Comment on attachment 8871425 [details]
Bug 1365089 - Follow-up bits after updating the secrets

chmanchester: this is some small follow-up to the patches you've already reviewed; I will review the follow-ups.  Locking in the SDK token names, etc.
Flags: needinfo?(nalexander)
Attachment #8871425 - Flags: review?(cmanchester) → review?(nalexander)
nechen: I tried to get to this today but found a problem which I can't address before my EOD.

We made MOZ_ANDROID_MMA depend on MOZ_INSTALL_TRACKING to avoid having to add some library handling code in Makefile.in, however we want:
MMA on in MOZILLA_OFFICIAL builds for Nightly, Beta, and Release
but:
INSTALL_TRACKING is only on for Beta and Release

We don't want to turn INSTALL_TRACKING on for Nightly, so we have to do more work to not require INSTALL_TRACKING.  It's not that hard and I should get to it tomorrow.  (You can see the comments I put in Makefile.in if you want to make the changes; you'll need to remove duplicates from JAVA_CLASSPATH and other places in the Makefile.in to handle the case where both flags are enabled.)
Comment on attachment 8871425 [details]
Bug 1365089 - Follow-up bits after updating the secrets

Sorry I forget to check your patch again before I submit mine.
What I want to do here is 
1. Change /builds/leanplum-sdk.token -> /builds/leanplum-sdk-release.token(Bug 1365372)
2. Add leanplum-sdk-nightly.token and leaplum-sdk-sandbox.token.

Please feel free to remove my patch if it doesn't make sense. 

Thanks again for your help.
Attachment #8868657 - Attachment is obsolete: true
Attachment #8871516 - Attachment is obsolete: true
chmanchester: sorry for the chatter on this ticket.  The defaults have changed enough that I've prepared a new version and think I should get fresh review.  It turns out that we don't want to put a default file in the tree (so we don't use the default code and tests I added) and it's not easy to inspect MOZILLA_OFFICIAL in moz.configure so we don't set the default in moz.configure any more.  (I think the default code and tests for the keyfiles is still useful so intend to land it.)

Could you vet the logic I document in the commit message?  Try builds are running now.  Thanks!
Comment on attachment 8867923 [details]
Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.

https://reviewboard.mozilla.org/r/139456/#review147190

::: mobile/android/moz.configure:50
(Diff revision 6)
> +add_old_configure_assignment('MOZ_ANDROID_MMA',
> +                             depends_if('MOZ_ANDROID_MMA')(lambda _: True))
> +

Old configure doesn't seem to need this anymore.
Comment on attachment 8867923 [details]
Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.

https://reviewboard.mozilla.org/r/139456/#review147192
Comment on attachment 8871425 [details]
Bug 1365089 - Follow-up bits after updating the secrets

I am about to land my version of this.
Attachment #8871425 - Flags: review?(nalexander)
Comment on attachment 8867923 [details]
Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.

https://reviewboard.mozilla.org/r/139456/#review147204

::: mobile/android/moz.configure:50
(Diff revision 6)
> +add_old_configure_assignment('MOZ_ANDROID_MMA',
> +                             depends_if('MOZ_ANDROID_MMA')(lambda _: True))
> +

I've removed this stanza.  (The others are necessary, since they add library checks in `android.m4`.)
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9c7688e54e92
Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/9c7688e54e92
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Backed this out for breaking Android L10n nightlies:

https://hg.mozilla.org/mozilla-central/rev/1815768e6a1f7d9027c3c8400e324fc6dde70879

Push showing failures (previous merge had other bustage): https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=35099b4caec14bf0e3c5e3fed7a17dd3faf51dbe&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=102597476&repo=mozilla-central
[task 2017-05-27T16:47:58.173109Z] 16:47:58     INFO -  checking for the Leanplum SDK key... no
[task 2017-05-27T16:47:58.173507Z] 16:47:58     INFO -  ERROR: '/builds/leanplum-sdk-nightly.token': No such file or directory.
Status: RESOLVED → REOPENED
Flags: needinfo?(nalexander)
Resolution: FIXED → ---
Hi Andrei
Could you please chekck the nightly token for us?
Thanks!
Flags: needinfo?(aobreja)
The l10n builds don't pull in the secrets like the main nightly does. I suggest you try extending this block of code 

# Disable Keyfile Loading (and checks) since l10n doesn't need these keys
# This overrides the settings in the common android mozconfig
ac_add_options --without-mozilla-api-keyfile
ac_add_options --without-adjust-sdk-keyfile

at https://dxr.mozilla.org/mozilla-central/rev/701e0ebc2b4b7ae57248e44fd06278e5309e1a05/mobile/android/config/mozconfigs/android-api-15/l10n-nightly#27

for the leanplum case. Then configure shouldn't complain about the missing secret, which doesn't really need because we're going to be repacking a build where it was present.
Hi Nick. Could you please land it again for me?

Thanks!
Comment on attachment 8867923 [details]
Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure.

https://reviewboard.mozilla.org/r/139456/#review147470

::: mobile/android/config/mozconfigs/common:74
(Diff revision 7)
> +# keyfile set, so if we misconfigure beta or release, the builds will fail (at
> +# configure time).
> +if test "$MOZ_UPDATE_CHANNEL" = "release" ; then
> +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-release.token
> +elif test "$MOZ_UPDATE_CHANNEL" = "beta" ; then
> +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-beta.token

Should we add export MOZ_ANDROID_MMA=1 to all channels?

::: mobile/android/config/mozconfigs/common:79
(Diff revision 7)
> +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-beta.token
> +elif test "$MOZ_UPDATE_CHANNEL" = "nightly" ; then
> +    export MOZ_ANDROID_MMA=1
> +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-nightly.token
> +else
> +    ac_add_options --with-leanplum-sdk-keyfile="$topsrcdir/mobile/android/base/leanplum-sdk-sandbox.token"

Should we add more options for the fix in https://bugzilla.mozilla.org/show_bug.cgi?id=1365089#c48 ?
Flags: needinfo?(aobreja)
(In reply to Nevin Chen [:nechen] from comment #50)
> Comment on attachment 8867923 [details]
> Bug 1365089 - Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to
> configure.
> 
> https://reviewboard.mozilla.org/r/139456/#review147470
> 
> ::: mobile/android/config/mozconfigs/common:74
> (Diff revision 7)
> > +# keyfile set, so if we misconfigure beta or release, the builds will fail (at
> > +# configure time).
> > +if test "$MOZ_UPDATE_CHANNEL" = "release" ; then
> > +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-release.token
> > +elif test "$MOZ_UPDATE_CHANNEL" = "beta" ; then
> > +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-beta.token
> 
> Should we add export MOZ_ANDROID_MMA=1 to all channels?
> 
> ::: mobile/android/config/mozconfigs/common:79
> (Diff revision 7)
> > +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-beta.token
> > +elif test "$MOZ_UPDATE_CHANNEL" = "nightly" ; then
> > +    export MOZ_ANDROID_MMA=1
> > +    ac_add_options --with-leanplum-sdk-keyfile=/builds/leanplum-sdk-nightly.token
> > +else
> > +    ac_add_options --with-leanplum-sdk-keyfile="$topsrcdir/mobile/android/base/leanplum-sdk-sandbox.token"
> 
> Should we add more options for the fix in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1365089#c48 ?

Sigh, just figuring out which builds are Nightly builds is bananas.  I've pushed a follow-up commit to try to enable Leanplum only for _real_ Nightly builds, and to work around the l10n builds being special snowflakes.  Try pushes, including Nightly + l10n (if it works; in the recent past, these were broken) are out.
Flags: needinfo?(nalexander)
Comment on attachment 8872398 [details]
Bug 1365089 - Follow-up: Don't require Leanplum SDK secrets for repacks.

https://reviewboard.mozilla.org/r/143908/#review147678
Attachment #8872398 - Flags: review?(nthomas) → review+
Attachment #8872397 - Flags: review?(cmanchester) → review?(nalexander)
Comment on attachment 8872397 [details]
Bug 1365089 - Pre: Support setting default keyfile locations in moz.configure.

https://reviewboard.mozilla.org/r/143906/#review147680

This is a carry-forward r+.  Why MozReview figured out that the second (more complicated!) part carries forward, but this part doesn't... that I can't say.
Attachment #8872397 - Flags: review?(nalexander) → review+
Pushed by nalexander@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c6887edb612c
Pre: Support setting default keyfile locations in moz.configure. r=nalexander
https://hg.mozilla.org/integration/autoland/rev/48aff114c781
Add MOZ_ANDROID_MMA and --with-leanplum-sdk-keyfile to configure. r=chmanchester
https://hg.mozilla.org/integration/autoland/rev/cc3616fd254b
Follow-up: Don't require Leanplum SDK secrets for repacks. r=nthomas
I see an android installer size regression from this change:
== Change summary for alert #6934 (as of May 29 2017 21:46 UTC) ==

Regressions:

  1%  installer size summary android-api-15-gradle opt      36,273,026.58 -> 36,725,398.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6934

this is >400K, it might be verifying this was intended.
Flags: needinfo?(nalexander)
(In reply to Joel Maher ( :jmaher) from comment #60)
> I see an android installer size regression from this change:
> == Change summary for alert #6934 (as of May 29 2017 21:46 UTC) ==
> 
> Regressions:
> 
>   1%  installer size summary android-api-15-gradle opt      36,273,026.58 ->
> 36,725,398.17
> 
> For up to date results, see:
> https://treeherder.mozilla.org/perf.html#/alerts?id=6934
> 
> this is >400K, it might be verifying this was intended.

jmaher -- thanks for flagging this, we appreciate the effort that it takes to find the change and the correct person to alert.

Absolutely intended, although it's a pity it's so much code!
Flags: needinfo?(nalexander)
Whiteboard: [LP_M2]
Whiteboard: [LP_M2] → [LP_M1]
Assignee: nobody → nalexander
Product: Firefox for Android → Firefox Build System
Target Milestone: Firefox 55 → mozilla55
You need to log in before you can comment on or make changes to this bug.