Performance improvements after caching html fragments

• re-use event cache with other users
• cache commit fragment
• cache issue fragment in list
• cache comment parsed with markdown

@DouweM @rspeicher please take a look

Added ~18308 label

Added 1 commit:

• 70666a52 - Use to_reference menthod when render issue id in list

Status changed to merged

mentioned in commit e934a62a

We shouldn't do this, what is rendered as a link in Markdown depends on the user's access to other projects and groups.

I agree we should cache it, but we need to add the current_user to the key.

cc @rspeicher

@dzaporozhets Ping.

@DouweM adding current_user to cache makes it less efficient. If you have one user visited page with comments -> all other visitors will receive cached copy. Its much faster and uses less storage.

I understand, but that's not the issue... We're giving up security with the current implementation because users will be able to see referenced groups and projects that they shouldnt't.

@dzaporozhets Ping

@dzaporozhets I'm guessing it's too late to revert this for 7.13 now, but did you see my concern here:

We're giving up security with the current implementation because users will be able to see referenced groups and projects that they shouldn't.

/cc: @vsizov

@DouweM I think it would be fine to fix this with a patch release.
It is a security concern, but I don't think it's necessary to delay the release or hurry to revert it today.

@haynes I hoped it would've been fixed earlier, or reverted until we had a better solution. I really don't like that we're introducing a security issue (even if small) in a release, when we've known that it exists for weeks.

@DouweM @stanhu On a second look.... why not remove the caching for notes for now again? We only need to remove one line right?

Thanks @haynes.

I reverted caching itemshttps://gitlab.com/gitlab-org/gitlab-ce/issues/2054

@dzaporozhets I just realized the "Push" event also depends on the current user since the "Create Merge Request" link is only shown if it concerns the user's own push.

@DouweM that bad. I dont want push to be cached with current_user - it does not give good benefits in this way.

I really don't like fact that shared resources like dashboard event or comment are rendered with current_user context. We should get rid of this.

Basecamp has a really cool idea for this, where they render both the the author == current_user and author != current_user Versions at the same time, with `data-user="username"

Commented too soon, and can't edit on phone :(

Continuing:

data-user-is="DouweM" on the one and data-user-is-not="DouweM" on the other, and then use dynamic CSS to show the appropriate one:

[data-user-is="DouweM"] {
  display: block;
}

[data-user-is-not="DouweM"] {
  display: none;
}

That way the whole thing can be cached, and the user will always see the appropriate parts.

(we'd also need to CSS to hide the one with data-user-is for everyone else, but that's really straightforward)

@dzaporozhets Ping!

@DouweM interesting proposal. Please submit merge request!

mentioned in merge request !1014 (merged)

mentioned in issue #2033 (closed)

mentioned in issue #2034 (closed)

mentioned in commit 95d89e44

Again I will repeat:

• markdown rendering is slow. It should be cached. Especially for comments which are rendered again and again in popular issues
• events and comment rendering should not depends on current user context. Its bad architecture design. We pass current user variable so deep into code that it becomes scary.
• if you revert caching in 7.13 make sure to create issue and bring it back in 7.14 with proper fixes

Cc @vsizov @DouweM

@dzaporozhets Valery already created an issue for this. :)

https://gitlab.com/gitlab-org/gitlab-ce/issues/2054

markdown rendering is slow. It should be cached. Especially for comments which are rendered again and again in popular issues

Agreed.

events and comment rendering should not depends on current user context. Its bad architecture design. We pass current user variable so deep into code that it becomes scary.

It seems to be that this in inevitable, since group and project referencing (and following from that, issue, MR, commit, etc) inherently depends on what groups/projects exist as far as the user knows.

if you revert caching in 7.13 make sure to create issue and bring it back in 7.14 with proper fixes

That's the plan :)

@DouweM markdown processing and filtering what is available to user should be separated. Markdown != authorization. We can cache markdown output but always process authorization filter

mentioned in issue #2063 (closed)

Started a new issue for discussion, see system note above.

@dzaporozhets Rendering @private-group as a link leaks group existence. Rendering public-group/private-project#123 as a link leaks project and issue existence, and leaks the issue title since it's in the HTML link's title attribute. As far as I can see, Markdown and authorization need to go hand in hand, at least to some extent.

Idea:

Render every link in Markdown, regardless of the current_user's access level, adding data-group_id and data-project_id to all rendered links. Cache this output HTML. Upon rendering, run one last pipeline processor over this cached HTML, that will verify access to the group and project IDs, and replace the links by their text content if the user doesn't have access.

This would allow caching of almost all of the overhead of Markdown rendering.

cc @rspeicher