-
Notifications
You must be signed in to change notification settings - Fork 1.3k
remotes.http: Support dvc push for http remotes #3343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bf7f0ad
3e73a59
c55fb3d
b45b9f7
5a92a8b
fa69951
c193be3
b893874
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,6 +30,7 @@ | |
| GCP, | ||
| GDrive, | ||
| HDFS, | ||
| HTTP, | ||
| Local, | ||
| S3, | ||
| SSHMocked, | ||
|
|
@@ -290,6 +291,20 @@ def _get_cloud_class(self): | |
| return RemoteHDFS | ||
|
|
||
|
|
||
| @pytest.mark.usefixtures("http_server") | ||
| class TestRemoteHTTP(HTTP, TestDataCloudBase): | ||
skshetry marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| @pytest.fixture(autouse=True) | ||
| def setup_method_fixture(self, request, http_server): | ||
| self.http_server = http_server | ||
| self.method_name = request.function.__name__ | ||
|
|
||
| def get_url(self): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think DS has a valid point here, the original method is static, shouldn't we make all of them non-static now, that it is required by this particular change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For what it's worth, I wrote it like this because it was done the same way in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, it was only a question, its not severe, because any potential problems should be detected on the build stage. For me, it can stay as is. |
||
| return super().get_url(self.http_server.server_port) | ||
|
|
||
| def _get_cloud_class(self): | ||
| return RemoteHTTP | ||
|
|
||
|
|
||
| class TestDataCloudCLIBase(TestDvc): | ||
| def main(self, args): | ||
| ret = main(args) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmrowla, I'm unable to make it work with Digest auth (I'm using the script that you provided on the gist).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But, when I change this to:
it works. Something's wrong in our cached session perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I figured it out. When I try to push, we send a
HEADrequest, after which the script sets a cookie. And, then allPOSTrequests fails. So, I first tried clearing cookies withself._session.cookies.clear()and it worked.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, we need to set auth when on creating sessions?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if this is a side effect of how my test app in the gist was configured?
By default, flask uses client-side cookies for session data, so flask-httpauth does as well. But for a web server running digest auth to be properly secured, it needs to be handled server-side: https://flask-httpauth.readthedocs.io/en/latest/#security-concerns-with-digest-authentication
If I modify the test app to actually use server-side sessions (https://gist.github.com/pmrowla/0615f162d1308cab4f429b6efafe276a) the existing remote code works without needing to clear any cached cookies
If I set
session.authin the http remote at the time we first create the session instead of setting it per request, I still see the same issue making requests against the original test app. So I'm not sure if it's just thatrequests.auth.HTTPDigestAuthwon't work properly when talking to improperly configured flask apps?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pmrowla, I tried that too but, does not work. It's unclear what's the best thing to do here.
If it's probably only
flask-httpauth, let's ignore for now then.