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

Media Proxy を実装 #5649

Merged
merged 2 commits into from
Dec 19, 2019
Merged

Media Proxy を実装 #5649

merged 2 commits into from
Dec 19, 2019

Conversation

SanMurakami
Copy link
Contributor

同じ画像を何個ものインスタンスでストレージに保存するのは非効率なので、以下のメディアプロキシを使える機能を実装しました。

https://github.com/riku6460/media-proxy

@syuilo syuilo merged commit 9bc07c1 into misskey-dev:develop Dec 19, 2019
@syuilo
Copy link
Member

syuilo commented Dec 19, 2019

👍

@@ -22,7 +23,11 @@ export class DriveFileRepository extends Repository<DriveFile> {
}

public getPublicUrl(file: DriveFile, thumbnail = false): string | null {
return thumbnail ? (file.thumbnailUrl || file.webpublicUrl || null) : (file.webpublicUrl || file.url);
let url = thumbnail ? (file.thumbnailUrl || file.webpublicUrl || null) : (file.webpublicUrl || file.url);
if (file.src !== null && file.userHost !== null && config.mediaProxy !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

config.mediaProxyundefinedの時にtrueになっちゃいそう
全ての演算子は!= nullでよさそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

なるほど

Copy link
Contributor

Choose a reason for hiding this comment

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

file.srcはローカルのURLからアップロードでも使用されそう
file.userHostのチェックがあるから最終的には大丈夫かもしれないけどfile.uriを検証した方がいいのかも

Copy link
Member

Choose a reason for hiding this comment

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

そもそもこ関数、let使わずreturnを2回使う記法の方が好ましいと思います

return thumbnail ? (file.thumbnailUrl || file.webpublicUrl || null) : (file.webpublicUrl || file.url);
let url = thumbnail ? (file.thumbnailUrl || file.webpublicUrl || null) : (file.webpublicUrl || file.url);
if (file.src !== null && file.userHost !== null && config.mediaProxy !== null) {
url = `${config.mediaProxy}/${thumbnail ? 'thumbnail' : ''}?url=${file.src}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

確認してないけどfile.srcURLエンコードが必要かも

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

設定があればリモート分はとりあえずメディアプロキシに振り分けるという実装だと
メディアプロキシ側をオープンリダイレクトにせざるを得ないのがセキュリティ的に気になる
https://github.com/riku6460/media-proxy/blob/50991234b82380196b3ee604dbaa783d3f566c38/app.ts#L56-L61

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

外部のメディアプロキシサーバーを指定する方法ではなくて

特定のオプション有効時に
/files/:fileIdのように非オブジェクトストレージと同じURLでアクセスさせるようにして
/files/:fileIdのハンドラでリモートファイル未保存だったらProxyしてあげるような実装ではどうかしら
つまりインスタンス自体にProxyさせる(キャッシュは前段のNginxなりCDNにまかせる)

これならインスタンスが認知してない未知のURLに対するアクセスが拒否できる。

インスタンス間でURLを共有する形にはならないけど
そもそもProxyをするときには「リモートファイル未保存」オプションを有効にして運用すると思うので、「各インスタンス間で同じファイル保存を行いたくない」という要件は満たすには問題ないと思うわ。

@SanMurakami
Copy link
Contributor Author

1.misskey.local, 2.misskey.local, 3.misskey.local とあった場合にそれぞれでプロキシを行うって認識で大丈夫?
それなら複数ある場合に(少し特殊な環境かもしれないけど)無駄なリクエストが増える気がする

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

1.misskey.local, 2.misskey.local, 3.misskey.local とあった場合にそれぞれでプロキシを行うって認識で大丈夫?

Yes

あまり「クライアントから見たURLを同じにする」を突き詰めると結局リモート直リンが最適解になっちゃう気がする。

1ユーザーが複数のインスタンスで同じリモート投稿を見るというのがあまりないパターンだから、インスタンスでキャッシュが別れていてもさほど不具合がない気がする。

@SanMurakami
Copy link
Contributor Author

リモート直リンだと結構遅いサーバーがある(キャッシュをオフにしたら結構違いが出る)のと、サムネイルの生成が行われないのでモバイル等の従量課金制とか格安SIMとかのネットワークだと直ぐに容量を使い切りそう

Misskey側にリモートファイルのサムネイル(ビデオを含む)を生成するプログラムを組み込んでストレージに保存しないけどプロキシだけはするみたいなやつだと解決できそうな気がする

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

Misskey側にリモートファイルのサムネイル(ビデオを含む)を生成するプログラムを組み込んでストレージに保存しないけどプロキシだけはするみたいなやつだと解決できそうな気がする

たぶんm544がそんな実装
基本はオブジェクトストレージのリモートファイルキャッシュを使うけど
期限切れ削除とか最初から保存してない場合は本体でProxy (with image/video thumbnail) して、キャッシュはNginxとCloudFlareに任せちゃう感じ

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

↑たぶんメディアプロキシと競合するわけでもないから近日入れる…

@SanMurakami
Copy link
Contributor Author

なるほど
複数インスタンスを運営するときにMedia Proxy自体はあったほうが帯域節約になるので実装後も残してくれると助かる

@mei23
Copy link
Contributor

mei23 commented Dec 19, 2019

統合すると

メディアプロキシ設定ありでメディアなMINEだったらそっちに移譲(このメディアプロキシ機能)
(下の本体プロキシが機能があるならば、メディア以外のMINEは拒否するようにしてもいいかも)

リモートファイルのキャッシュが保存されてればそれを使用(今まで通り)

リモートファイルのキャッシュなし(期限切れor最初から保存しないオプション)&&本体Proxyを許可する設定だったら本体Proxyする(本体プロキシオプション)

直リン(今まで通り)

みたいなイメージ

@acid-chicken
Copy link
Member

実装が標準仕様に基づくものでなさそう(→ id のように将来的に複数の実装をサポートするのを見越した方が良さそう)なのと、既存の config スタイルとの協調性を保つことを考えると、

mediaProxy: http://127.0.0.1:3000

よりは、

proxyMedia:
  protocol: github:riku6460/media-proxy
  url: http://127.0.0.1:3000

とかの方がいいかなと思ったりするのですが、いかがでしょうか。

CC: @CookieRamen @mei23 @riku6460

@mei23
Copy link
Contributor

mei23 commented Dec 20, 2019

プロトコルや実装アプリの説明もなしにいきなり
非標準かつ要件不明な設定が登場してるのはやはり不自然な気はします。

複数の実装を切り替え出来るように増やしていくよりは
特定の運用環境でしか使えない機能はforkで行ったほうがいいかなと思います

ioはメディアサーバーから返るサムネイルにwebp変換される機能等があり
既にメディア部分は完全にバニラかというと怪しいので、今更バニラにこだわる必要もないかなと思います。

ioが公式で公式はバニラで運用する前提があるのかどうかはわからないです。

@SanMurakami
Copy link
Contributor Author

ioのメディア関連は出力部分に外部プログラムを経由させているだけでMisskey自体は公式のDockerイメージを利用してます。

forkして利用すると公式リポジトリの追従にラグが出るというのは置いといて、そもそも意外とMisskey自体複数インスタンスを運用している管理者が多いような気がするので必要と感じPRだしました。

もうmergeされてますが、必要ないと考えるならrevertして頂いて大丈夫です。

ただ、画像については外部プログラムを利用しないにしても、 https://images.weserv.nl/ を使うような実装があったほうが良いと思います。

@mei23
Copy link
Contributor

mei23 commented Dec 20, 2019

別に入れちゃったならいいかなというレベルです

@mei23
Copy link
Contributor

mei23 commented Dec 20, 2019

mediaProxy: http://127.0.0.1:3000

今見たら

  • 127.0.0.1:3000って必ずMisskeyがいそうな気がする
  • config値の最後に/を含めるべきかどうか

と思いました

@mei23
Copy link
Contributor

mei23 commented Dec 20, 2019

クライアントが直接見るURLなので
例示は https かつ ドメイン名 かつ ポート指定なし の方が親切かなと

@mei23
Copy link
Contributor

mei23 commented Dec 20, 2019

あくまでもURLなのでサンプルはmediaProxy: https://example.net/proxyみたいにして

パスに/thumbnail/付加してると configで/を付けるか悩んじゃうので
与えられたパス自体は変更せずに、追加の情報はクエリで付加する

https://example.com/proxy?url=https://...&thumbnail=1

https://example.com/proxy?url=https://...

とかがいいかなと

mei23 referenced this pull request Dec 31, 2019
* Media Proxy を実装

* サンプルを追加

* https://github.com/syuilo/misskey/pull/5649#discussion_r359967471 の修正

* https://github.com/syuilo/misskey/pull/5649#discussion_r359967966 の修正

* https://github.com/syuilo/misskey/pull/5649#discussion_r359968219 の修正

* 期限切れ/未保存リモートファイルのローカルプロキシ

* 設定

* 説明

* comment out

* fix

Co-authored-by: 和風ドレッシング <37681609+CookieRamen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants