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

Add configuration to serve public routes from a different base url path #2704

Conversation

elepedus
Copy link
Contributor

Background
I've been trying to deploy Livebook in Elastic Kubernetes Service, at a non-root path (/livebook/), behind an Application Load Balancer Ingress with SSO enabled via annotations. This breaks smart cell JS loading, because /livebook/public/* can't be excluded from the authenticated routes (annotations apply to all rules in the Ingress)

It is possible to create another, unauthenticated, ingress using the same ALB group name, and the rules will get combined. Unfortunately, when combining the rules, the ALB Ingress implementation doesn't follow the official Kubernetes Ingress specification, which dictates that longer path prefix rules should get higher priority. Instead, all the rules from the secure ingress are added first, and then the ones from the public ingress are added. This means that /livebook/* authenticated route matches /livebook/public/* requests, making them require auth. I've verified that manually swapping the priority of these rules in the AWS console makes everything work, but since they're controlled by Kubernetes, they reset every time the ingress is redeployed.

Until AWS fixes their ingress implementation, we can work around this limitation by mapping the public routes to a completely different path prefix, that doesn't match the secure route.

Implementation
This PR introduces a new environment variable called LIVEBOOK_PUBLIC_BASE_URL_PATH_OVERRIDE, that overrides the base URL path only for /public/* routes.

Testing
The easiest way to test this locally is using Caddy as a reverse proxy.

# Caddyfile
:8080
handle_path /livebook/* {
    basic_auth {
    		# Username "Bob", password "hiccup"
    		Bob $2a$14$Zkx19XLiW6VYouLHR5NmfOFU0z2GTNmpkT/5qqR7hx4IjWJPDhjvG
    	}
    rewrite * {uri}
    reverse_proxy localhost:4000
 }

handle_path /livebook-no-auth/public/* {
    rewrite * /public{uri}
    reverse_proxy localhost:4000
 }
# Run livebook with the override
LIVEBOOK_BASE_URL_PATH="/livebook" LIVEBOOK_PUBLIC_BASE_URL_PATH_OVERRIDE="/livebook-no-auth" mix phx.server
# Run Caddy
caddy run

Visit localhost:8080/livebook/, create a new notebook, add a Map smart cell. Observe scripts loading correctly.

Counter-example

# Run Livebook without the public base url path override
LIVEBOOK_BASE_URL_PATH="/livebook" mix phx.server

Visit localhost:8080/livebook/, create a new notebook, add a Map smart cell. Observe error loading scripts, as the Iframe attempts to load the scripts from the authenticated base path without the correct basic auth credentials.

I appreciate this is a bit of a hack, and will gladly re-work as needed, if there's a better way of making Livebook work in this scenario :)

Copy link

github-actions bot commented Jul 11, 2024

Uffizzi Preview deployment-54092 was deleted.

lib/livebook/config.ex Outdated Show resolved Hide resolved
@elepedus elepedus force-pushed the allow_public_routes_to_be_served_from_a_different_base_url_path branch 2 times, most recently from 7806e41 to 4f7a881 Compare July 12, 2024 09:08
@elepedus elepedus marked this pull request as ready for review July 15, 2024 08:39
@elepedus elepedus force-pushed the allow_public_routes_to_be_served_from_a_different_base_url_path branch from 4f7a881 to 9f44f9e Compare July 15, 2024 14:20
README.md Outdated Show resolved Hide resolved
lib/livebook.ex Outdated Show resolved Hide resolved
Comment on lines 132 to 134
~p"/public/sessions/node/#{node_id}/assets/#{hash}/#{file_parts}"
|> String.replace(
Livebook.Config.base_url_path(),
Livebook.Config.public_base_url_path()
)
Copy link
Member

Choose a reason for hiding this comment

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

Replacing in the whole string may break in very specific cases. Let's define a function:

def public_path(path) do
  base_url_path = Livebook.Config.base_url_path()
  public_base_url_path = Livebook.Config.public_base_url_path()
  ^base_url_path <> rest = url
  public_base_url_path <> rest
end

We can put it in LivebookWeb and import in verified_routes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I don't know how to make verified_routes use the newly defined function.

I've tried removing the String.replace() call from the highlighted code, and adding this to LivebookWeb, but it doesn't seem to be doing anything

def public_path(path) do
    base_url_path = Livebook.Config.base_url_path()
    public_base_url_path = Livebook.Config.public_base_url_path()
    ^base_url_path <> rest = path
    public_base_url_path <> rest
  end

  def verified_routes do
    quote do
      use Phoenix.VerifiedRoutes,
        endpoint: LivebookWeb.Endpoint,
        router: LivebookWeb.Router,
        statics: LivebookWeb.static_paths()

      # We don't know the hostname Livebook runs on, so we don't use
      # absolute URL helpers
      import Phoenix.VerifiedRoutes, only: :sigils
      import LivebookWeb, only: [public_path: 1]
    end
  end

Could you explain what I need to do, please?

Copy link
Member

@jonatanklosko jonatanklosko Jul 16, 2024

Choose a reason for hiding this comment

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

@elepedus oh, this looks good, we still need to call it explicitly, but at least it's more convenient. So in the highlighted code, do this:

public_path(~p"/public/sessions/node/#{node_id}/assets/#{hash}/#{file_parts}")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome -- I've done that :)
Out of curiosity, would there be a way to integrate that new function with sigil_p, so we don't need to call it explicitly?

Copy link
Member

@jonatanklosko jonatanklosko Jul 17, 2024

Choose a reason for hiding this comment

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

Aaactually, we can :)

def verified_routes do
  quote do
    use Phoenix.VerifiedRoutes,
      endpoint: LivebookWeb.Endpoint,
      router: LivebookWeb.Router,
      statics: LivebookWeb.static_paths()

    # We don't know the hostname Livebook runs on, so we don't use
    # absolute URL helpers. We don't import sigil_p either, because
    # we override it.
    import Phoenix.VerifiedRoutes, only: []

    import LivebookWeb, only: [sigil_p: 2]
  end
end

# ...

# Overrides

require Phoenix.VerifiedRoutes

defmacro sigil_p({:<<>>, _meta, ["/public/" <> _ | _]} = route, extra) do
  # We allow configuring a base path for all routes and we configure
  # Phoenix to use it. However, we have an additional configuration
  # for base path applying only to /public. We use a custom sigil_p
  # to insert this base path if needed.
  quote do
    path = Phoenix.VerifiedRoutes.sigil_p(unquote(route), unquote(extra))
    LivebookWeb.__rewrite_public_base_path__(path)
  end
end

defmacro sigil_p(route, extra) do
  quote do
    Phoenix.VerifiedRoutes.sigil_p(unquote(route), unquote(extra))
  end
end

def __rewrite_public_base_path__(path) do
  base_url_path = Livebook.Config.base_url_path()
  public_base_url_path = Livebook.Config.public_base_url_path()
  ^base_url_path <> rest = path
  public_base_url_path <> rest
end

@josevalim wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks fine to me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's really cool! 😎

I'm curious, though, why do we still need import Phoenix.VerifiedRoutes, only: []?

Reading naively, it looks like we're saying "import nothing from Phoenix.VerifiedRoutes", which should be equivalent to deleting the line entirely?

Copy link
Member

Choose a reason for hiding this comment

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

use Phoenix.VerifiedRoutes imports everything (ref), so we need to unimport it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! I see! Thanks! :)

@jonatanklosko
Copy link
Member

We also need to fix the paths in image and audio input components. Search for ~p"/public in the project to make sure we got them all :)

@elepedus elepedus force-pushed the allow_public_routes_to_be_served_from_a_different_base_url_path branch from 9f44f9e to 06e0a7e Compare July 16, 2024 14:17
@elepedus
Copy link
Contributor Author

We also need to fix the paths in image and audio input components. Search for ~p"/public in the project to make sure we got them all :)

I've done this, but the relevant smart cells (Neural Network Task -> Image Classification / Speech-to-Text) seemed to work even before the change?

@elepedus elepedus force-pushed the allow_public_routes_to_be_served_from_a_different_base_url_path branch 2 times, most recently from acb4a11 to aad5688 Compare July 17, 2024 10:41
@jonatanklosko
Copy link
Member

I've done this, but the relevant smart cells (Neural Network Task -> Image Classification / Speech-to-Text) seemed to work even before the change?

In case of these inputs we use /public route as the <audio>/<img> source for previous. Those elements are directly on the page (not inside iframe), so those requests carry the auth cookies and work fine even when guarded by auth.

But I think we should apply the base path to all /public for consistency. With the overridden sigil it will happen automatically, so we are good :)

static/assets/app.js Outdated Show resolved Hide resolved
@elepedus elepedus force-pushed the allow_public_routes_to_be_served_from_a_different_base_url_path branch from aad5688 to 669c065 Compare July 17, 2024 12:52
lib/livebook_web.ex Outdated Show resolved Hide resolved
lib/livebook_web.ex Outdated Show resolved Hide resolved
Copy link
Member

@jonatanklosko jonatanklosko left a comment

Choose a reason for hiding this comment

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

Thanks :)

@jonatanklosko jonatanklosko changed the title PROPOSAL: allow public routes to be served from a different base url path Allow public routes to be served from a different base url path Jul 17, 2024
@jonatanklosko jonatanklosko changed the title Allow public routes to be served from a different base url path Add configuration to serve public routes from a different base url path Jul 17, 2024
@jonatanklosko jonatanklosko merged commit d4201c6 into livebook-dev:main Jul 17, 2024
7 checks passed
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.

4 participants