aspire icon indicating copy to clipboard operation
aspire copied to clipboard

Hosting.Keycloak: Fix `importDirectory` param validation in `WithRealmImport`

Open radical opened this issue 1 year ago • 3 comments

Description

The import path is expected to be relative to the AppHost path, but here it was being used as-is - relative to the current working directory. Instead, expand the path relative to the AppHost path and use that for Directory.Exists check.

This manifested as a failure to import the file when running the app with Aspire.Hosting.Testing which would start the apphost with cwd!=apphost .

Checklist

  • Is this feature complete?
    • [x] Yes. Ready to ship.
    • [ ] No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • [x] Yes
    • [ ] No
  • Did you add public API?
    • [ ] Yes
      • If yes, did you have an API Review for it?
        • [ ] Yes
        • [ ] No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • [ ] Yes
        • [ ] No
    • [x] No
  • Does the change make any security assumptions or guarantees?
    • [ ] Yes
      • If yes, have you done a threat model and had a security review?
        • [ ] Yes
        • [ ] No
    • [x] No
  • Does the change require an update in our Aspire docs?
    • [ ] Yes
      • Link to aspire-docs issue:
    • [x] No
Microsoft Reviewers: Open in CodeFlow

radical avatar Aug 07 '24 22:08 radical

cc @julioct

radical avatar Aug 09 '24 00:08 radical

We can drop the changes in TestingAppHost1, but then we need to add a test somewhere else. I did try adding Keycloak playground app to the tests for which /alive, and /health work fine but /weatherforecast fails with Endpoint 'http://localhost:52119/weatherforecast' for resource 'apiservice' in app 'Keycloak.AppHost' returned status code Unauthorized.

https://github.com/dotnet/aspire/blob/972f9a9128e949964249fe33b627de9ce67be964/playground/keycloak/Keycloak.ApiService/Program.cs#L28-L40

radical avatar Aug 09 '24 20:08 radical

Looks like WithBindMount does not expect the path to exist. And ContainerMountAnnotation even allows a null source path. https://github.com/dotnet/aspire/blob/ac3dbc4e0d6982ebf69b980789f9b06a90ed63c1/src/Aspire.Hosting/ApplicationModel/ContainerMountAnnotation.cs#L18-L19

I'll restore the check, and expand the path using the apphost path.

radical avatar Aug 09 '24 21:08 radical

/azp run

radical avatar Aug 12 '24 20:08 radical

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Aug 12 '24 20:08 azure-pipelines[bot]

Merge after https://github.com/dotnet/aspire/pull/5276 .

radical avatar Aug 13 '24 18:08 radical