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

NodeJs app in GitHub Codespaces missing environment variable with hyphen #6680

Open
1 task done
rudiv opened this issue Nov 14, 2024 · 12 comments
Open
1 task done

NodeJs app in GitHub Codespaces missing environment variable with hyphen #6680

rudiv opened this issue Nov 14, 2024 · 12 comments
Assignees
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication documentation Improvements or additions to documentation
Milestone

Comments

@rudiv
Copy link

rudiv commented Nov 14, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug

We have a fairly simple Nodejs app spun up alongside SQL Server, Redis, Azure Storage and a .NET API. This works locally (across Windows, Linux and OSX developers), however does not work in GitHub Codespaces.

Our Redis cache is called "project-cache", which in turn tries to set the environment variable ConnectionStrings__project-cache. I've also set 2 manual environment variables to confirm the root cause:

Image

Of the 3, only ConnectionStrings__project_cache_test is set.

On Codespaces using the default environment, it seems that DCP is not sending the Environment Variables correctly which causes this to be dropped. As it's a black box I'm unable to see exactly where or why this would happen. It seems the use of env may be required depending on the shell, but I don't understand why the NodeJs acts differently from the .NET project.

The only solution currently is to remove the hyphen from the resource name within the AppHost.

I know that this is a "Codespaces thing" but it seems that Environment Variables should be reliably set across all platforms / shells, which I'm assuming means that DCP isn't quoting them / using env specifically in the NodeJS application type, as these Environment Variables are correctly passed to the .NET application.

Expected Behavior

Environment variables with hyphens in the name are set correctly in a NodeJs app.

Steps To Reproduce

Add an Npm app to the AppHost with an environment variable containing a dash.

builder.AddNpmApp("anything", "path/to/anything", "dev")
    .WithEnvironment("Something-Has-A-Hypen", "Anything");

Run Aspire within Github Codespaces, note that the environment variable is not set.

Exceptions (if any)

No response

.NET Version info

.NET SDK:
Version: 9.0.100
Commit: 59db016f11
Workload version: 9.0.100-manifests.3068a692
MSBuild version: 17.12.7+5b8665660

Runtime Environment:
OS Name: ubuntu
OS Version: 20.04
OS Platform: Linux
RID: linux-x64
Base Path: /usr/share/dotnet/sdk/9.0.100/

.NET workloads installed:
There are no installed workloads to display.
Configured to use loose manifests when installing new manifests.

Host:
Version: 9.0.0
Architecture: x64
Commit: 9d5a6a9aa4

.NET SDKs installed:
8.0.403 [/usr/share/dotnet/sdk]
9.0.100 [/usr/share/dotnet/sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 7.0.20 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 8.0.10 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 9.0.0 [/usr/share/dotnet/shared/Microsoft.AspNetCore.App]
Microsoft.NETCore.App 7.0.20 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 8.0.10 [/usr/share/dotnet/shared/Microsoft.NETCore.App]
Microsoft.NETCore.App 9.0.0 [/usr/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
None

Environment variables:
DOTNET_ROOT [/usr/share/dotnet]

global.json file:
Not found

Anything else?

Seems to be a DCP issue. This is running through VS Code with the C# Dev Kit installed on GitHub Codespaces.

@davidfowl
Copy link
Member

Wild guess that linux doesn't support environment variables with - in the name 😄

@rudiv
Copy link
Author

rudiv commented Nov 15, 2024

Yup, that's not the point of the post though.

Are the .NET projects not also passing configuration through environment variables? If so, it works fine there, just not for the NodeJS apps. It also works on a Fedora 40 machine, but not in Codespaces (which I'm guessing is an Ubuntu image) that I've tested so far. I'm guessing it's shell dependent.

A workaround is to use env with a quoted string to set "unusual" environment variables which would let this be set correctly and ensure compatibility.

env 'ConnectionStrings__project-cache=localhost:63999' npm run dev works for example.

The hyphens being allowed in names I'm assuming is for Bicep, but as it's not compatible with environment variables being set in the way that they are, it may be worth at the very least restricting the names to not include hyphens there thus ensuring compatibility?

@dbreshears dbreshears added this to the Backlog milestone Nov 18, 2024
@karolz-ms
Copy link
Member

The only "safe" characters in environment variable names are uppercase letters, digits, and underscore https://pubs.opengroup.org/onlinepubs/009695399/basedefs/xbd_chap08.html Anything else is implementation-dependent and may or may not be supported. Hyphen in particular is quite problematic as it is used for invocation flags, and different shells on different platforms may parse the command line differently, depending on how things are quoted, whether single- or double hyphens are used for flags etc. etc.

@rudiv would it be OK for you to work around this issue by avoiding the hyphen in env var names?

@rudiv
Copy link
Author

rudiv commented Nov 18, 2024

@karolz-ms yes it's not me that's adding the hyphens, it's aspire based on the resource name. We've obviously just changed the resource names to not have them for now.

My point is that there is a discrepancy between the .NET app and the NodeJS app, assuming that they're both using environment variables (which is what the Aspire Dashboard would imply). The .NET app works, the NodeJS app does not work.

It's my opinion that Aspire should be aware of this and map the environment variables correctly in one way or another, either by calling env on platforms where it's available (less compatible), or adjusting the names to be globally compatible by removing these "special" characters cross platform, and adjusting the internal connection strings/mappings to suit.

@davidfowl
Copy link
Member

Of it can just fail...

@rudiv
Copy link
Author

rudiv commented Nov 18, 2024

@davidfowl so when code is written on Windows or MacOS (where the hyphens work) and you deploy to a Linux environment which would attempt to use environment variables, it should just fail? Or a development team that uses different OSes shouldn't be able to collaborate because a simple / innocuous resource name causes a complete failure cross platform? The latter of which is the actual use case here.

Am I just disallowed from making suggestions on this repo?

@davidfowl
Copy link
Member

davidfowl commented Nov 18, 2024

Within reason, platforms try to mask the differences as much as possible, it's our job to figure out what is within reason. Sometimes we are able to do it well if it is done at the right level (e.g. maybe we can do it specifically for AddNpmApp). If you told me we should make it work generally across OSes, that'd be a harder pill to swallow (.NET doesn't even solve this today).

@rudiv
Copy link
Author

rudiv commented Nov 18, 2024

I fully agree, and I'm just trying to get across that this doesn't work in a specific scenario (.AddNpmApp()) whilst still working in others (.AddProject<>).

To me, that sounds like there's a difference in some internals somewhere (DCP?) that is doing things differently between resource types.

With that said, if the resolution to this is just a note in the documentation that states that resource names with hyphens, whilst allowed by Aspire, may not work across platforms (e.g. linux) with a suggestion to perhaps use RuntimeInformation.IsOSPlatform to work around it or just try to avoid it that would probably suffice. The main issue is that this is undocumented / unexpected behaviour.

@dbreshears dbreshears removed this from the Backlog milestone Nov 18, 2024
@dbreshears dbreshears added the untriaged New issue has not been triaged label Nov 18, 2024
@mitchdenny mitchdenny added the documentation Improvements or additions to documentation label Nov 19, 2024
@mitchdenny
Copy link
Member

mitchdenny commented Nov 19, 2024

This might be another good one for an analyzer. Somewhat amusing because we debated at length how strict we should be on resource names :)

@joperezr
Copy link
Member

[Triage] - we need to do more investigation here to see what breaks so that we can determine whether or not an analyzer is appropriate.

@joperezr joperezr added this to the Backlog milestone Nov 20, 2024
@joperezr joperezr added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed untriaged New issue has not been triaged area-orchestrator labels Nov 20, 2024
@mitchdenny
Copy link
Member

@rudiv we took a second look at this. We have codespaces enabled in the dotnet/aspire repo so I was able to test your scenario with the bits on main and we found that it was working fine (you could have a resource called cache-with-hyphen, reference it from a Node app, and access that environment variable in the Node code.

Given this I think the next step is having a closer look at your exact Codespaces setup. Is it possible for you to setup a Codespace which replicates the issue you are seeing so that we can take a look?

@mitchdenny mitchdenny added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 20, 2024
@rudiv
Copy link
Author

rudiv commented Nov 20, 2024

Sure, here's a minimal-ish sample. Sorry it's hastily thrown together.

Spinning up a default Codespaces instance on that, running Aspire and hitting port 3000 will show that there's no environment variables set with hyphens (there should be 2).

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

6 participants