Closed Bug 1075896 Opened 10 years ago Closed 9 years ago

Do not request for http credentials for developer mode unless we actually need it

Categories

(Release Engineering :: Applications: MozharnessCore, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: armenzg, Assigned: anuragchaudhury, Mentored)

Details

(Whiteboard: [good first bug][easier-mozharness])

Attachments

(2 files)

Hello contributor!

To get started:
In order to help in here you can start by checking out Mozharness:
 hg clone http://hg.mozilla.org/build/mozharness
 cd mozharness
See this wiki for more information:
https://wiki.mozilla.org/Auto-tools/Projects/Mozharness#Setup
At that point, you should aim to try running a desktop job [1] and not to be asked for http authentication.
FYI: you can just press enter twice for http credentials since they're not needed.

= Background =
A bit ago, we added a developer mode for mozharness, this allows developers to run tests in a similar way to what is done in the Release Engineering infrastructure (tbpl.mozilla.org).

One of these features, allows for developers to download files from locations which require LDAP authentication.
Unfortunately, the developer mode always prompts for http authentication even if you won't need it.

I know that we can determine in advance if we need authentication or not, hence, I would like to remove prompting if not needed.
Below, we can see the output of two jobs running in developer mode [1][2].
In the first example, we won't need http authentication while we do for the second one.

= How to fix it =
Developer mode kicks in when a user appends --cfg developer_config.py to a mozharness script.
The code triggered comes from here [3].
In that function we call _replace_url(), a URL will need replacing if we need to hit a public facing host which requires http authentication; this makes it suitable to add a flag when a replacement has happened. We could then check for that flag to determine if we need to request for credentials.
You can see the original code for developer mode in here:
http://hg.mozilla.org/build/mozharness/diff/bfb8521ff3c6/mozharness/mozilla/testing/testbase.py

Another piece of code that might be useful is to add a try/except block in case we get an http return code of 401.
You can see such code in here (note, this is not checked-in anymore):
http://hg.mozilla.org/build/mozharness/rev/500783a50ea4#l2.69
The code could look like this:
        if "developer_config.py" in self.config["config_files"]:
            try:
                return urllib2.urlopen(url, **kwargs)
            except urllib2.HTTPError, e:
                if e.code == 401:
                    return _urlopen_basic_auth(url, **kwargs)


[1]
armenzg-thinkpad mozharness hg:[default!] $ python scripts/desktop_unittest.py --cfg unittests/linux_unittest.py --reftest reftest --installer-url ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-35.0a1.en-US.linux-x86_64.tar.bz2 --test-url ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-35.0a1.en-US.linux-x86_64.tests.zip --cfg developer_config.py
14:56:46     INFO - MultiFileLogger online at 20141001 14:56:46 in /home/armenzg/repos/mozharness
14:56:46  WARNING - When you use developer_config.py, we drop 'read-buildbot-config' from the list of actions.
14:56:46     INFO - NOTICE: Files downloaded from outside of Release Engineering network require LDAP credentials.
Please enter your full LDAP email address:

[2]
armenzg-thinkpad mozharness hg:[default!] $ python scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite reftest --this-chunk 1 --total-chunks 20 \
> --installer-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz \
> --test-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip \
> --cfg developer_config.py
14:57:24     INFO - MultiFileLogger online at 20141001 14:57:24 in /home/armenzg/repos/mozharness
14:57:24  WARNING - When you use developer_config.py, we drop 'read-buildbot-config' from the list of actions.
14:57:24     INFO - Replacing url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz -> https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/emulator.tar.gz
14:57:24     INFO - Replacing url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip -> https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/tinderbox-builds/mozilla-central-emulator/20141001060621/b2g-35.0a1.en-US.android-arm.tests.zip
14:57:24     INFO - Replacing url http://runtime-binaries.pvt.build.mozilla.org/tooltool/sha512/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4 -> https://secure.pub.build.mozilla.org/tooltool/pvt/build/sha512/263f4e8796c25543f64ba36e53d5c4ab8ed4d4e919226037ac0988761d34791b038ce96a8ae434f0153f9c2061204086decdbff18bdced42f3849156ae4dc9a4
14:57:24     INFO - Replacing url http://runtime-binaries.pvt.build.mozilla.org/tooltool/sha512/0748e900821820f1a42e2f1f3fa4d9002ef257c351b9e6b78e7de0ddd0202eace351f440372fbb1ae0b7e69e8361b036f6bd3362df99e67fc585082a311fc0df -> https://secure.pub.build.mozilla.org/tooltool/pvt/build/sha512/0748e900821820f1a42e2f1f3fa4d9002ef257c351b9e6b78e7de0ddd0202eace351f440372fbb1ae0b7e69e8361b036f6bd3362df99e67fc585082a311fc0df
14:57:24     INFO - NOTICE: Files downloaded from outside of Release Engineering network require LDAP credentials.
Please enter your full LDAP email address:

[3]
http://hg.mozilla.org/build/mozharness/file/default/mozharness/mozilla/testing/testbase.py#l174
I created this authenticated site to test the changes against (user: test; password: test):
http://people.mozilla.org/~armenzg/permanent/private_builds

Let me know if I can put smaller size builds in there for you to test.
hey, I am interested in working on this. Can you please assign it to me ?
Cheers!
Hi Ingrid, happy to hear from you and that you're interested on contributing.

This is one of the bugs that gets the most on the way of my development (as well as other developers) so I'm happy to get some help in this area.

Let me know if you have questions by making comments in here or come and chat in the #ateam IRC channel.
I'm generally available from 9 to 5 ET, however, there are plenty of people in the #ateam or #releng channels that have mozharness and python knowledge.

Good luck!
Assignee: nobody → ingrid.epure
Ingrid, are you doing? Did the requirements make sense?
Flags: needinfo?(ingrid.epure)
Ingrid, let me know if you're still interested on working on this.
For now I will make the bug available for others to grab.
Assignee: ingrid.epure → nobody
Flags: needinfo?(ingrid.epure)
Hi Armen,

I started working on this. I have a couple of questions - 

1. Is this the right way to use the url you provided (the test one) -

 python scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite reftest  --installer-url http://people.mozilla.org/~armenzg/permanent/private_builds/emulator.tar.gz  --test-url http://people.mozilla.org/~armenzg/permanent/private_builds/b2g-35.0a1.en-US.android-arm.tests.zip  --cfg developer_config.py

?

The thing is even after I enter the user and pwd for the LDAP as 'test' it fails to connect. In fact, I'm not sure at what point it even uses the credentials. It looked like 

b2g_emulator_unittest's download_and_extract is called which calls the method in testbase.py which in turn eventually calls download_proxied_file in proxxy.py which calls the download_file in script.py. At no point does it seem that the credentials being entered are being used. Could you please point out where that would be happening (using the credentials)?

2.In testbase.py there is the method __urlopen which seems like it could be using the credentials but this method doesn't seem to be ever invoked. Where is this needed/called?

The changes I made worked for [1] and [2] that you provided in the sense that when I ran it [1] did not prompt for credentials whereas [2] did. But I'm not sure what's happening with the test url, as even without my changes I see the things I mentioned above. So I wanted to clarify before submitting a patch.

Thanks!
I will set the needinfo flag on myself so I don't miss getting back to this.

I'm at a conference and meetings all week so it will be hard for me to get back to you in a promptly manner.
If you pushed making contributions to next week it will be much easier for me to assist you.

Many thanks again!

(In reply to Anurag Chaudhury from comment #6)
> Hi Armen,
> 
> I started working on this. I have a couple of questions - 
> 
> 1. Is this the right way to use the url you provided (the test one) -
> 
>  python scripts/b2g_emulator_unittest.py --cfg
> b2g/emulator_automation_config.py --test-suite reftest  --installer-url
> http://people.mozilla.org/~armenzg/permanent/private_builds/emulator.tar.gz 
> --test-url
> http://people.mozilla.org/~armenzg/permanent/private_builds/b2g-35.0a1.en-US.
> android-arm.tests.zip  --cfg developer_config.py
> 
> ?
> 
> The thing is even after I enter the user and pwd for the LDAP as 'test' it
> fails to connect. In fact, I'm not sure at what point it even uses the
> credentials. It looked like 
> 
> b2g_emulator_unittest's download_and_extract is called which calls the
> method in testbase.py which in turn eventually calls download_proxied_file
> in proxxy.py which calls the download_file in script.py. At no point does it
> seem that the credentials being entered are being used. Could you please
> point out where that would be happening (using the credentials)?
> 

You're very right! That is an issue which I have tried to tackle in bug 1087664.

Let me get back to you and use desktop_unittest.py instead.

> 2.In testbase.py there is the method __urlopen which seems like it could be
> using the credentials but this method doesn't seem to be ever invoked. Where
> is this needed/called?
> 

desktop_unittest.py makes use of this. I haven't looked at the b2g code (even though is the one that will eventually need it).

> The changes I made worked for [1] and [2] that you provided in the sense
> that when I ran it [1] did not prompt for credentials whereas [2] did. But
> I'm not sure what's happening with the test url, as even without my changes
> I see the things I mentioned above. So I wanted to clarify before submitting
> a patch.
> 
> Thanks!
Flags: needinfo?(armenzg)
Hi Armen,

No problem! Thanks for letting me know. I'll play around with it a bit in the meantime.
Attached file patch.py
Comment on attachment 8532833 [details]
patch.py

Added the lines for :

orig_config = copy.deepcopy(c) and the 

if not c == orig_config:
            self._get_credentials()

at the end.
Attachment #8532833 - Flags: review?(armenzg)
Armen, I submitted the changed function as a patch. Let me know what updates I should make and I'll work on it further. With the desktop_unittest it doesnt ask for credentials but for the other one it does.Thanks.
I will be looking at this today. My apologies for the delay. It was difficult last week with all the flying and work week.
(In reply to Anurag Chaudhury from comment #6)
> Hi Armen,
> 
> I started working on this. I have a couple of questions - 
> 
> 1. Is this the right way to use the url you provided (the test one) -
> 
>  python scripts/b2g_emulator_unittest.py --cfg
> b2g/emulator_automation_config.py --test-suite reftest  --installer-url
> http://people.mozilla.org/~armenzg/permanent/private_builds/emulator.tar.gz 
> --test-url
> http://people.mozilla.org/~armenzg/permanent/private_builds/b2g-35.0a1.en-US.
> android-arm.tests.zip  --cfg developer_config.py
> 
> ?
> 
> The thing is even after I enter the user and pwd for the LDAP as 'test' it
> fails to connect. In fact, I'm not sure at what point it even uses the
> credentials. It looked like 
> 
> b2g_emulator_unittest's download_and_extract is called which calls the
> method in testbase.py which in turn eventually calls download_proxied_file
> in proxxy.py which calls the download_file in script.py. At no point does it
> seem that the credentials being entered are being used. Could you please
> point out where that would be happening (using the credentials)?
> 

To fix the download issues, apply the code changes mentioned in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1087664#c5

That bug needs fixing since the proxied files code path does not use http authentication.
Use the workaround for now.

> 2.In testbase.py there is the method __urlopen which seems like it could be
> using the credentials but this method doesn't seem to be ever invoked. Where
> is this needed/called?
> 

Look at the code in base/script.py and pay attention to the inheritance.
Let me know if you have questions after you look at it.

armenzg@armenzg-thinkpad:~/repos/mozharness$ grep -r "_urlopen" mozharness/ scripts/ configs/
Binary file mozharness/mozilla/auth_download.pyc matches
Binary file mozharness/mozilla/testing/testbase.pyc matches
mozharness/mozilla/testing/testbase.py:    def _urlopen(self, url, **kwargs):
mozharness/mozilla/testing/testbase.py:        def _urlopen_basic_auth(url, **kwargs):
mozharness/mozilla/testing/testbase.py:            return _urlopen_basic_auth(url, **kwargs)
Binary file mozharness/base/script.pyc matches
mozharness/base/script.py:    def _urlopen(self, url, **kwargs):
mozharness/base/script.py:            f = self._urlopen(url, timeout=30)


> The changes I made worked for [1] and [2] that you provided in the sense
> that when I ran it [1] did not prompt for credentials whereas [2] did. But
> I'm not sure what's happening with the test url, as even without my changes
> I see the things I mentioned above. So I wanted to clarify before submitting
> a patch.
> 
> Thanks!
Flags: needinfo?(armenzg)
Comment on attachment 8532833 [details]
patch.py

Hello Anurag,
r- means that the code needs some more work before being able to make it final.

For the next patch, could you please do the following?
* clone mozharness
* modify mozharness/mozilla/testing/testbase.py
* hg diff > my_patch.diff
* attach my_patch.diff

If you don't have working code, instead of using the "review" flag use the "feedback" flag. This is for future patches.

Try to only request credentials if a replacement happens, see this:
> new_url = url.replace(from_, to_)
Attachment #8532833 - Flags: review?(armenzg) → review-
Let me know if you have further questions.
I now should be able to get back to you in a more timely manner.
Assignee: nobody → anuragchaudhury
Attachment #8536039 - Flags: feedback?(armenzg)
Hey Armen,

Just wanted to ask/mention a few things.

1. I attached the diff. Basically you said only request cred if replacement happens, so from what I understood when url.replace(from_, to_) happens,the dictionary c is changed (the value updated to "to"). That's why I make a copy in the beginning and if they are not the same request credentials. Please correct me if I'm wrong.

2. I'm not sure how to use the diff comment you pointed me to. the TestMixin class does not seem to inherit from ScriptMixin, so I'm not sure how changing to download_file would work? This method doesn't exist in this class in any case. Also, I realize that download_proxied_file itself uses the download_file method from ScriptMixin and does not use the http credentials in any case. Really confused about this part!
I mean since all downloads seem to go through proxxy's download_proxy_file, which basically executes scriptmixin's download_file, we can just update that method to use http credentials and should work (I think).


3. Lastly, I'm not sure what I'm looking for in terms of the inheritance in script.py. Its _urlopen method seems distinct from the _urlopen method in testbase.py. As in I cant see how these 2 are related. From what I see in desktop_unittest and b2g_unittest, download_extract_file is called which is in testbase.py. This method does somethings and calls a bunch of other download_ methods which all use download_proxied_file -> proxxy.download_proxied_file -> scriptmixin.download_file -> scriptmixin._urlopen .  Basically, I think i'm paying attention to the inheritance but dont see the link between the 2 _urlopen methods or how the one in testbase.py is being used. :(

I must be missing something I guess but I cant see what. Sorry about all the bother!
Comment on attachment 8536039 [details] [diff] [review]
http_cred_patch.diff

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

I will land this on your behalf. It works as expected. Many thanks!

I tested that this [1] does not require credentials, however, this [2] should and does require credentials.

[1]
python scripts/desktop_unittest.py --cfg unittests/linux_unittest.py --reftest reftest --installer-url http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-37.0a1.en-US.linux-x86_64.tar.bz2 --test-url http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/firefox-37.0a1.en-US.linux-x86_64.tests.zip --cfg developer_config.py

[2]
python scripts/b2g_emulator_unittest.py --cfg b2g/emulator_automation_config.py --test-suite reftest --this-chunk 1 --total-chunks 20 --blob-upload-branch mozilla-central --download-symbols ondemand --cfg developer_config.py --installer-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/nightly/mozilla-central-emulator/latest/emulator.tar.gz --test-url http://pvtbuilds.pvt.build.mozilla.org/pub/mozilla.org/b2g/nightly/mozilla-central-emulator/latest/b2g-37.0a1.en-US.android-arm.tests.zip
Attachment #8536039 - Flags: feedback?(armenzg) → review+
Great!Thanks Armen.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → INVALID
Resolution: INVALID → WORKSFORME
mozharness change merged to production.

Thanks to you Anurag!
Resolution: WORKSFORME → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: