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

Issue template addition: Are you using Gitea behind CloudFlare? #14098

Conversation

wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor

As title.
Since more often than not CF appears to serve stale cache and cause troubles (such as here - 14089 and initially also here, although we found out about the CF part in our Discord chat), I'd argue it might be helpful to ask about it in this here issue template, hence this PR.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 21, 2020
@codecov-io
Copy link

Codecov Report

Merging #14098 (3c7fa72) into master (addd424) will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14098   +/-   ##
=======================================
  Coverage   42.34%   42.34%           
=======================================
  Files         726      726           
  Lines       77741    77741           
=======================================
+ Hits        32918    32923    +5     
+ Misses      39421    39416    -5     
  Partials     5402     5402           
Impacted Files Coverage Δ
modules/util/timer.go 42.85% <0.00%> (-42.86%) ⬇️
modules/queue/unique_queue_disk_channel.go 53.84% <0.00%> (-1.54%) ⬇️
modules/log/event.go 58.96% <0.00%> (-0.95%) ⬇️
services/pull/pull.go 42.35% <0.00%> (-0.51%) ⬇️
models/error.go 39.47% <0.00%> (+0.48%) ⬆️
modules/git/repo_commit_nogogit.go 65.00% <0.00%> (+1.66%) ⬆️
services/pull/check.go 51.09% <0.00%> (+2.18%) ⬆️
modules/charset/charset.go 73.03% <0.00%> (+4.49%) ⬆️
modules/indexer/stats/db.go 60.00% <0.00%> (+12.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update addd424...3c7fa72. Read the comment docs.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 21, 2020
@mrsdizzie
Copy link
Member

The issues is putting anything in front of Gitea because now there is another layer involved which can modify content generated by Gitea. This is less often related to using a CDN and is an issue with using a proxy in general. Many of the issues (and problems in discord) are because of a nginx/apache/etc.. Misconfiguration and things like that (CloudFlare proxy settings too).

The linked example isn't related to a stale cache or the CDN feature of CloudFlare but rather something that modifies the content Gitea sends before it reaches a browser. I think a maybe more general <!-- If using a proxy or CDN in front of Gitea, please connect to Gitea directly and confirm the issue still happens without those services -->

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor Author

The issues is putting anything in front of Gitea because now there is another layer involved which can modify content generated by Gitea. This is less often related to using a CDN and is an issue with using a proxy in general. Many of the issues (and problems in discord) are because of a nginx/apache/etc.. Misconfiguration and things like that (CloudFlare proxy settings too).

The linked example isn't related to a stale cache or the CDN feature of CloudFlare but rather something that modifies the content Gitea sends before it reaches a browser. I think a maybe more general <!-- If using a proxy or CDN in front of Gitea, please connect to Gitea directly and confirm the issue still happens without those services -->

the stale cache part doesn't apply here, you're right.
the first example constitutes the causes troubles part and while you're right the problem is more of a general reverse proxy problem, IMO quite often the issue is directly tied specifically to CF, as it's what many people use apparently, or maybe I just see a CF issue and go kaboom :D.
It's better, to use broader terms in this case, though, I'll update the comment.

since more often than not CF appears to serve stale cache and cause
troubles, I'd argue it might be helpful to ask about it in this here
issue template
* implement @mrsdizzie's suggestion
* as the comment grows, rather span multiple lines
* Gitea --> gitea to match case used in the rest of the template
@zeripath
Copy link
Contributor

I think it might be helpful to add something to the FAQ here.

The referenced issue was specifically due to the AutoMinify option on Cloudfare causing whitespace changes.

This option needs some way to say to say in that in this element whitespace actually matters and although any such marker would be non-standard I would argue it would be useful but until they provide that it simply cannot be used on Gitea - (until that is we move to an AJAX based diff scheme which would nicely resolve this)

I'm not certain if there's any way other way we could mitigate.

@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf
Copy link
Contributor Author

I think it might be helpful to add something to the FAQ here.

The referenced issue was specifically due to the AutoMinify option on Cloudfare causing whitespace changes.

This option needs some way to say to say in that in this element whitespace actually matters and although any such marker would be non-standard I would argue it would be useful but until they provide that it simply cannot be used on Gitea - (until that is we move to an AJAX based diff scheme which would nicely resolve this)

I'm not certain if there's any way other way we could mitigate.

What exactly should be added to the FAQ?
Something like this?

Q: I want to use CF, is it ok?
A: Yes - but - make sure AutoMinify (or similar option with other providers) is turned off since it causes trouble.

@6543 6543 added the type/docs This PR mainly updates/creates documentation label Dec 22, 2020
@6543 6543 added this to the 1.14.0 milestone Dec 22, 2020
@CirnoT
Copy link
Contributor

CirnoT commented Dec 23, 2020

This option needs some way to say to say in that in this element whitespace actually matters and although any such marker would be non-standard I would argue it would be useful but until they provide that it simply cannot be used on Gitea - (until that is we move to an AJAX based diff scheme which would nicely resolve this)

I'm not certain if there's any way other way we could mitigate.

Nothing special about CF's minifier, it's us that are breaking norms by not putting code in <pre> or <code> (technically we are using <code> block but actual content is then wrapped in <span> elements). This will trip up pretty much any HTML minifier.

@CirnoT
Copy link
Contributor

CirnoT commented Dec 23, 2020

Something like this?

Q: I want to use CF, is it ok?
A: Yes - but - make sure AutoMinify (or similar option with other providers) is turned off since it causes trouble.

Please attach FAQ link on how to disable it on CF and specifically mention it's for HTML only, the options for JS and CSS are separate.

@zeripath
Copy link
Contributor

zeripath commented Dec 23, 2020

Nothing special about CF's minifier, it's us that are breaking norms by not putting code in <pre> or <code> (technically we are using <code> block but actual content is then wrapped in <span> elements). This will trip up pretty much any HTML minifier.

Would sticking the spaces in a <![CDATA[ ]]> help do you think?

@silverwind
Copy link
Member

silverwind commented Dec 23, 2020

I'd say we just replace one of the parent <div> in the code and diff views with <pre> (blame already has it) and that should be it to no longer confuse such broken minifiers.

BTW, GitHub would be broken too behind Cloudflare, they also have no <pre> or <code> in the ancestors of the code view.

As for this issue, I don't think we should pollute the issue template more. Less is more.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 18, 2021
@lunny
Copy link
Member

lunny commented Feb 18, 2021

If nobody against, I think this could be merged.

@6543
Copy link
Member

6543 commented Feb 18, 2021

ping

@6543 6543 merged commit c9a04cf into go-gitea:master Feb 18, 2021
@wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf wULLSnpAXbWZGYDYyhWTKKspEQoaYxXyhoisqHf deleted the are-you-using-cloudflare branch February 18, 2021 12:06
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/docs This PR mainly updates/creates documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants