fix: fail fast if remote is not reachable#41210
Merged
DeepDiver1975 merged 4 commits intomasterfrom Apr 26, 2024
Merged
Conversation
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
2 times, most recently
from
bb7b5b8 to
7d32e4c
Compare
IljaN
approved these changes
Mar 11, 2024
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
from
7d32e4c to
6a853e0
Compare
phil-davis
reviewed
Mar 12, 2024
Contributor
phil-davis
left a comment
phil-davis
left a comment
There was a problem hiding this comment.
and code-style needs to be fixed
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
from
9f7fae9 to
ac07d8c
Compare
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
2 times, most recently
from
952c78f to
80304f2
Compare
Contributor
|
Various acceptance tests for federated sharing are failing with: Example: https://drone.owncloud.com/owncloud/core/39820/96/19 Maybe the test environment is not so good, or maybe the new code is too enthusiastic about deciding that the remote is not reachable. I will have a look... |
Member
Author
|
@phil-davis any idea yet what is going on here? THX a lot |
phil-davis
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
from
80304f2 to
5ecb099
Compare
phil-davis
reviewed
Apr 17, 2024
Comment on lines
+78
to
+84
| $resp = $externalManager->testRemoteUrl(\OC::$server->getHTTPClientService(), $remote); | ||
| if ($resp === false) { | ||
| \OCP\JSON::error(['data' => ['message' => $l->t('Remote is unreachable')]]); | ||
| exit(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
The failing tests are all scenarios where a user tries to add a public link share.
The "federated" share is actually a public link.
That comes through this code.
Variable $remote at this point has the scheme http:// on the front.
But testRemoteUrl expects a URL without the scheme on the front.
I hacked the code like this:
$x = preg_replace("(^https?://)", "", $remote);
$resp = $externalManager->testRemoteUrl(\OC::$server->getHTTPClientService(), $x);
if ($resp === false) {
\OCP\JSON::error(['data' => ['message' => $l->t("Remote '$x' is unreachable")]]);
exit();
}
Now I run a test scenario:
make test-acceptance-api BEHAT_FEATURE=tests/acceptance/features/apiFederationToRoot1/savePublicShare.feature:22
...
Feature: Save public shares created by oC users
Background: # /home/phil/git/owncloud/core/tests/acceptance/features/apiFederationToRoot1/savePublicShare.feature:4
Given using server "LOCAL" # FeatureContext::usingServer()
And user "Alice" has been created with default attributes and without skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
Scenario: Mount public share and then delete (local server share) # /home/phil/git/owncloud/core/tests/acceptance/features/apiFederationToRoot1/savePublicShare.feature:22
Given user "Brian" has been created with default attributes and without skeleton files # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
And user "Alice" has created folder "/PARENT" # FeatureContext::userHasCreatedFolder()
And user "Alice" has created a public link share with settings # FeatureContext::userHasCreatedAPublicLinkShareWithSettings()
| path | /PARENT |
| permissions | read |
And user "Brian" has added the public share created from server "LOCAL" using the sharing API # FeatureContext::userHasAddedPublicShareCreatedByUser()
Sharing::userHasAddedPublicShareCreatedByUser
Failed to save public share.
'Remote '172.17.0.1:8080' is unreachable'
Failed asserting that 'error' is not equal to 'error'.
But it still complains that '172.17.0.1:8080` is not reachable - that is my test installation which is running fine.
I wonder what is the next piece of this puzzle?
Contributor
|
The failing test scenarios are the ones about Brian adding a public share. The usual case of users adding a federated share that is an ordinary file share on another server, that works. |
Contributor
|
If one of the calls in If I separate out each I can just have it do one And that still gets But I can manually do: So the So I suppose that there is some problem with how |
Member
Author
|
@phil-davis THX a lot for the analysis! Much appreciated! |
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
2 times, most recently
from
8a95a20 to
5410eab
Compare
Member
Author
SSL issue |
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
2 times, most recently
from
f14cf07 to
07bab99
Compare
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
3 times, most recently
from
28360d9 to
7095340
Compare
DeepDiver1975
force-pushed
the
fix/sharing-external-unreachable-hosts
branch
from
7095340 to
4fe718e
Compare
Member
Author
|
@phil-davis mind another review? THX |
11 tasks
phil-davis
reviewed
Apr 23, 2024
Contributor
phil-davis
left a comment
phil-davis
left a comment
There was a problem hiding this comment.
If the remote system is not listening on port 80, but some other port, then the checks do not work - they get directed to the "default" port 80, but need to be directed to the correct port.
See the last commit of PR #41237 - with that extra code the tests pass for me in my dev setup that has 2 servers running on localhost port 8080 and 8180 on my laptop.
Comment on lines
+611
to
+614
| if ($this->testUrl($clientService, $schema . $remote . '/status.php', false, true)) { | ||
| \OC::$server->getLogger()->error("Remote $schema$remote/status.php reachable"); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
Suggested change
| if ($this->testUrl($clientService, $schema . $remote . '/status.php', false, true)) { | |
| \OC::$server->getLogger()->error("Remote $schema$remote/status.php reachable"); | |
| return true; | |
| } | |
| if ($this->testUrl($clientService, $schema . $remote . '/status.php', false, true)) { | |
| return true; | |
| } | |
| \OC::$server->getLogger()->error("Remote $schema$remote/status.php not reachable"); |
If the host:port of the remote is completely down, then testUrl is returning false, but I get nothing in the log about it.
Maybe move the error logging to the unhappy path, and don't log when reachable.
Member
Author
There was a problem hiding this comment.
thx for catching this 🙏
Contributor
|
Note: for my local testing, I found that this scenario was good to run: That scenario uses both local and remote (federated) servers and a public link. |
|
delete-merged-branch
bot
deleted the
fix/sharing-external-unreachable-hosts
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Not responding remote servers can cause bad user experience.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: