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

entrypoint.sh内にスペースインデントが入ってしまう問題の対応と可読性の向上 #105

Merged
merged 3 commits into from
Sep 18, 2021

Conversation

tantan-tanuki
Copy link
Contributor

masterをマージしてから再度修正した際にforceでpushしたため
再OPEN出来ませんでした

https://github.com/Hiroshiba/voicevox_engine/pull/102

上記PRの続きとなりややこしくなってしまい申し訳ありません

ご査収のほど宜しくおねがいします

ヒアドキュメント内のインデントに関しては
<<EOT
ではなく
<<- EOT
にすることでタブインデントが可能となりますのでそちらの方に変更させていただきました
コーディング規約的にタブインデントが禁止されているようであれば再度対応したいと思います

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM! Userを途中で作るので、わかりやすく感じました。

ちょっとgosuをよく存じず、gosuを利用するメリットを僕が把握できていない可能性があるかもしれません。
@aoirint さん、もしよろしければご意見頂けると嬉しいです!

@aoirint
Copy link
Member

aoirint commented Sep 17, 2021

コンテナ内のコマンドを一般ユーザで実行することは、
コンテナブレイクアウト(Dockerによる環境の隔離が無効化される)の脆弱性が発見された際の影響を軽減するためにするものという認識で、正直あまり詳しくないですが、念のためにそうしています。

gosuは、コンテナの起動時にrootユーザで初期化処理(ldconfigの実行)をし、HTTPサーバを動作させるpython3コマンドは一般ユーザで実行する、ために導入しました。ENTRYPOINTをrootユーザで実行し、CMDを一般ユーザで実行するという形になっています(デバッグの都合でgosuをentrypoint.sh側ではなくCMDに含めていますが、コマンドが長くなる以外は問題ないと思っています、ENTRYPOINTのコマンドライン引数としてCMDが使われます)。

このPRでは、ldconfigの実行をDockerイメージのレイヤーにしているため、gosuが不要になっています。

個人的には、USER命令を使わず、RUN命令の中のコマンドがすべてrootユーザで実行されるという想定をもっていた方が、Dockerfileが読みやすく、書きやすくなると思っています(差分表示、処理順の自由度において)。

ちなみに、マルチステージビルドでは、USER命令の実行はレイヤーとして引き継がれます。

いまのところ、 #96 では、ホストからマウントされたディレクトリの所有者を書き換えるchownコマンドをentrypointで実行するためにgosuを使っています。

とりあえず事例を調べたところ、mysqlの公式イメージでgosuが使われているようでした。
https://github.com/docker-library/mysql/blob/c506174eab8ae160f56483e8d72410f8f1e1470f/8.0/Dockerfile.debian#L14-L16

@Hiroshiba
Copy link
Member

Hiroshiba commented Sep 17, 2021

@aoirint なるほど、詳細をありがとうございます!

論点は「gosuを使うかUSERを使うか」というよりも、「Dockerfileのマルチステージビルドを再利用可能だとして使い回す設計にするかどうか」だと感じました。

使い回す設計なら、次のステージ?に影響が及ぶUSERの使用は控えてgosuを利用すべきだと思います。
使い回さない設計なら、一般的に馴染みの深いUSERを使っても良いと思います。

個人的には、前者は後者を兼ねているというのもあって、gosuを使用する方が良いのかなと思いました。
が、そもそも別解がありそうな気もしているので、お二人に意見お伺いしたいです。

Dockerfile Outdated
Comment on lines 199 to 204
cat <<- EOT > /entrypoint.sh
#!/bin/bash
cat /opt/voicevox_core/README.txt > /dev/stderr
exec "\$@"
EOT
chmod +x /entrypoint.sh
Copy link
Member

@Hiroshiba Hiroshiba Sep 17, 2021

Choose a reason for hiding this comment

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

別件なのですが、tabとspaceが混じっているとちょっと読みづらいかもしれません。
コーディング規約に入っておらず申し訳ないのですが、<<でお願いします!

@tantan-tanuki
Copy link
Contributor Author

@aoirintさんがgosuを採用したのは

  • RUN命令の中のコマンドがすべてrootユーザで実行されるという想定をもっていた方が、Dockerfileが読みやすく、書きやすくなる
  • entrypoint.shはrootで実行してpythonはuserで実行したい

が大きな理由かと読み取りました

ちょっと理解が浅くて申し訳ないのですが2点確認したいのは

コンテナブレイクアウトに関しては外部公開されているコンテナに関して意識するものと思っていましたがHTTP開いているので意識する必要があるということでしょうか?

ldconfigはコンテナビルド時に実行してしまえばもう不要かと思うのですがentrypoint.sh内で実行していたのは何か別の理由があったのでしょうか?

次に@Hiroshibaさんの

インデントにタブとスペースが混じると可読性が悪いという問題に対してですが

ヒアドキュメントでスペースインデントを採用してファイルを作成すると作成されたファイル内にそのままスペースインデントまで残ってしまう
という問題があるため<<-の採用をしたのですがインデントが混在するのは確かに可読性が悪いですね
なのでCOPYコマンドでファイルを作成するように変更します

ということで現状私が理解した範囲で意見をまとめたものをコミットしてみました

タイトルとは違って
entrypoint.sh内にスペースインデントが入ってしまう問題の対応と可読性の向上
みたいな内容になってしまいましたがご確認下さい

Dockerfile Outdated
# if all tries are failed, `docker build` will be failed.

# Download openjtalk dictionary
parallel --retries 5 --delay 5 --ungroup <<- EOT
Copy link
Member

Choose a reason for hiding this comment

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

ここでも<<-が用いられていそうです。(修正忘れ?)

@aoirint
Copy link
Member

aoirint commented Sep 18, 2021

@tantan-tanuki

コンテナブレイクアウトに関しては
...
HTTP開いているので意識する必要があるということでしょうか?

はい、そのように考えています(あくまでセキュリティ素人の考えです)。

細工したHTTPリクエストにより、実装やライブラリの脆弱性を利用して、
ホスト側に攻撃が行われる可能性を考えています。

通常では隔離されたコンテナの外に出ることはできませんが、
Dockerの未知の脆弱性と組み合わせることで、隔離を突破できる可能性があります。

このとき、HTTPポートを開くプロセス(または脆弱性の影響を受けるプロセス)を
rootユーザ(UID 0)で実行していた場合、
ホスト側のroot権限を獲得できる可能性があります。

そこで、メインのプロセス(python3 run.py)を一般ユーザで実行することで、上記の流れによる、ホスト側のroot権限獲得を予防しようとしています。

もっと本格的に対策するならば、コンテナ内一般ユーザのUID・GIDをランダムに設定する(ホスト側で権限を持つUID・GIDを使用しない)ということも考えられると思います(同じ流れにより、ホスト側の一般ユーザとしては、振る舞える可能性があるため)。

ちなみに、サンプルの実行コマンドでは、カジュアルに実行する可能性があるため、-p 127.0.0.1:50021:50021のようにローカルループバックアドレスにbindし、他のコンピュータからの予期しないアクセスを予防していますが、仕様としてはLAN内(プライベートネットワーク)や、インターネットに公開することも可能です。

規約的にも、例えばWebアプリケーションに組み込むという用途で、パブリックなWeb APIとして公開することも可能だと考えています(直接的には、計算資源を提供するメリットがありませんが)。

インターネットに公開されたWebアプリケーションから、プライベートネットワークのAPIにアクセスすることも仕様上可能だと考えています(これはブラウザ上で動作するWebアプリケーションからのアクセスならば、CORSを適切に設定すれば制御できるように思いますが)。


ldconfigはコンテナビルド時に実行してしまえばもう不要かと思うのですが

わたしも不要だなと思っていました。可能ならそのようにする方がよいと思っています。それから、/etc/ld.so.cacheの削除も実際には不要だと思います。

しかし、ldconfigをコンテナ実行時に実行している経緯はあるので、以下で説明します。

entrypoint.sh内で実行していたのは何か別の理由があったのでしょうか?

はい、 #88 の開発中に正確な原因がわかっていない問題が発生したため、念のためにentrypointでldconfigを実行している形になります。

しかし、原因が分かっていないため、entrypoint.sh内で実行しなければならない、という明確な根拠はありません。

経緯としては、 #88 の開発中に、
Dockerコンテナの起動後にあらためてldconfigを実行しなければ、
LibTorchの共有ライブラリのうち、libtorch_cuda_cpp.soのみが認識されないという現象が起きたということがありました。

この現象は、entrypointでのldconfig実行の挿入で解決しましたが、検証の方法に明るくないため、正確な原因の調査をしていません。

ローカルDockerイメージキャッシュを使う限りにおいて、ローカル環境で再現していますが、キャッシュを破棄した場合に再現するかは確認していません。

当初LibTorchのPre-cxx11 ABIを誤って使用していたことが原因かもしれませんが、問題のコミットを確認すると(LibTorchのダウンロードURLが)cxx11 ABIのものになっています。

いまのところ、Dockerのイメージキャッシュが影響している可能性を考えています。

あまり明るくないので憶測なのですが、ldconfigがバックグラウンドプロセスを起動していて、その実行が終わる前にldconfigが制御を返してRUN命令の実行が終わり、バックグラウンドプロセスが強制終了され、ldconfigのキャッシュが一部しか書き込まれていないという可能性も考えています。

他に、RUN命令内の処理に失敗したがRUN命令自体は成功した扱いとなった可能性を考えています。
exit codeが0でなければRUN命令の実行が中断され、失敗すると思っている(&&でコマンドが繋がれるため)のですが、間違いかもしれませんし、
ldconfigは一部の処理に失敗してもexit codeが0になるのかもしれません。

rm -f /etc/ld.so.cacheについては、Hiroshiba@4f8cec7 において効果がなかったため、削除してよいと思っています。

@tantan-tanuki
Copy link
Contributor Author

tantan-tanuki commented Sep 18, 2021

@aoirintさん

はい、そのように考えています(あくまでセキュリティ素人の考えです)。

なるほどゆくゆくは外部公開する前提で作成されていたのですね
処理が重いのでローカルでしか使うことないだろうという前提で読んでいました

Dockerコンテナの起動後にあらためてldconfigを実行しなければ、
LibTorchの共有ライブラリのうち、libtorch_cuda_cpp.soのみが認識されないという現象が起きたということがありました。

これは恐らく

COPY --from=download-libtorch-env /etc/ld.so.conf.d/libtorch.conf

より前にldconfigしているのが原因じゃないでしょうかこのファイルがない状態でldconfigしたので
libtorch.confに記載されている/opt/libtorch/lib配下のライブラリにパスが通らなかったんだと思います
今回は必要なファイルを前段で揃えてから必要な処理を実施するようにしているので恐らく問題は解消しているかと思います

rm -f /etc/ld.so.cacheについては、4f8cec7 において効果がなかったため、削除してよいと思っています。

了解です!

@Hiroshibaさん

ここでも<<-が用いられていそうです。(修正忘れ?)

前回もこんなミスしてしまいましたね
申し訳ない😅

ここは<<だとEOTのインデント位置が見苦しくなってしまいますので\での改行に変更させていただきました

恐らくこれで問題ないかと思いますのでご確認をお願いします

@aoirint
Copy link
Member

aoirint commented Sep 18, 2021

@tantan-tanuki

rm -f /etc/ld.so.cacheについては、4f8cec7 において効果がなかったため、削除してよいと思っています。

了解です!

ありがとうございます!

このファイルがない状態でldconfigしたので
libtorch.confに記載されている/opt/libtorch/lib配下のライブラリにパスが通らなかったんだと思います

どうなんでしょう..。以下は問題が起きたコミット de20f62 の記述なのですが、その点については問題がないように見えるんですよね..

再発しないかがちょっと心配ではありますが、おそらくわたしの環境の一時的な問題で、問題ないと思います! (どこかではっきりと原因がわかるといいのですが..)

https://github.com/Hiroshiba/voicevox_engine/blob/de20f62f34bbe55ca465fa885318d3d4606cd0c3/Dockerfile#L94-L99

ちなみに、以下のような記述は、download-libcoreやdownload-libtorchを単体でイメージ化した場合に、それぞれの共有ライブラリを参照できるように、という意図で記述しています(が、需要はほぼないと思うので、冗長になるldconfig用のentrypointまでは作成しませんでした)。

https://github.com/Hiroshiba/voicevox_engine/blob/de20f62f34bbe55ca465fa885318d3d4606cd0c3/Dockerfile#L28-L31

https://github.com/Hiroshiba/voicevox_engine/blob/de20f62f34bbe55ca465fa885318d3d4606cd0c3/Dockerfile#L57-L60

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

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

LGTM! 議論により良い着地点に到達できたのではないかと思いました。
お二人とも、ありがとうございます!

(タイトルがプルリクエスト内容と変わってしまっているので、勝手ながら変更させていただきます。)

@Hiroshiba Hiroshiba changed the title gosuの削除 entrypoint.sh内にスペースインデントが入ってしまう問題の対応と可読性の向上 Sep 18, 2021
@Hiroshiba Hiroshiba merged commit dfb917a into VOICEVOX:master Sep 18, 2021
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.

3 participants