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

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

Closed
Hermanverschooten opened this issue Feb 8, 2024 · 20 comments
Closed

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

Hermanverschooten opened this issue Feb 8, 2024 · 20 comments

Comments

@Hermanverschooten
Copy link
Contributor

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)76def 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
Copy link
Contributor Author

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

@sasa1977
Copy link
Owner

sasa1977 commented Feb 8, 2024

Thanks for the report! This is now replaced with {MyEndpoint, endpoint_opts} (phoenixframework/phoenix@5539590), 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 (

defmacro configure_https(config, https_opts \\ []) do
quote bind_quoted: [config: config, https_opts: https_opts] do
# Default to cowboy
adapter = Keyword.get(config, :adapter, Phoenix.Endpoint.Cowboy2Adapter)
https_config =
case adapter do
Phoenix.Endpoint.Cowboy2Adapter ->
(Keyword.get(config, :https) || [])
|> Config.Reader.merge(https_opts)
|> Config.Reader.merge(SiteEncrypt.https_keys(__MODULE__))
Bandit.PhoenixAdapter ->
(Keyword.get(config, :https) || [])
|> Config.Reader.merge(https_opts)
|> Config.Reader.merge(
thousand_island_options: [transport_options: SiteEncrypt.https_keys(__MODULE__)]
)
end
Keyword.put(config, :https, https_config)
end
end
) 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
Copy link
Contributor Author

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

@sasa1977
Copy link
Owner

sasa1977 commented Feb 8, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

sasa1977 commented Feb 8, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@sasa1977
Copy link
Owner

sasa1977 commented Feb 8, 2024

Yeah, was about to write that :-)

@sasa1977
Copy link
Owner

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:

  • step 1 (note: this example uses the :endpoint_opts option, but you don't need to provide this if the opts are provided in app config)
  • step 2

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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

@sasa1977
Copy link
Owner

I created the phoenix issue: phoenixframework/phoenix#5758

@sasa1977
Copy link
Owner

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

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

No branches or pull requests

2 participants