1021021 - add schema for h264 updates

Status: RESOLVED FIXED

(Release Engineering Graveyard :: Applications: Balrog (backend), defect)

Description

[[e-mail] (:bhearsum)]

Not sure exactly what the schema will look like yet, but it will probably be somewhat different than existing ones.

Comment 1

[Brian R. Bondy [:bbondy]]

My implementation for bug 1009816 uses Bug 1009816 Comment 4's format.  Although if you want to make changes that's fine too.  They want my stuff landed for FF33 though so it would be best to finalize this soon.  

Also it's implemented out of the nsUpdateService.js code, so that means it's a different ping to the server. If you want to change the URL it's fine with me.   My component currently ignores <update/> elements and the nsUpdateService one currently ignores <addons> elements. So it's the same if you want to use a different endpoint or not.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Brian R. Bondy [:bbondy] from comment #1)
> My implementation for bug 1009816 uses Bug 1009816 Comment 4's format. 
> Although if you want to make changes that's fine too.  They want my stuff
> landed for FF33 though so it would be best to finalize this soon.  

Yeah, agreed. I think Nick and I probably want to talk about how well that one will work - we haven't looked at it much to date.

> Also it's implemented out of the nsUpdateService.js code, so that means it's
> a different ping to the server.

Is this how we're going to move forward?

> If you want to change the URL it's fine with
> me.

If we're going with a separate ping, the URL will definitely be different. IIRC, our current thinking is that the product will be different (eg, "H264" instead of "Firefox").

> My component currently ignores <update/> elements and the
> nsUpdateService one currently ignores <addons> elements. So it's the same if
> you want to use a different endpoint or not.

Good to know, thanks.

Comment 3

[Brian R. Bondy [:bbondy]]

> Is this how we're going to move forward?

Not my call, but I think it'll be like that for v1. Modulo potential strong objections in a review.

Comment 4

[Benjamin Smedberg]

My only desire is that we leave room in the schema and ping facility for the case where a Firefox update also needs an OpenH264 update. Although it's not a requirement for FF33, in the future we will want to download and stage the new OpenH264 before we apply the Firefox update.

If we can do that with two pings, then let's go with two pings. We don't need a privacy review as long as the OpenH264 ping can be controlled by the addon manager UX (disabling OpenH264 should suppress the ping).

It's not clear to me who is going to decide the final schema proposal. Is that you bhearsum? That really should be finalized today.

Comment 5

[[e-mail] (:bhearsum)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> My only desire is that we leave room in the schema and ping facility for the
> case where a Firefox update also needs an OpenH264 update. Although it's not
> a requirement for FF33, in the future we will want to download and stage the
> new OpenH264 before we apply the Firefox update.

As far as the server side is concerned, going with two pings now is quicker and easier. One ping is also doable, but it requires bug 1021029 and bug 1021032 as well as everything else blocking bug 1013354. If we go with two pings now, there's no reason (as far as the server is concerned) that we can't switch to one ping later. So, if it's not a requirement for 33 I'd recommend sticking with two pings for now based on our tight timeline.

> If we can do that with two pings, then let's go with two pings. We don't
> need a privacy review as long as the OpenH264 ping can be controlled by the
> addon manager UX (disabling OpenH264 should suppress the ping).

I know Nick had some concerns about whether or not it would be doable, I don't remember precisely what they were though. In any case, this question is about client code, not the server - per above, we can support either on the server.

> It's not clear to me who is going to decide the final schema proposal. Is
> that you bhearsum? That really should be finalized today.

I've looked over your strawmen from bug 1009816 again, here's a tweaked version of your first option:
<updates>
  <addons>
    <addon id="[e-mail]" URL="http://download.cdn.cisco.com/openh264/win32/openh264-1.1.zip" hashFunction="SHA512" hashValue="ABCDEF123456" version="1.1" />
  </addons>
</updates>

Basically, I've tweaked things to be more consistent with what we do for Firefox updates. I think it's a bit nicer this way, and probably simplifies a couple of bits of the implementation. As is, the above should work fine as a response in a two ping scenario. If we're doing one ping, the response will also contain an <update> section for the Firefox update info.

Benjamin, Brian - what do you think?

Comment 6

[Brian R. Bondy [:bbondy]]

I anticipated that hash tweak and my current patches will fall back to using hashFunction and hashValue when hash is not provided. I just need to update the URL attribute, but that's fine with me. I'll update tonight to what you proposed. :)

The parent updates/ element doesn't seem needed, but in case we need to merge to one URL it might make sense to leave it that way.

Comment 7

[Brian R. Bondy [:bbondy]]

Oh can we also add an expected file size attribute? It allows us to ensure there's no downloading huge file attack to fill up disk space. We do that for updates because a security team member in the past recommended it.

Comment 8

[Benjamin Smedberg]

wfm, as long as it works for bbondy

Comment 9

[[e-mail] (:bhearsum)]

(In reply to Brian R. Bondy [:bbondy] from comment #7)
> Oh can we also add an expected file size attribute? It allows us to ensure
> there's no downloading huge file attack to fill up disk space. We do that
> for updates because a security team member in the past recommended it.

Yeah, we should do that. So to avoid confusion, here is the revised schema:
<updates>
  <addons>
    <addon id="[e-mail]" URL="http://download.cdn.cisco.com/openh264/win32/openh264-1.1.zip" hashFunction="SHA512" hashValue="ABCDEF123456" size="123" version="1.1" />
  </addons>
</updates>

Comment 10

[[e-mail] (:bhearsum)]

I started looking at implementing this today. One issue I came across very quickly is that even though we've moved XML to the blobs, we still have blob-dependent code in AUS.evaluateRules. We probably need to move this code to the blobs, too.

Comment 11

[[e-mail] (:bhearsum)]

(In reply to Ben Hearsum [:bhearsum] from comment #10)
> I started looking at implementing this today. One issue I came across very
> quickly is that even though we've moved XML to the blobs, we still have
> blob-dependent code in AUS.evaluateRules. We probably need to move this code
> to the blobs, too.

Taking care of this in bug 1021018.

Comment 12

[[e-mail] (:bhearsum)]

Attachment: reorganize blob code

I meant to post these earlier, but Apple stuff got in the way. I've split the work here into two patches. This one is a no-op that re-organizes the blob code to support multiple different "types" (I haven't been able to come up with a better word for "series of related blobs").

The only new code here is the processSpecialForceHosts method, which is a simple refactor to not duplicate that code in every blob type.

Comment 13

[[e-mail] (:bhearsum)]

Attachment: add gmp blob

...and here's a patch which adds the GMP blob. You'll notice that i'm using BASIC style versioning, because switching to non-sequential schema_versions was going to be a huge amount of work. Other than that I think this is simple and straightforward. Landing should be quite easy too - we can create a new, proper blob for the GMP builds, create a rule, and then remove the hardcoded routes + HackyH264Blob.

No rush on these patches - I just wanted to get them up rather than let them rot in my repo.

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8470979 [details] [diff] [review]
reorganize blob code

Review of attachment 8470979 [details] [diff] [review]:
-----------------------------------------------------------------

r+ from a high level check on re-organisation. One comment ...

::: auslib/blobs/apprelease.py
@@ +31,2 @@
>      def getResolvedPlatform(self, platform):
>          return self['platforms'][platform].get('alias', platform)

By leaving this here we won't have aliasing for GMP. Could follow that up in the GMP patch to leave this one a no-op.

Comment 15

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8470982 [details] [diff] [review]
add gmp blob

Review of attachment 8470982 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments about tests addressed.

::: auslib/blobs/base.py
@@ +67,5 @@
>              return ReleaseBlobV2(**data)
>          elif data['schema_version'] == 3:
>              return ReleaseBlobV3(**data)
> +        elif data['schema_version'] == 1000:
> +            return GMPBlobV1(**data)

enh, we could make a dict to map all this, although I'm not sure how we'd handle the imports.

::: auslib/blobs/gmp.py
@@ +43,5 @@
> +                continue
> +
> +            url = platformInfo["fileUrl"]
> +            if updateQuery["force"]:
> +                url = self.processSpecialForceHosts(url, specialForceHosts)

I don't mind if we have this functionality (or not) when the d/l is from an external site, but please add a test if you'd like to keep it.

@@ +48,5 @@
> +            if containsForbiddenDomain(url, whitelistedDomains):
> +                continue
> +            vendorXML.append('        <addon id="%s" URL="%s" hashFunction="%s" hashValue="%s" size="%d" version="%s"/>' % \
> +                (id_, url, self["hashFunction"], platformInfo["hashValue"],
> +                    platformInfo["filesize"], vendorInfo["version"]))

enh, I know the error handler will catch any cases where these (or fileUrl) are missing from the blob, but it makes me yearn for stricter blob checking again.

@@ +58,5 @@
> +            xml.extend(vendorXML)
> +            xml.append('    </addons>')
> +        xml.append('</updates>')
> +        # ensure valid xml by using the right entity for ampersand
> +        return re.sub('&(?!amp;)','&amp;', '\n'.join(xml))

enh, we could add a generic 'fix encoding' function on the Blob class.

::: auslib/test/blobs/test_gmp.py
@@ +10,5 @@
> +    return
> +
> +class TestSchema1Blob(unittest.TestCase):
> +    maxDiff = 2000
> +    

nit, whitespace

@@ +62,5 @@
> +        self.cef_patcher.stop()
> +
> +    def testGMPUpdate(self):
> +        updateQuery = {
> +            "product": "gg", "version": "3", "buildID": "1",

Having 'gg' for the product here implies a connection to the name 'gg' in the blob, but does one really exist ? If not it would be good to change one place it's used.

Comment 16

[[e-mail] (:bhearsum)]

(In reply to Nick Thomas [:nthomas] from comment #14)
> Comment on attachment 8470979 [details] [diff] [review]
> reorganize blob code
> 
> Review of attachment 8470979 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ from a high level check on re-organisation. One comment ...
> 
> ::: auslib/blobs/apprelease.py
> @@ +31,2 @@
> >      def getResolvedPlatform(self, platform):
> >          return self['platforms'][platform].get('alias', platform)
> 
> By leaving this here we won't have aliasing for GMP. Could follow that up in
> the GMP patch to leave this one a no-op.

Good point. I'll fix this in the second patch, as you suggest.

Comment 17

[[e-mail] (:bhearsum)]

Attachment: add helper methods to gmp blob; create a map for blob types; remove specialForceHosts

I think this addreses everything except a couple of the enhancements. I looked at sharing getResolvedPlatform with the apprelease blobs, but their structures are different, so we can't do that ("vendors" vs "platforms" at the top level).

I created a blob mapping object within the createBlob method. It's not as nice as having it at the top of the file, but still worthwhile to avoid the error checking via exception catching IMO.

Comment 18

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8491551 [details] [diff] [review]
add helper methods to gmp blob; create a map for blob types; remove specialForceHosts

Review of attachment 8491551 [details] [diff] [review]:
-----------------------------------------------------------------

Interdiff looks good.

Comment 19

[[e-mail] (:bhearsum)]

Attachment: make gmp aliasing work

Sorry for the follow-up -- I realized while putting together a proper GMP blob that the blob format didn't allow for aliasing, and we'd get an error when submitting. Yet another reason to have stricter blob checking!

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8492293 [details] [diff] [review]
make gmp aliasing work

This looks like a git log.

Comment 21

[[e-mail] (:bhearsum)]

Attachment: fix gmp aliasing

Ack, sorry about that.

Comment 22

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 8493040 [details] [diff] [review]
fix gmp aliasing

Review of attachment 8493040 [details] [diff] [review]:
-----------------------------------------------------------------

::: auslib/test/admin/views/test_releases.py
@@ +465,5 @@
> +    }
> +}
> +"""))
> +
> +                                                        #json.dumps(newReleaseFile.getvalue())))

Debugging leftover ?

Comment 23

[[github robot]]

Commits pushed to master at https://github.com/mozilla/balrog

https://github.com/mozilla/balrog/commit/9f5e5bdf36a5e75d6155749fcd4149e4a16aec08
bug 1021021: add schema for h264 updates - reorganize blob code. r=nthomas

https://github.com/mozilla/balrog/commit/19e628875c36dc03601f7aad8c6c0f9d5a131f8f
bug 1021021: add schema for h264 updates - add gmp blob. r=nthomas

Comment 24

[[e-mail] (:bhearsum)]

Comment on attachment 8493040 [details] [diff] [review]
fix gmp aliasing

(In reply to Nick Thomas [:nthomas] from comment #22)
> Comment on attachment 8493040 [details] [diff] [review]
> fix gmp aliasing
> 
> Review of attachment 8493040 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: auslib/test/admin/views/test_releases.py
> @@ +465,5 @@
> > +    }
> > +}
> > +"""))
> > +
> > +                                                        #json.dumps(newReleaseFile.getvalue())))
> 
> Debugging leftover ?

Oops, yeah.

Comment 25

[[e-mail] (:bhearsum)]

I've pushed all of these patches now, but they're not yet in production. I need to finish preparing the blobs and rules in bug 1013354 before pushing these to prod.

Comment 26

[[e-mail] (:bhearsum)]

Unfortunately, this required two follow-ups:
1) Back out the HackyH264Blob code (https://github.com/mozilla/balrog/commit/5f74ff32881f0a377aefa06e10bc16147c175741). I thought I'd done this as part of one of these patches.
2) Fix xml generation type error. We store filesizes as strings (I'm not sure why, but we do it for Gecko apps too...), and when we tried to put them into xml with %d, we got exceptions. https://github.com/mozilla/balrog/commit/c752a38cd14a559d0b5e6a9d0df494696fec438d fixed it.

But now it's working fine \o/.