Skip to content
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

Elaborate on Service Discovery troubleshooting #11113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

martin-rueegg
Copy link
Contributor

  • provide subsections for the different services and configurations
  • provide command line curl examples to debug the redirects
  • provide example config for apache .htaccess to redirect any well known service
  • consistently use NGINX in headers, and nginx in text
  • fix some spelling/grammar

☑️ Resolves

🖼️ Screenshots

  • /issues/general_troubleshooting.html#service-discovery-label
    image
  • /installation/nginx.html#troubleshooting
    image

@martin-rueegg
Copy link
Contributor Author

Happy to backport, once accepted.

@kesselb
Copy link
Contributor

kesselb commented Sep 14, 2023

Thank you 👍

@kesselb
Copy link
Contributor

kesselb commented Sep 14, 2023

/backport to stable27

@kesselb
Copy link
Contributor

kesselb commented Sep 14, 2023

/backport to stable26

@martin-rueegg
Copy link
Contributor Author

@kesselb Thanks for looking into this and approving.

I'm not sure I understand the last two comments correctly: does the bot do the backports or shall I provide them?

@joshtrichards
Copy link
Member

@martin-rueegg Bot does them. :) Or tries to. Succeeds more often than not as long as it's not a high traffic change area in the docs.

@martin-rueegg
Copy link
Contributor Author

@martin-rueegg Bot does them. :) Or tries to. Succeeds more often than not as long as it's not a high traffic change area in the docs.

@kesselb Thanks for the clarification.

Will you accept PR's to backport it to older versions too?

It seems an issue that is confusing quite some people over the years and I think it would be great to have the docs updated on this issue. This will cause less tickets and forum questions.


::

<IfModule mod_rewrite.c>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This configuration is done by our .htaccess file.
We should not duplicate it here without a good reason.

Copy link
Contributor Author

@martin-rueegg martin-rueegg Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I mentioned # the standard redirection rules for Nextcloud.

How about my suggested alternative:

      RewriteEngine on
      # The standard redirection rules for Nextcloud; they should already in your .htaccess file and
      # are here just for reference:
      #
      # RewriteRule ^/\.well-known/carddav /remote.php/dav [R=301,L]
      # RewriteRule ^/\.well-known/caldav /remote.php/dav [R=301,L]
      # RewriteRule ^/\.well-known/webfinger /index.php/.well-known/webfinger [R=301,L]
      # RewriteRule ^/\.well-known/nodeinfo /index.php/.well-known/nodeinfo [R=301,L]
      ...

Also note, that I've left the two redirects for webfinger and nodeinfo as they are configured with a 301 permanent redirect, while the general rule below uses a 302 redirect for the reason indicated in the example's comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan. At worst, it's outdated in a few weeks.
Please remove the actual rewrite rules.
Feel free to add a link to our htaccess file.

RewriteCond "%{REQUEST_URI}" "!^/.well-known/pki-validation/" [OR]
# add additional excludes, as required by your server setup
# Use a temporary redirect here (302) in case an future service is handled outside of Nextcloud
RewriteRule ^/\.well-known/ /index.php/ [R=302,L]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think R=302 is wrong. Apache2 should rewrite the request internally, and Nextcloud may respond with a redirect. But Apache2 should not respond with a 302 redirect to index.php.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then the same applies to the rules above which use R=301 and are shipped with nextcloud ...

If that should be changed into an internal redirect, this should be subject to a different PR. Here, for the sake of consistency, R=302 should be fine.

Copy link
Contributor

@kesselb kesselb Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CalDAV/CardDAV redirects are fine as /remote.php/dav is the final destination and the current behavior is as described in https://www.rfc-editor.org/rfc/rfc6764#section-5

Other requests should be handled by our WellKnownController. For the sake of consistency, the examples in our documentation should match our default implementation: https://github.com/nextcloud/server/blob/3b6a9cd23628464cd24da3d426764a8232124f33/.htaccess#L92 (rewrite, not a 302 redirect).

Comment on lines +176 to +177
:ref:`webfinger and hostinfo <service-discovery-webfinger-and-nodeinfo-label>` are not correctly resolved,
although in fact they may well be. Please use the above mentioned **Command line analysis** and
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:ref:`webfinger and hostinfo <service-discovery-webfinger-and-nodeinfo-label>` are not correctly resolved,
although in fact they may well be. Please use the above mentioned **Command line analysis** and
:ref:`webfinger and hostinfo <service-discovery-webfinger-and-nodeinfo-label>` are not correctly resolved. Please use the above mentioned **Command line analysis** and

You cannot use a custom error handler with Nextcloud.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your suggestion.

The whole point of the test with curl command line is to find out the real cause of the problem. In my case, I've spent hours in checking redirection, as the warning message suggests there is something wrong with the redirection. Where in fact, the error resulted because a custom error page was configured in the web server for 404 status code. This custom error page removed the required header from the response, which is the actual reason why the test in admin settings overview failed. I.e. it was not the redirection.

So with the sentence above, I try to hint to the possibility that the redirection might be working, yet due to the removal of the header, the test fails. The curl command line analyses does actually test for that header.

As such, it would be counterproductive to remove the part "although in fact they may well be", as it is not enough to just resolve the endpoints correctly.

@martin-rueegg martin-rueegg force-pushed the enh/improve-troubleshooting-service-discovery branch 4 times, most recently from 42923d8 to 1f1bf58 Compare September 15, 2023 10:51
- provide subsections for the different services and configurations
- provide command line `curl` examples to debug the redirects
- provide exmaple config for apache .htaccess to redirect any well known service
- consistently use `NGINX` in headers, and `nginx` in text
- fix some spelling/grammar

Signed-off-by: Martin Rüegg <[email protected]>
Signed-off-by: Martin Rüegg <[email protected]>
@martin-rueegg martin-rueegg force-pushed the enh/improve-troubleshooting-service-discovery branch from 1f1bf58 to c783cad Compare September 15, 2023 10:54
@martin-rueegg
Copy link
Contributor Author

martin-rueegg commented Sep 15, 2023

Why is this DCO check so picky?! All commits are perfectly signed with my signature. It even shows verified in the entries above...

Copy link

@rlew-is rlew-is left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed a couple of typos

@martin-rueegg
Copy link
Contributor Author

@rlew-is

fixed a couple of typos

Thank you. Appreciated and implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants