sasa1977 / site_encrypt

Integrated certification via Let's encrypt for Elixir-powered sites
MIT License
470 stars 34 forks source link

Phoenix.Endpoint.init is deprecated as of 1.7.11 #58

Closed Hermanverschooten closed 6 months ago

Hermanverschooten commented 7 months ago

I just upgraded one of my app from phoenix 1.7.10 to 1.7.11 and got a deprecation warning on the init/2 callback.

Compiling 1 file (.ex)
    warning: got "@impl Phoenix.Endpoint" for function init/2 but this behaviour does not specify such callback. The known callbacks are:

      * Phoenix.Endpoint.broadcast/3 (function)
      * Phoenix.Endpoint.broadcast!/3 (function)
      * Phoenix.Endpoint.broadcast_from/4 (function)
      * Phoenix.Endpoint.broadcast_from!/4 (function)
      * Plug.call/2 (function)
      * SiteEncrypt.certification/0 (function)
      * Phoenix.Endpoint.config/2 (function)
      * Phoenix.Endpoint.config_change/2 (function)
      * SiteEncrypt.handle_new_cert/0 (function)
      * Phoenix.Endpoint.host/0 (function)
      * Plug.init/1 (function)
      * Phoenix.Endpoint.local_broadcast/3 (function)
      * Phoenix.Endpoint.local_broadcast_from/4 (function)
      * Phoenix.Endpoint.path/1 (function)
      * Phoenix.Endpoint.script_name/0 (function)
      * Phoenix.Endpoint.server_info/1 (function)
      * Phoenix.Endpoint.start_link/1 (function)
      * Phoenix.Endpoint.static_integrity/1 (function)
      * Phoenix.Endpoint.static_lookup/1 (function)
      * Phoenix.Endpoint.static_path/1 (function)
      * Phoenix.Endpoint.static_url/0 (function)
      * Phoenix.Endpoint.struct_url/0 (function)
      * Phoenix.Endpoint.subscribe/2 (function)
      * Phoenix.Endpoint.unsubscribe/1 (function)
      * Phoenix.Endpoint.url/0 (function)

    │
 76 │   def init(_key, config) do
    │   ~~~~~~~~~~~~~~~~~~~~~~~~~
    │
    └─ lib/admin_web/endpoint.ex:76: AdminWeb.Endpoint (module)

warning: AdminWeb.Endpoint.init/2 is deprecated, use config/runtime.exs instead or pass additional options when starting the endpoint in your supervision tree
  (phoenix 1.7.11) lib/phoenix/endpoint/supervisor.ex:36: Phoenix.Endpoint.Supervisor.init/1
  (stdlib 5.2) supervisor.erl:330: :supervisor.init/1
  (stdlib 5.2) gen_server.erl:980: :gen_server.init_it/2
  (stdlib 5.2) gen_server.erl:935: :gen_server.init_it/6
  (stdlib 5.2) proc_lib.erl:241: :proc_lib.init_p_do_apply/3

There is nothing in the changelog for this. Currently it is a warning, but ....

Hermanverschooten commented 7 months ago

Can the adding of the https info happen where the endpoint is started?

sasa1977 commented 7 months ago

Thanks for the report! This is now replaced with {MyEndpoint, endpoint_opts} (https://github.com/phoenixframework/phoenix/commit/55395900f39a56a0431575f0f88eb89dd24c7a46), which means we need to change the way configure_https is used. I'm not even sure that this helper macro makes sense anymore.

The code of configure_https (https://github.com/sasa1977/site_encrypt/blob/65d98aad30d5604f7aab78c6c37cda708e3df146/lib/site_encrypt/phoenix.ex#L46-L68) is just a convenience for adding http keys config . We could offload this responsibility to the client code. The required https settings can still be obtained with SiteEncrypt.http_keys(endpoint_module).

Alternatively, perhaps use SiteEncrypt.Phoenix could generate child_spec/1 which does this automatically.

Hermanverschooten commented 7 months ago

Am I correct the real start of the endpoint is in Adapter.start_all_children!/1?
Could we then not add it there?

sasa1977 commented 7 months ago

That's a nice idea. However I don't think it will work because we need to read the base config (from app env), so we can figure out which adapter is used (cowboy or phoenix). For the same reason I'm not sure that generating child_spec/1 would work, but I might be wrong there, because that function would reside in the endpoint module, so we might be able to get to the base config. But even so I'm not sure it's worth it.

However, there is another problem. The current implementation doesn't support passing args to the endpoint, so we need to fix that and support children = [{SiteEncrypt.Phoenix, {MyEndpoint, endpoint_opts}}, ...].

Hermanverschooten commented 7 months ago

I haven't checked but what is inside the adapter_config that start_all_children!/1 gets in its first line, was kind a hoping that that would be the config.

sasa1977 commented 7 months ago

It isn't, it's the adapter config, not the endpoint config. We only have the latter in init/1. With the new approach we need to fetch that with Application.get_env(otp_app, endpoint_module, []). And then we need to provide the https config which somehow needs to be merged with the client's custom config. I actually think we could automate this with use SiteEncrypt.Phoenix, otp_app: my_app. But TBH I'm not convinced it's worth it.

Hermanverschooten commented 7 months ago

In SiteEncrypt.Phoenix.start_link/1 we get the module, couldn't we get the adapter there using apply(endpoint, :config, [:adapter]) and then pass that into the underlying.

Hermanverschooten commented 7 months ago

No we cannot, as that only works once phoenix has booted.

sasa1977 commented 7 months ago

Yeah, was about to write that :-)

sasa1977 commented 7 months ago

I've pushed a test version to the remove-init branch. It's a bit rough around the edges, especially docs, but the gist of it is:

  1. In your endpoint, replace use Phoenix.Endpoint, otp_app: :my_app and use SiteEncrypt.Phoenix with use SiteEncrypt.Phoenix, otp_app: :my_app
  2. In supervisor children, replace the child {SiteEncrypt.Phoenix, MyEndpoint} with MyEndpoint or {MyEndpoint, endpoint_opts}.

This should be enough. You can see the example in the demo app:

You can give it a try if you're curious, but don't fully switch, because I'll probably make some minor tweaks in the interface. But the general idea should remain: the client code does use Some.SiteEncrypt.Module, and the rest automagically works :-)

Feedback is welcome.

Hermanverschooten commented 7 months ago

I had to use use SiteEncrypt.Phoenix.Endpoint, otp_app: :my_app and remove the init/2. I like it that there is no longer a need to adjust the supervisor. In :dev it works, but for :prod the app crashes with:

{"message":"Error starting the child :site: {:shutdown, {:failed_to_start_child, {AdminWeb.Endpoint, :https}, {:EXIT, {%RuntimeError{message: \"Plug.SSL.configure/1 encountered error: missing option :key/:keyfile\"}, [{Bandit, :start_link, 1, [file: ~c\"lib/bandit.ex\", line: 262, error_info: %{module: Exception}]}, {:supervisor, :do_start_child_i, 3, [file: ~c\"supervisor.erl\", line: 420]}, {:supervisor, :do_start_child, 2, [file: ~c\"supervisor.erl\", line: 406]}, {:supervisor, :\"-start_children/2-fun-0-\", 3, [file: ~c\"supervisor.erl\", line: 390]}, {:supervisor, :children_map, 4, [file: ~c\"supervisor.erl\", line: 1258]}, {:supervisor, :init_children, 2, [file: ~c\"supervisor.erl\", line: 350]}, {:gen_server, :init_it, 2, [file: ~c\"gen_server.erl\", line: 980]}, {:gen_server, :init_it, 6, [file: ~c\"gen_server.erl\", line: 935]}]}}}}","time":"2024-02-12T08:39:09.781Z","severity":"ERROR"}
sasa1977 commented 7 months ago

In :dev it works, but for :prod the app crashes with:

I've updated the demo project (in the demos/phoenix folder) to use the latest Phoenix and switch to bandit, and can't reproduce this. It works fine in both dev and prod. My first suspect would be that bandit adapter is configured after the keys are initialized. How do you configure the adapter?

sasa1977 commented 6 months ago

@Hermanverschooten did you get a chance to try this out? You mentioned some bug, but I didn't see this on the demo project.

Hermanverschooten commented 6 months ago

Hey @sasa1977 sorry about the delay.

this is what I changed:

diff --git a/lib/admin/application.ex b/lib/admin/application.ex
index 91e8643..86a7861 100644
--- a/lib/admin/application.ex
+++ b/lib/admin/application.ex
@@ -22,7 +22,7 @@ defmodule Admin.Application do
       # Task supervisor
       {Task.Supervisor, name: Admin.TaskSupervisor},
       # Start the Endpoint (http/https)
-      {SiteEncrypt.Phoenix, AdminWeb.Endpoint},
+      AdminWeb.Endpoint,
       # Start the Quantum Scheduler
       Admin.Scheduler
     ]
diff --git a/lib/admin_web/endpoint.ex b/lib/admin_web/endpoint.ex
index a6a3dd1..05cb3b7 100644
--- a/lib/admin_web/endpoint.ex
+++ b/lib/admin_web/endpoint.ex
@@ -1,7 +1,6 @@
 defmodule AdminWeb.Endpoint do
   use Sentry.PlugCapture
-  use Phoenix.Endpoint, otp_app: :admin
-  use SiteEncrypt.Phoenix
+  use SiteEncrypt.Phoenix.Endpoint, otp_app: :admin

   @moduledoc false

@@ -17,7 +16,9 @@ defmodule AdminWeb.Endpoint do

   socket "/live", Phoenix.LiveView.Socket, websocket: [connect_info: [session: @session_options]]

-  if Mix.env() == :prod, do: plug(LoggerJSON.Plug)
+  # if Mix.env() == :prod, do: plug(LoggerJSON.Plug)
+
+  plug SiteEncrypt.AcmeChallenge, __MODULE__

   # Serve at "/" the static files from "priv/static" directory.
   #
@@ -71,8 +72,4 @@ defmodule AdminWeb.Endpoint do
         end
     )
   end
-
-  def init(_key, config) do
-    {:ok, SiteEncrypt.Phoenix.configure_https(config)}
-  end
 end
diff --git a/mix.exs b/mix.exs
index df7dc7e..cb954be 100644
--- a/mix.exs
+++ b/mix.exs
@@ -53,7 +53,7 @@ defmodule Admin.MixProject do
       {:tailwind, "~> 0.1", runtime: Mix.env() == :dev},
       {:argon2_elixir, "~> 4.0"},
       {:logger_json, "~> 5.0"},
-      {:site_encrypt, github: "sasa1977/site_encrypt"},
+      {:site_encrypt, github: "sasa1977/site_encrypt", branch: "remove-init"},
       {:timex, "~> 3.7"},
       {:nimble_csv, "~> 1.2"},
       {:number, "~> 1.0"},

and this is what I get when I start the app in production:

Mar 20 14:25:45 admin systemd[1]: Started Herman's admin.
Mar 20 14:25:50 admin admin[646772]: {"message":"Running AdminWeb.Endpoint with Bandit 1.3.0 at :::80 (http)","time":"2024-03-20T14:25:50.045Z","severity":"INFO"}
Mar 20 14:25:50 admin admin[646772]: {"message":"Error starting the child :site: {:shutdown, {:failed_to_start_child, {AdminWeb.Endpoint, :https}, {:EXIT, {%RuntimeError{message: \"Plug.SSL.configure/1 encountered error: missing option :key/:keyfile\"}, [{Bandit, :start_link, 1, [file: ~c\"lib/bandit.ex\", line: 272, error_info: %{module: Exception}]}, {:supervisor, :do_start_child_i, 3, [file: ~c\"supervisor.erl\", line: 420]}, {:supervisor, :do_start_child, 2, [file: ~c\"supervisor.erl\", line: 406]}, {:supervisor, :\"-start_children/2-fun-0-\", 3, [file: ~c\"supervisor.erl\", line: 390]}, {:supervisor, :children_map, 4, [file: ~c\"supervisor.erl\", line: 1258]}, {:supervisor, :init_children, 2, [file: ~c\"supervisor.erl\", line: 350]}, {:gen_server, :init_it, 2, [file: ~c\"gen_server.erl\", line: 980]}, {:gen_server, :init_it, 6, [file: ~c\"gen_server.erl\", line: 935]}]}}}}","time":"2024-03-20T14:25:50.067Z","severity":"ERROR"}
Mar 20 14:25:50 admin admin[646772]: {"message":"Application admin exited: Admin.Application.start(:normal, []) returned an error: shutdown: failed to start child: SiteEncrypt.Phoenix.Endpoint\n    ** (EXIT) :start_error","time":"2024-03-20T14:25:50.077Z","severity":"INFO"}
Hermanverschooten commented 6 months ago

Oke, I found the reason. I had some thousand_island_options set in config/runtime.exs and apparently they clash with the new way of working. The options were:

    https: [
      ip: {0, 0, 0, 0, 0, 0, 0, 0},
      cipher_suite: :strong,
      thousand_island_options: [
         transport_options: [
           :inet6,
           secure_renegotiate: true,
           reuse_sessions: true,
           log_level: :warning
         ]
       ],
      port: 443
sasa1977 commented 6 months ago

I'll need to investigate this further before I can say with certainty, but this looks to me like a bug somewhere in Phoenix or Bandit. If you omit :inet6, it works. If you list only :inet6 it doesn't work. If you replace :inet6 with :foo it also fails. Which indicates to me that some config merge deeper inside fails because transport options is not a kw list.

sasa1977 commented 6 months ago

So, this is definitely not a bug of site_encrypt. Basically, when deep merging two configs (the one from .exs and the one passed via start_link), the merge silently fails because one value is not a kw list (because of the :inet6 element). What's funny is that the failure is silent, so the error cause is not obvious.

I'll submit this to Phoenix, but I'm not sure that they can completely fix it, but rather raise an error in this case. I think a separate proposal should be issued to bandit to support the :net option, similarly to Plug.Cowboy.

Hermanverschooten commented 6 months ago

I sent @mtrudel as message on slack referencing this issue.

sasa1977 commented 6 months ago

I created the phoenix issue: https://github.com/phoenixframework/phoenix/issues/5758

sasa1977 commented 6 months ago

This is resolved with cb5477ad87881c41e0006859668a5ac6dcf2f454 Merged and pushed the new version to 0.6.0. Changelog.