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 flex to utoipa-swagger-ui build #845

Merged
merged 5 commits into from
Jan 23, 2024
Merged

Conversation

oscar6echo
Copy link
Contributor

@oscar6echo oscar6echo commented Jan 13, 2024

This is meant to widen config options, while keeping defaults unchanged.

Resolves #844

@oscar6echo
Copy link
Contributor Author

Btw I think that the overwrite folder captured in env var SWAGGER_UI_OVERWRITE_FOLDER , now a path, could also be a repo url. This would make it simpler to use in a organisation.

I'll adapt build.rs in my PR accordingly.

Copy link
Owner

@juhaku juhaku left a comment

Choose a reason for hiding this comment

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

Very good, perhaps the the Swagger UI version could be increased along with this PR to the latest available.

Also one thing that should be done is for the download need to support proxy host and port settings. E.g. in some enterprice environments the traffic needs to be proxied through a server and in such a case those unlucky people would feel sad because they cannot build their project due failing build. Or alternatively it could still provide the "vendored" version inside.

@@ -34,3 +34,6 @@ rustdoc-args = ["--cfg", "doc_cfg"]
[build-dependencies]
zip = { version = "0.6", default-features = false, features = ["deflate"] }
regex = "1.7"
reqwest = { version = "0.11", features = ["blocking"] }
openssl = { version = "0.10", features = ["vendored"] }
Copy link
Owner

Choose a reason for hiding this comment

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

Is the openssl needed? I don't see any usecases?

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 think reqwest requires openssl (reqwest -> native-tls -> openssl) but then it should pull it so no need to add it to Cargo.tom. I removed it . Cf. commit.

NB: I came to install openssl vendored because I had problem with my SSL apt package on Ubuntu, then dnf on RHEL8 but these installs - I think these issues were independent - anyway a utoipa user can always import openssl vendored in their app if they want/need.

@@ -34,3 +34,6 @@ rustdoc-args = ["--cfg", "doc_cfg"]
[build-dependencies]
zip = { version = "0.6", default-features = false, features = ["deflate"] }
regex = "1.7"
reqwest = { version = "0.11", features = ["blocking"] }
openssl = { version = "0.10", features = ["vendored"] }
anyhow = "1.0"
Copy link
Owner

Choose a reason for hiding this comment

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

Probably overkill to add anyhow error for just one error, instead I would maybe just return Box<&dyn Error> it is only a build code and there is necessity to know what the error is. 👇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I full agree. I changed it. Cf. commit.

@oscar6echo
Copy link
Contributor Author

Btw I think that the overwrite folder captured in env var SWAGGER_UI_OVERWRITE_FOLDER , now a path, could also be a repo url. This would make it simpler to use in a organisation.

I'll adapt build.rs in my PR accordingly.

On second thought, and after some practice, I believe this is both unecessary complexity and less rigorous.
In a corp env, it is as simple to pull the required folder & files and snapshot them in the api repo along side the rest of the code.

@oscar6echo
Copy link
Contributor Author

Very good, perhaps the the Swagger UI version could be increased along with this PR to the latest available.

Yes indeed. I changed it to 5.11.0. Cf. commit.

@oscar6echo
Copy link
Contributor Author

oscar6echo commented Jan 23, 2024

Also one thing that should be done is for the download need to support proxy host and port settings. E.g. in some enterprice environments the traffic needs to be proxied through a server and in such a case those unlucky people would feel sad because they cannot build their project due failing build. Or alternatively it could still provide the "vendored" version inside.

Well that's what I thought too at the beginning. But in fact, a user behind a corp firewall will need internet access beyond utoipa, for any cargo install/build. So they must have access to the outside world so no need to hold the swagger-ui .zip in the crate.

It could also be that they have a special corp version of it which they host on their network. Even a standalone user can manually get the .zip from the internet and host it themselves on say an enterprise github. Besides it keeps the build.rs simple: just one case.

Actually I have tested it from a corp network.
Here is a Dockerfile for a sample todo app.
Proxy env variables are indeed necessary for the build process.

############ chef ############
FROM clux/muslrust:stable AS chef

COPY ./crt/corp-all/corp-cert.crt /usr/local/share/ca-certificates/
RUN cat /usr/local/share/ca-certificates/corp-cert.crt >> /etc/ssl/certs/ca-certificates.crt

RUN cargo install cargo-chef 

WORKDIR /app


############ planner ############
FROM chef as planner

COPY . .
RUN cargo chef prepare  --recipe-path recipe.json


############ builder ############
FROM chef as builder

COPY --from=planner /app/recipe.json recipe.json
# copy local dependencies - containing utoipa and other corp crates
COPY ./dep ./dep
# build dependencies - caching Docker layer!
RUN cargo chef cook --release --target x86_64-unknown-linux-musl --recipe-path recipe.json


# build app
ENV SWAGGER_UI_DOWNLOAD_URL="https://github.com/swagger-api/swagger-ui/archive/refs/tags/v5.6.2.zip"
ENV SWAGGER_UI_OVERWRITE_FOLDER="/app/swagger-ui-overwrite"
ENV CORP_CONNECT_ENV="PRD"

COPY ./Cargo.toml ./Cargo.toml
COPY ./src ./src
COPY ./swagger-ui-overwrite ./swagger-ui-overwrite

# cross compile
RUN cargo build --release --target x86_64-unknown-linux-musl


############ runner ############
FROM alpine:3.19 AS runner

COPY ./crt/corp-all/corp-cert.crt /usr/local/share/ca-certificates/
RUN cat /usr/local/share/ca-certificates/corp-cert.crt >> /etc/ssl/certs/ca-certificates.crt

RUN apk update && rm -rf /var/cache/apk/*

ENV USER=phuser
RUN addgroup -S $USER && adduser -S $USER -G $USER

COPY --from=builder /app/target/x86_64-unknown-linux-musl/release/todo-axum /usr/local/bin/
COPY ./crt ./crt

# ENV RUST_LOG="todo-axum=debug,info"

USER $USER
ENTRYPOINT ["/usr/local/bin/todo-axum"]

EXPOSE 8080/tcp

Commands:

# build needs proxy env variables
docker build --build-arg HTTP_PROXY=$HTTP_PROXY --build-arg HTTPS_PROXY=$HTTPS_PROXY  -t todo-axum:alp .

# run
docker run --rm --name todo-axum-alp -p 8499:8499 --init todo-axum:alp

@juhaku
Copy link
Owner

juhaku commented Jan 23, 2024

Nice, in that case we can have this like this and make further changes in future if needed. Thanks for your contribution 👍

utoipa-swagger-ui/README.md Outdated Show resolved Hide resolved
Update Swagger UI default version in REAMDE.md to the latest
@juhaku juhaku merged commit 776f753 into juhaku:master Jan 23, 2024
5 checks passed
nightkr added a commit to stackabletech/stackable-cockpit that referenced this pull request May 13, 2024
@nightkr
Copy link

nightkr commented May 13, 2024

This PR also breaks sandboxed builds that deny network access, sadly.

Nix builds typically have no external network access, instead letting Nix download files itself and injecting them into the build. Typically you'd do this with a tool like crate2nix, but that is only aware of Cargo dependencies, not external downloads like this.

IMO, the ideal would be to either:

  1. Add a vendored feature (this could pull in a separate utoipa-swagger-ui-vendored crate to avoid bloating the main crate archive),
  2. Make SWAGGER_UI_DOWNLOAD_URL support file:// URLs as well, or
  3. Add a separate SWAGGER_UI_ZIP=/path/to/the/zip environment variable

github-merge-queue bot pushed a commit to stackabletech/stackable-cockpit that referenced this pull request May 13, 2024
* Bump Rust dependencies

* Bump Go dependencies

* Bump Node dependencies

* Update Go to 1.21

* Use setup-go action in workflows

* niv update

* Remove overlays for dropped Babel plugins

* Add override for utoipa-swagger-ui trying to download UI bundle

Caused by juhaku/utoipa#845

* Update Cargo.toml

Co-authored-by: Nick <NickLarsenNZ@users.noreply.github.com>

* Mark go.sum and yarn.lock as generated

* Update default.nix

Co-authored-by: Nick <NickLarsenNZ@users.noreply.github.com>

---------

Co-authored-by: Natalie Klestrup Röijezon <nat@nullable.se>
Co-authored-by: Nick <NickLarsenNZ@users.noreply.github.com>
@juhaku
Copy link
Owner

juhaku commented May 13, 2024

@nightkr Sure could be added. Any of the options is good for me. Though if vendored is provided then there is always a need to update the package. But it shouldn't be a too big of an issue.

How I thought that the SWAGGER_UI_OVERWRITE_FOLDER build time environment variable should have provided similar functionality but seems like it did not. 😄

@oscar6echo
Copy link
Contributor Author

How I thought that the SWAGGER_UI_OVERWRITE_FOLDER build time environment variable should have provided similar functionality but seems like it did not. 😄

SWAGGER_UI_OVERWRITE_FOLDER is meant to allow custom overwriting of swagger-ui files extracted from the swagger.zip itself, e.g. that could be a custom index.html to overwrite swagger-ui-5.17.3/dist/index.html.

Nix builds typically have no external network access, instead letting Nix download files itself and injecting them into the build. Typically you'd do this with a tool like crate2nix, but that is only aware of Cargo dependencies, not external downloads like this.

Ok, I did not know nix and this kind of constraints.


I think fixes /2 and 3/ are better than 1/ - if I may opine. For simplicity re doc, probably 2/ ?
@juhaku you agree I will try amend build.rs and doc accordingly in the next few days.

@juhaku
Copy link
Owner

juhaku commented May 14, 2024

I think fixes /2 and 3/ are better than 1/ - if I may opine. For simplicity re doc, probably 2/ ?
@juhaku you agree I will try amend build.rs and doc accordingly in the next few days.

Sure that is less work for me.

@oscar6echo
Copy link
Contributor Author

Pls check new PR #923 which implements 2/.

@oscar6echo
Copy link
Contributor Author

Now that PR #923 is merged @nightkr pls check this works for your use case.

@nightkr
Copy link

nightkr commented May 14, 2024

Yep, this seems to build fine now, thanks! I have a few minor nits on the implementation, but the overall idea looks fine to me.

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.

Add flexibility to utoipa-swagger-ui build process
3 participants