diff --git a/lib/livebook/file_system/s3.ex b/lib/livebook/file_system/s3.ex index 3b919e3b2..d128232e6 100644 --- a/lib/livebook/file_system/s3.ex +++ b/lib/livebook/file_system/s3.ex @@ -24,43 +24,11 @@ defmodule Livebook.FileSystem.S3 do end @doc """ - Returns a new file system struct. - - ## Options - - * `:region` - the bucket region. By default the URL is assumed - to have the format `*.[region].[rootdomain].com` and the region - is inferred from that URL - - * `:external_id` - the external id from Teams. - - * `:hub_id` - the Hub id. - - * `:id` - the file system id. - + Infers region from the given bucket URL. """ - @spec new(String.t(), String.t(), String.t(), keyword()) :: t() - def new(bucket_url, access_key_id, secret_access_key, opts \\ []) do - opts = Keyword.validate!(opts, [:region, :external_id, :hub_id, :id]) - - bucket_url = String.trim_trailing(bucket_url, "/") - region = opts[:region] || region_from_uri(bucket_url) - - hub_id = Keyword.get(opts, :hub_id, Livebook.Hubs.Personal.id()) - id = opts[:id] || id(hub_id, bucket_url) - - %__MODULE__{ - id: id, - bucket_url: bucket_url, - external_id: opts[:external_id], - region: region, - access_key_id: access_key_id, - secret_access_key: secret_access_key, - hub_id: hub_id - } - end - - defp region_from_uri(uri) do + @spec region_from_uri(String.t()) :: String.t() + # TODO: make it private again on Livebook v0.12 + def region_from_uri(uri) do # For many services the API host is of the form *.[region].[rootdomain].com %{host: host} = URI.parse(uri) splitted_host = host |> String.split(".") |> Enum.reverse() @@ -72,36 +40,6 @@ defmodule Livebook.FileSystem.S3 do end end - @doc """ - Parses file system from a configuration map. - """ - @spec from_config(map()) :: {:ok, t()} | {:error, String.t()} - def from_config(config) do - case config do - %{ - bucket_url: bucket_url, - access_key_id: access_key_id, - secret_access_key: secret_access_key - } -> - file_system = - new(bucket_url, access_key_id, secret_access_key, - region: config[:region], - external_id: config[:external_id] - ) - - {:ok, file_system} - - _config -> - {:error, - "S3 configuration is expected to have keys: :bucket_url, :access_key_id and :secret_access_key, but got #{inspect(config)}"} - end - end - - @spec to_config(t()) :: map() - def to_config(%__MODULE__{} = s3) do - Map.take(s3, [:bucket_url, :region, :access_key_id, :secret_access_key]) - end - @doc """ Returns an `%Ecto.Changeset{}` for tracking file system changes. """ @@ -121,7 +59,7 @@ defmodule Livebook.FileSystem.S3 do :hub_id ]) |> put_region_from_uri() - |> validate_required([:bucket_url, :access_key_id, :secret_access_key]) + |> validate_required([:bucket_url, :region, :access_key_id, :secret_access_key, :hub_id]) |> Livebook.Utils.validate_url(:bucket_url) |> put_id() end @@ -421,19 +359,31 @@ defimpl Livebook.FileSystem, for: Livebook.FileSystem.S3 do }) end - def load(_file_system, fields) do - S3.new(fields.bucket_url, fields.access_key_id, fields.secret_access_key, - region: fields[:region], - external_id: fields[:external_id], - id: fields[:id], - hub_id: fields[:hub_id] - ) + def load(file_system, fields) do + %{ + file_system + | id: fields.id, + bucket_url: fields.bucket_url, + external_id: fields.external_id, + region: fields.region, + access_key_id: fields.access_key_id, + secret_access_key: fields.secret_access_key, + hub_id: fields.hub_id + } end def dump(file_system) do file_system |> Map.from_struct() - |> Map.take([:id, :bucket_url, :region, :access_key_id, :secret_access_key, :hub_id]) + |> Map.take([ + :id, + :bucket_url, + :region, + :access_key_id, + :secret_access_key, + :hub_id, + :external_id + ]) end def external_metadata(file_system) do diff --git a/lib/livebook/live_markdown/import.ex b/lib/livebook/live_markdown/import.ex index 7315cb9f4..6dc24c5fe 100644 --- a/lib/livebook/live_markdown/import.ex +++ b/lib/livebook/live_markdown/import.ex @@ -414,7 +414,7 @@ defmodule Livebook.LiveMarkdown.Import do when is_list(file_entry_metadata) -> file_system_by_id = if Enum.any?(file_entry_metadata, &(&1["type"] == "file")) do - for file_system <- Livebook.Settings.file_systems(), + for file_system <- Livebook.Hubs.get_file_systems(), do: {file_system.id, file_system}, into: %{} else diff --git a/lib/livebook/migration.ex b/lib/livebook/migration.ex index 79b6fabdf..e28aa0b2d 100644 --- a/lib/livebook/migration.ex +++ b/lib/livebook/migration.ex @@ -64,9 +64,19 @@ defmodule Livebook.Migration do id_mapping = for config <- configs, into: %{} do old_id = config.id + + # Ensure new file system fields + new_fields = %{ + hub_id: Livebook.Hubs.Personal.id(), + external_id: nil, + region: Livebook.FileSystem.S3.region_from_uri(config.bucket_url) + } + + config = Map.merge(new_fields, config) + # At this point S3 is the only file system we store - {:ok, file_system} = Livebook.FileSystem.S3.from_config(config) - Livebook.Settings.save_file_system(file_system) + file_system = Livebook.FileSystems.load("s3", config) + Livebook.Hubs.Personal.save_file_system(file_system) Livebook.Storage.delete(:filesystem, old_id) {old_id, file_system.id} end @@ -92,11 +102,10 @@ defmodule Livebook.Migration do with {:ok, default_file_system_id} <- Livebook.Storage.fetch_key(:settings, "global", :default_file_system_id) do - with {:ok, default_file_system} <- - Livebook.Settings.fetch_file_system(default_file_system_id) do - default_dir = Livebook.FileSystem.File.new(default_file_system) - Livebook.Settings.set_default_dir(default_dir) - end + Livebook.Hubs.get_file_systems() + |> Enum.find(&(&1.id == default_file_system_id)) + |> Livebook.FileSystem.File.new() + |> Livebook.Settings.set_default_dir() Livebook.Storage.delete_key(:settings, "global", :default_file_system_id) end diff --git a/lib/livebook/notebook_manager.ex b/lib/livebook/notebook_manager.ex index 1814b7432..1dfc75c1c 100644 --- a/lib/livebook/notebook_manager.ex +++ b/lib/livebook/notebook_manager.ex @@ -243,8 +243,13 @@ defmodule Livebook.NotebookManager do _ -> %{} end + file_systems = + Livebook.Storage.all(:file_systems) + |> Enum.sort_by(& &1.bucket_url) + |> Enum.map(fn fields -> Livebook.FileSystems.load(fields.type, fields) end) + file_system_by_id = - for file_system <- Livebook.Settings.file_systems(), + for file_system <- file_systems, do: {file_system.id, file_system}, into: %{} diff --git a/lib/livebook/settings.ex b/lib/livebook/settings.ex index 001bd4b5d..f67bed1a5 100644 --- a/lib/livebook/settings.ex +++ b/lib/livebook/settings.ex @@ -42,70 +42,6 @@ defmodule Livebook.Settings do Storage.delete_key(:settings, "global", :autosave_path) end - @doc """ - Returns all known file systems. - """ - @spec file_systems() :: list(FileSystem.t()) - def file_systems() do - restored_file_systems = - Storage.all(:file_systems) - |> Enum.sort_by(&Map.get(&1, :order, System.os_time())) - |> Enum.map(&storage_to_fs/1) - - [Livebook.Config.local_file_system() | restored_file_systems] - end - - @doc """ - Finds a file system by id. - """ - @spec fetch_file_system(FileSystem.id()) :: {:ok, FileSystem.t()} - def fetch_file_system(file_system_id) do - local_file_system = Livebook.Config.local_file_system() - - if file_system_id == local_file_system.id do - {:ok, local_file_system} - else - with {:ok, config} <- Storage.fetch(:file_systems, file_system_id) do - {:ok, storage_to_fs(config)} - end - end - end - - @doc """ - Saves a new file system to the configured ones. - """ - @spec save_file_system(FileSystem.t()) :: :ok - def save_file_system(%FileSystem.S3{} = file_system) do - attributes = - file_system - |> FileSystem.S3.to_config() - |> Map.to_list() - - attrs = [{:type, "s3"}, {:order, System.os_time()} | attributes] - :ok = Storage.insert(:file_systems, file_system.id, attrs) - end - - @doc """ - Removes the given file system from the configured ones. - """ - @spec remove_file_system(FileSystem.id()) :: :ok - def remove_file_system(file_system_id) do - if default_dir().file_system.id == file_system_id do - Storage.delete_key(:settings, "global", :default_dir) - end - - Livebook.NotebookManager.remove_file_system(file_system_id) - - Storage.delete(:file_systems, file_system_id) - end - - defp storage_to_fs(%{type: "s3"} = config) do - case FileSystem.S3.from_config(config) do - {:ok, fs} -> fs - {:error, message} -> raise ArgumentError, "invalid S3 configuration: #{message}" - end - end - @doc """ Returns whether the update check is enabled. """ @@ -246,9 +182,10 @@ defmodule Livebook.Settings do @spec default_dir() :: FileSystem.File.t() def default_dir() do with {:ok, %{file_system_id: file_system_id, path: path}} <- - Storage.fetch_key(:settings, "global", :default_dir), - {:ok, file_system} <- fetch_file_system(file_system_id) do - FileSystem.File.new(file_system, path) + Storage.fetch_key(:settings, "global", :default_dir) do + Livebook.Hubs.get_file_systems() + |> Enum.find(&(&1.id == file_system_id)) + |> FileSystem.File.new(path) else _ -> FileSystem.File.new(Livebook.Config.local_file_system()) end diff --git a/test/livebook/file_system/file_test.exs b/test/livebook/file_system/file_test.exs index eda721ec1..d94199110 100644 --- a/test/livebook/file_system/file_test.exs +++ b/test/livebook/file_system/file_test.exs @@ -1,6 +1,8 @@ defmodule Livebook.FileSystem.FileTest do use ExUnit.Case, async: true + import Livebook.Factory + import Livebook.HubHelpers import Livebook.TestHelpers alias Livebook.FileSystem @@ -271,7 +273,7 @@ defmodule Livebook.FileSystem.FileTest do test "supports regular files from different file systems via stream read and write", %{tmp_dir: tmp_dir} do bypass = Bypass.open() - s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret") + s3_fs = build_bypass_file_system(bypass) local_fs = FileSystem.Local.new() create_tree!(tmp_dir, @@ -295,7 +297,7 @@ defmodule Livebook.FileSystem.FileTest do test "supports directories from different file systems via stream read and write", %{tmp_dir: tmp_dir} do bypass = Bypass.open() - s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret") + s3_fs = build_bypass_file_system(bypass) local_fs = FileSystem.Local.new() create_tree!(tmp_dir, @@ -329,7 +331,7 @@ defmodule Livebook.FileSystem.FileTest do @tag :tmp_dir test "returns an error when files from different file systems are given and the destination file exists", %{tmp_dir: tmp_dir} do - s3_fs = FileSystem.S3.new("https://example.com/mybucket", "key", "secret") + s3_fs = build(:fs_s3, bucket_url: "https://example.com/mybucket") local_fs = FileSystem.Local.new() create_tree!(tmp_dir, @@ -349,7 +351,7 @@ defmodule Livebook.FileSystem.FileTest do test "supports regular files from different file systems via explicit read, write, delete", %{tmp_dir: tmp_dir} do bypass = Bypass.open() - s3_fs = FileSystem.S3.new("http://localhost:#{bypass.port}/mybucket", "key", "secret") + s3_fs = build_bypass_file_system(bypass) local_fs = FileSystem.Local.new() create_tree!(tmp_dir, diff --git a/test/livebook/file_system/s3_test.exs b/test/livebook/file_system/s3_test.exs index c7bdd1ca0..1f0102663 100644 --- a/test/livebook/file_system/s3_test.exs +++ b/test/livebook/file_system/s3_test.exs @@ -11,25 +11,6 @@ defmodule Livebook.FileSystem.S3Test do {:ok, bypass: bypass, file_system: file_system} end - describe "new/3" do - test "trims trailing slash in bucket URL" do - assert %{bucket_url: "https://example.com/mybucket"} = - S3.new("https://example.com/mybucket/", "key", "secret") - end - - test "determines region based on the URL by default" do - assert %{region: "eu-central-1"} = - S3.new("https://s3.eu-central-1.amazonaws.com/mybucket", "key", "secret") - end - - test "accepts explicit region as an option" do - assert %{region: "auto"} = - S3.new("https://s3.eu-central-1.amazonaws.com/mybucket", "key", "secret", - region: "auto" - ) - end - end - describe "FileSystem.default_path/1" do test "returns the root path" do file_system = build(:fs_s3, bucket_url: "https://example.com/mybucket", region: "auto") @@ -988,66 +969,55 @@ defmodule Livebook.FileSystem.S3Test do end describe "FileSystem.load/2" do - test "loads region and external id from fields map" do + test "loads from atom keys" do + bucket_url = "https://mybucket.s3.amazonaws.com" + hash = :crypto.hash(:sha256, bucket_url) + encrypted_hash = Base.url_encode64(hash, padding: false) + fields = %{ - bucket_url: "https://mybucket.s3.amazonaws.com", + id: "s3-#{encrypted_hash}", + bucket_url: bucket_url, region: "us-east-1", external_id: "123456789", access_key_id: "key", - secret_access_key: "secret" + secret_access_key: "secret", + hub_id: "personal-hub" } - hash = :crypto.hash(:sha256, fields.bucket_url) - id = "s3-#{Base.url_encode64(hash, padding: false)}" - assert FileSystem.load(%S3{}, fields) == %S3{ - id: id, + id: fields.id, bucket_url: fields.bucket_url, external_id: fields.external_id, region: fields.region, access_key_id: fields.access_key_id, - secret_access_key: fields.secret_access_key - } - end - - test "loads region from bucket url" do - fields = %{ - bucket_url: "https://mybucket.s3.us-east-1.amazonaws.com", - access_key_id: "key", - secret_access_key: "secret" - } - - hash = :crypto.hash(:sha256, fields.bucket_url) - - assert FileSystem.load(%S3{}, fields) == %S3{ - id: "s3-#{Base.url_encode64(hash, padding: false)}", - bucket_url: fields.bucket_url, - external_id: nil, - region: "us-east-1", - access_key_id: fields.access_key_id, - secret_access_key: fields.secret_access_key + secret_access_key: fields.secret_access_key, + hub_id: fields.hub_id } end test "loads from string keys" do + bucket_url = "https://mybucket.s3.amazonaws.com" + hash = :crypto.hash(:sha256, bucket_url) + encrypted_hash = Base.url_encode64(hash, padding: false) + fields = %{ - "bucket_url" => "https://mybucket.s3.amazonaws.com", + "id" => "s3-#{encrypted_hash}", + "bucket_url" => bucket_url, "region" => "us-east-1", "external_id" => "123456789", "access_key_id" => "key", - "secret_access_key" => "secret" + "secret_access_key" => "secret", + "hub_id" => "personal-hub" } - hash = :crypto.hash(:sha256, fields["bucket_url"]) - id = "s3-#{Base.url_encode64(hash, padding: false)}" - assert FileSystem.load(%S3{}, fields) == %S3{ - id: id, + id: fields["id"], bucket_url: fields["bucket_url"], external_id: fields["external_id"], region: fields["region"], access_key_id: fields["access_key_id"], - secret_access_key: fields["secret_access_key"] + secret_access_key: fields["secret_access_key"], + hub_id: fields["hub_id"] } end end @@ -1062,7 +1032,8 @@ defmodule Livebook.FileSystem.S3Test do region: "us-east-1", access_key_id: "key", secret_access_key: "secret", - hub_id: "personal-hub" + hub_id: "personal-hub", + external_id: nil } end end diff --git a/test/livebook/session_test.exs b/test/livebook/session_test.exs index be2aedbda..87e3a678a 100644 --- a/test/livebook/session_test.exs +++ b/test/livebook/session_test.exs @@ -1,6 +1,7 @@ defmodule Livebook.SessionTest do use ExUnit.Case, async: true + import Livebook.HubHelpers import Livebook.TestHelpers alias Livebook.{Session, Delta, Runtime, Utils, Notebook, FileSystem, Apps, App} @@ -1653,8 +1654,8 @@ defmodule Livebook.SessionTest do test "when remote :file replies with the cached path" do bypass = Bypass.open() - bucket_url = "http://localhost:#{bypass.port}/mybucket" - s3_fs = FileSystem.S3.new(bucket_url, "key", "secret") + s3_fs = build_bypass_file_system(bypass) + bucket_url = s3_fs.bucket_url Bypass.expect_once(bypass, "GET", "/mybucket/image.jpg", fn conn -> Plug.Conn.resp(conn, 200, "content") diff --git a/test/livebook_teams/web/hub/edit_live_test.exs b/test/livebook_teams/web/hub/edit_live_test.exs index 559eea4a7..95a195579 100644 --- a/test/livebook_teams/web/hub/edit_live_test.exs +++ b/test/livebook_teams/web/hub/edit_live_test.exs @@ -373,7 +373,7 @@ defmodule LivebookWeb.Integration.Hub.EditLiveTest do end defp expect_s3_listing(bypass) do - Bypass.expect_once(bypass, "GET", "/", fn conn -> + Bypass.expect_once(bypass, "GET", "/mybucket", fn conn -> conn |> Plug.Conn.put_resp_content_type("application/xml") |> Plug.Conn.resp(200, """ diff --git a/test/livebook_web/live/hub/edit_live_test.exs b/test/livebook_web/live/hub/edit_live_test.exs index 128ed70f4..b3e2d8de1 100644 --- a/test/livebook_web/live/hub/edit_live_test.exs +++ b/test/livebook_web/live/hub/edit_live_test.exs @@ -276,7 +276,7 @@ defmodule LivebookWeb.Hub.EditLiveTest do end defp expect_s3_listing(bypass) do - Bypass.expect_once(bypass, "GET", "/", fn conn -> + Bypass.expect_once(bypass, "GET", "/mybucket", fn conn -> conn |> Plug.Conn.put_resp_content_type("application/xml") |> Plug.Conn.resp(200, """ diff --git a/test/support/hub_helpers.ex b/test/support/hub_helpers.ex index 3c5188a2c..78a816e7b 100644 --- a/test/support/hub_helpers.ex +++ b/test/support/hub_helpers.ex @@ -105,8 +105,8 @@ defmodule Livebook.HubHelpers do erpc_call(node, :create_file_system, [[org_key: org_key]]) end - def build_bypass_file_system(bypass, hub_id \\ nil) do - bucket_url = "http://localhost:#{bypass.port}" + def build_bypass_file_system(bypass, hub_id \\ Livebook.Hubs.Personal.id()) do + bucket_url = "http://localhost:#{bypass.port}/mybucket" file_system = build(:fs_s3,