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

docker support #367

Merged
merged 2 commits into from
Nov 12, 2019
Merged

Conversation

ChrisSzabo
Copy link
Contributor

  • Added support for building docker containers for Admin, Api, and STS projects. Based off of code from @rherlt (Feature/docker support seaear base #325).

  • This version is a little more efficient when building the images because the newer VS docker tooling generated dockerfile code that also copies project references (.csproj files) when creating the dependency cache layers.

To run:
docker-compose up --build

To stop:
docker-compose down

Api Url: http://127.0.0.1.xip.io:5000/swagger/index.html
IdentityServer Admin: http://127.0.0.1.xip.io:9000
IdentityServer : http://127.0.0.1.xip.io

@bravecobra
Copy link
Contributor

Any progress on this?

@skoruba
Copy link
Owner

skoruba commented Nov 9, 2019

Can you test this PR, please? I need help with this PR to migrate to net core 3.0. Can you help with this? Thanks

@bravecobra
Copy link
Contributor

@skoruba sure. I already gave #325 a go and that seemed to be working for me at the time, of course lacking the Admin.Api. I'll try this one as well. Looking at the changes, there won't be too much of a difference except for the obvious Admin.API addition.

@bravecobra
Copy link
Contributor

bravecobra commented Nov 9, 2019

I found only 2 minor configuration issues.

The default seed of the db did not add the proper allowed redirect URI for Swagger to work. Swagger is running at http://127.0.0.1.xip.io:5000 so it should be allowed to be redirected to that endpoint, Once I added that manually, like the screenshot below, it worked. Adding an entry (conditionally) to the seed will solve that.
image

Secondly the IdentityServerUri configuration for the API is incorrect in the docker-compose.yml file
That should be

skoruba.identityserver4.admin.api:
    environment:
          - "AdminApiConfiguration:IdentityServerBaseUrl=http://127.0.0.1.xip.io"

And not the URI of the API service itself at port 5000. The parameter describes where you should authenticate, not where the API service is running.

Once these 2 things were properly configured, everything seemed to work, including 2FA and API calls.

@bravecobra
Copy link
Contributor

@skoruba as you mentioned .NET Core 3 support, the dockerfiles will require a little update to use the proper build images. So either you merge #390 first and update this PR or vice versa. Your choice.
Those lines to be changed should then be resp. FROM mcr.microsoft.com/dotnet/core/sdk:3.0-buster AS builder and FROM mcr.microsoft.com/dotnet/core/aspnet:3.0-buster-slim. I expect that to work.

Here is a reference for building .NET Core 3.0 Docker images with Github Actions: https://hjerpbakk.com/blog/2019/10/09/asp-net-core-docker-and-github-actions

@bravecobra
Copy link
Contributor

As for reference, to get a running version IdentityServer4.Admin with SQL Server using docker compose, is now as easy as

docker-compose build
docker-compose up

We might want to add that to the documentation somewhere.

For people using ReSharper, add the **/ReSharper.Caches/ folders to the .dockerignore. docker-compose does not seem to be happy with them, but that's a minor issue and only for some.

@skoruba
Copy link
Owner

skoruba commented Nov 9, 2019

You are awesome @bravecobra - thanks for your feedback - can you send a PR with your changes?

@bravecobra
Copy link
Contributor

@skoruba that would result in yet another PR for this. I can add one, but I'm sure @ChrisSzabo is willing to add these few minor adjustments. I'll make the changes in my fork based on his branch but I suggest we give him a bit of time to add them in. I'll submit an PR otherwise.

@bravecobra
Copy link
Contributor

I tried to resolve the merge conflicts as well, but it seems there were "a lot" of configuration changes. @skoruba is that something you want to fix or should I attempt to fix that as well? That shouldn't be too hard, but it will result in changes in the docker-compose.yml as well since those values need to be overridden as environment variables.

@skoruba
Copy link
Owner

skoruba commented Nov 10, 2019

For sure @bravecobra - I will merge this PR. Can you resolve conflicts on this PR @ChrisSzabo and if you have time - it would be perfect - add the suggestions from @bravecobra. Thanks guys for your great work.
I am sorry for delay on docker stuff in general, but finally I resolved some issue with docker on my computer yesterday, so now I am able to work with docker as well and verify all things from PR. 😊👍🏼
I really appreciate your help on this, because for me is new area. Thanks.

@bravecobra
Copy link
Contributor

@skoruba will you be merging the .NET Core 3.0 branch to dev first, as that would affect this branch a b bit as well?

@skoruba
Copy link
Owner

skoruba commented Nov 10, 2019

For sure.

@skoruba
Copy link
Owner

skoruba commented Nov 10, 2019

@bravecobra - Done, merged on dev. :-)

@bravecobra
Copy link
Contributor

I took a stab at fixing those merge conflicts, and it seems you already added the IdentityRequireHttpsMetadata property as RequireHttpsMetadata. Further the Serilog sink will require an extra ENV override for the connection string.
Also the seeding works a bit differently, so my fix on my branch doesn't work anymore as the property is now read from appsettings.json. So the conflict merging require a bit more work, but I should have something cooked up later today.

@bravecobra
Copy link
Contributor

@bravecobra - Done, merged on dev. :-)

Cool, tnx. I'll work that in as well.

@skoruba
Copy link
Owner

skoruba commented Nov 10, 2019

Perfect, thanks a lot. 👍

@bravecobra
Copy link
Contributor

@skoruba I fixed the merge conflicts in #395 and merged in the latest dev branch adding extra .NET 3.0 support.

@skoruba skoruba merged commit a361c0e into skoruba:dev Nov 12, 2019
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.

3 participants