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

Update universal image to .NET 8 #855

Merged
merged 28 commits into from
Feb 1, 2024

Conversation

seesharprun
Copy link
Contributor

@seesharprun seesharprun commented Nov 16, 2023

.NET 8 is now the LTS version of .NET: https://dotnet.microsoft.com/platform/support/policy/dotnet-core#lifecycle

This PR proposes to update the universal image to install .NET 8 along with 7 and 6.

@seesharprun
Copy link
Contributor Author

Still a WIP

@seesharprun seesharprun marked this pull request as ready for review November 16, 2023 19:25
@seesharprun seesharprun requested a review from a team as a code owner November 16, 2023 19:25
@seesharprun
Copy link
Contributor Author

I'm not familiar with Oryx enough to figure out why it's failing to build in the smoke test.

@seesharprun
Copy link
Contributor Author

Here's the error message I'm seeing in the smoke test:

33.67 Build FAILED.
33.67 
33.67 /opt/tmp/oryx-repo/src/BuildScriptGenerator.Common/BuildScriptGenerator.Common.csproj : error NU1903: Warning As Error: Package 'Azure.Identity' 1.8.2 has a known high severity vulnerability, https://github.com/advisories/GHSA-5mfx-4wcx-rv27 [/opt/tmp/oryx-repo/Oryx.sln]
33.67     0 Warning(s)
33.67     1 Error(s)
33.67 
33.67 Time Elapsed 00:00:05.39
33.73 ERROR: Feature "Oryx" (ghcr.io/devcontainers/features/oryx) failed to install! Look at the documentation at https://github.com/devcontainers/features/tree/main/src/oryx for help troubleshooting this error.

If you look at the Oryx repo, they are indeed using version 1.8.2 of Azure.Identity:

https://github.com/microsoft/Oryx/blob/main/src/BuildScriptGenerator.Common/BuildScriptGenerator.Common.csproj#L19

If you look at NuGet, that version has a security vulnerability and that's being thrown as an error here and may be a warning elsewhere:

https://www.nuget.org/packages/Azure.Identity/1.8.2

@samruddhikhandale
Copy link
Member

Oryx tool needs to be built with .NET 7, however, looking at https://github.com/devcontainers/images/actions/runs/6895550673/job/18760553721?pr=855#step:3:18052, it's getting built with .NET 8. This is definitely a bug in the oryx Feature which needs to be fixed to unblock this PR.

If you look at NuGet, that version has a security vulnerability and that's being thrown as an error here and may be a warning elsewhere:

I'll reach out to the Oryx team and mention this security vulnerability. Thanks for pointing it out.
These build failures don't happen with .NET 7, hence, I wonder if .NET 8 is throwing them as errors as opposed to warnings.

@samruddhikhandale
Copy link
Member

I'll reach out to the Oryx team and mention this security vulnerability. Thanks for pointing it out.

Fix - microsoft/Oryx#2263 🎉

@seesharprun
Copy link
Contributor Author

@samruddhikhandale, did the PR from the Oryx team resolve this issue?

@samruddhikhandale
Copy link
Member

did the PR from the Oryx team resolve this issue?

It did resolve the issue we were seeing in #855 (comment)

Oryx tool needs to be built with .NET 7, however, looking at https://github.com/devcontainers/images/actions/runs/6895550673/job/18760553721?pr=855#step:3:18052, it's getting built with .NET 8. This is definitely a bug in the oryx Feature which needs to be fixed to unblock this PR.

(From #855 (comment))

However, this ^ is another issue which is currently blocking this PR. I have opened devcontainers/features#760 for tracking, but have been super busy and wasn't able to get to it. Apologies for that and thanks for your patience. I am hoping to fix devcontainers/features#760 sometime this week.

However, in case you are interested in contributing a fix for devcontainers/features#760, we'd really appreciate that. ✨

@seesharprun
Copy link
Contributor Author

seesharprun commented Nov 28, 2023

I missed that follow-up issue.

It looks like the Oryx feature auto-builds with the primary (latest?) version of .NET and we're only putting .NET 6 & 8 on the image.

Forgive me if this seems like an obvious question, but how should this be resolved:

  • Should the image be updated to have .NET 6, 7, and 8 and the feature updated to specify .NET 7?
  • Should the feature be updated to install .NET 7 specifically in isolation and use that to install the feature?
  • Should Oryx be updated to .NET 8?

Sorry, if you can't answer those questions. Oryx is still a bit of a mystery to me as I only ever encountered it with Azure App Service. I'm happy to contribute if I can, but I don't have a ton of experience with that project.

@samruddhikhandale
Copy link
Member

No worries, and thank you for your willingness to help @seesharprun, appreciate it.

Should Oryx be updated to .NET 8?

Currently, the oryx tool is built with .Net 7, they would switch it to .Net 8 sometime in the future, but I don't think it's worth waiting/ getting blocked because of it.

In the meanwhile, we can update the Oryx Feature to do the following -

  • Install .Net 7
  • Build oryx binary
  • Uninstall .Net 7

Ideally, the Oryx Feature should be doing that ^, see https://github.com/devcontainers/features/blob/main/src/oryx/install.sh#L136-L143 but maybe there is a bug? Previously, we had .Net 7 in the universal image, so most likely we didn't encounter it?

Should the image be updated to have .NET 6, 7, and 8 and the feature updated to specify .NET 7?

Nope, I don't expect changes to the image code.

Should the feature be updated to install .NET 7 specifically in isolation and use that to install the feature?

Yes, about right

Co-authored-by: Samruddhi Khandale <samruddhikhandale@github.com>
@seesharprun
Copy link
Contributor Author

Unfortunately, I can't test the image locally. I'm using Windows and it doesn't seem to let me install the local NVS feature.

@seesharprun
Copy link
Contributor Author

seesharprun commented Nov 28, 2023

This error makes me assume I need bash locally to test out the devcontainer:

image

EDIT: I'm getting the same error message trying to build the image using devcontainer up in WSL 2:

image

@seesharprun
Copy link
Contributor Author

seesharprun commented Nov 29, 2023

In the interim, would adding .NET 7 back solve the Oryx build error?

"ghcr.io/devcontainers/features/dotnet:1": {
    "version": "8.0",
    "additionalVersions": "7.0, 6.0"
},

or

"ghcr.io/devcontainers/features/dotnet:2": {
    "version": "8.0",
    "additionalVersions": "7.0, 6.0"
},

I'm going to change to the latter first option.

@seesharprun
Copy link
Contributor Author

Another update, it looks like using the older version of the feature causes an error when installing multiple versions of .NET.

I'm attempting to smoke test on my end with the latest version of the feature:

"ghcr.io/devcontainers/features/dotnet:1": {
    "version": "8.0",
    "additionalVersions": "7.0, 6.0"
},

For now, I will mark this PR as draft until there's a solution.

@seesharprun seesharprun marked this pull request as draft November 29, 2023 18:38
@timheuer
Copy link

And since we're using the mcr.microsoft.com/devcontainers/universal:2 image, we cannot run for example this sample on Codespaces: search application

To unblock, can you use a diff base image and add the notebook features you need?

@timheuer
Copy link

Related to #919

@samruddhikhandale
Copy link
Member

Update: Engaged the oryx team with microsoft/Oryx#2272 which is needed to fix the oryx version compatibility issue. Currently, they are resolving issues with their own Oryx build pipelines. Once that is fixed and validations pass on ^ the PR, they are ready to merge it in.

@seesharprun
Copy link
Contributor Author

I switched to just .NET 7 and 8 per the PR suggestions.

@samruddhikhandale
Copy link
Member

Update: Engaged the oryx team with microsoft/Oryx#2272 which is needed to fix the oryx version compatibility issue. Currently, they are resolving issues with their own Oryx build pipelines. Once that is fixed and validations pass on ^ the PR, they are ready to merge it in.

microsoft/Oryx#2272 was merged today. Hence, opened devcontainers/features#825 which unblocks this PR.

@samruddhikhandale
Copy link
Member

#855 is merged, and new Oryx Feature is released.

@seesharprun Can you help update src/universal/.devcontainer/devcontainer-lock.json ?

@seesharprun
Copy link
Contributor Author

#855 is merged, and new Oryx Feature is released.

@seesharprun Can you help update src/universal/.devcontainer/devcontainer-lock.json ?

Done:

https://github.com/seesharprun/devcontainers-images/blob/universal-dotnet-8/src/universal/.devcontainer/devcontainer-lock.json#L63-L67

@samruddhikhandale
Copy link
Member

@seesharprun Let me know if you need help with fixing the failing checks!

@seesharprun
Copy link
Contributor Author

@seesharprun Let me know if you need help with fixing the failing checks!

I can't seem to figure it out. I would appreciate the assistance. I have selected the option to allow maintainers to edit.

image

Copy link
Member

@samruddhikhandale samruddhikhandale left a comment

Choose a reason for hiding this comment

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

Looks good now, merged in changes which makes the test happy ⚡

I'll release dev tags today, and prod tags on Monday.

@samruddhikhandale samruddhikhandale merged commit 897f0a3 into devcontainers:main Feb 1, 2024
3 checks passed
@seesharprun seesharprun deleted the universal-dotnet-8 branch February 1, 2024 23:58
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.

5 participants