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

[release/9.1] Ensure EH and SB emulator files can be used from non-root containers #7765

Open
wants to merge 8 commits into
base: release/9.1
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Feb 25, 2025

Backport of #7764 to release/9.1

/cc @sebastienros

Customer Impact

Event Hubs emulator (and potentially Service Bus emulator) will fail on Linux if the default Aspire Store location is not patched.

This PR sets the configuration file mode with the correct permissions so that the non-root container can access the file.

Testing

  • Functional test
  • Unit test

Risk

  • Low, it's adding new permission on Linux hosts

Regression?

  • New feature in 9.1. Potentially breaking the EH/SB emulators on Linux

@davidfowl davidfowl closed this Feb 25, 2025
@davidfowl davidfowl reopened this Feb 25, 2025
@sebastienros sebastienros changed the title [release/9.1] Ensure containers EH and SB files can be used from non-root containers [release/9.1] Ensure EH and SB emulator files can be used from non-root containers Feb 25, 2025
@davidfowl davidfowl closed this Feb 25, 2025
@davidfowl
Copy link
Member

We decided to punt this for now until we hear more feedback

@sebastienros sebastienros deleted the backport/pr-7764-to-release/9.1 branch February 26, 2025 17:44
@danmoseley danmoseley restored the backport/pr-7764-to-release/9.1 branch February 28, 2025 22:42
@danmoseley danmoseley reopened this Feb 28, 2025
@danmoseley
Copy link
Member

cherrypicked in 0cc7553 also

we want to build a 9.1.1 of this. it crosses several packages, I assume we only update the changed ones.

@sebastienros
Copy link
Member

sebastienros commented Feb 28, 2025

We don't want this PR. I made more changes in main that are better, including with SQL Server. All good

@wtgodbe
Copy link
Member

wtgodbe commented Mar 1, 2025

Pushed a commit to update branding to 9.1.1-servicing

@danmoseley
Copy link
Member

Blocked on ack from 1P team.

@github-actions github-actions bot added the area-integrations Issues pertaining to Aspire Integrations packages label Mar 10, 2025
@sebastienros
Copy link
Member

Please don't merge yet, looking into a regression on windows after the change in Sql Server. @danmoseley @wtgodbe

@danmoseley danmoseley added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 11, 2025
@sebastienros
Copy link
Member

sebastienros commented Mar 11, 2025

I pushed a fix that will work on windows and linux until I get an official guidance from the SQL team. This also makes the code behave the same way it was on 9.1 with Windows.

public static IResourceBuilder<SqlServerServerResource> WithDataBindMount(this IResourceBuilder<SqlServerServerResource> builder, string source, bool isReadOnly = false)
{
ArgumentNullException.ThrowIfNull(builder);
ArgumentException.ThrowIfNullOrEmpty(source);

// c.f. https://learn.microsoft.com/sql/linux/sql-server-linux-docker-container-configure?view=sql-server-ver15&pivots=cs1-bash#mount-a-host-directory-as-data-volume
Copy link
Member

Choose a reason for hiding this comment

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

@sebastienros - What do you think about not porting this change at all for release/9.1?

I think the risk here isn't worth it. Sql emulator in 9.1 currently "works" as it did in 9.0 right? So we are only adding risk here. I think we should keep this change targeted to the broken scenarios.

@ncelisabet
Copy link

I'm sorry if this is the wrong place to comment for a user, but I'm waiting eagerly for a fix for #7764
I currently have copy pasted code in a very ugly manner to make it work somewhat locally, but I can't run EventHub emulator due to its file permissions issue (I'm on ubuntu). I heard I might get around this issue by running Rider as root, but that is not ideal (and I get 1000 other small issues with that workaround).

The error I get is:

Unhandled exception. System.UnauthorizedAccessException: Access to the path '/Eventhubs_Emulator/ConfigFiles/Config.json' is denied.

@sebastienros
Copy link
Member

@ncelisabet

You can mitigate the issue with this code:

public static IResourceBuilder<AzureEventHubsResource> FixUnixFilePermissions(this IResourceBuilder<AzureEventHubsResource> builder)
{
    builder.ApplicationBuilder.Eventing.Subscribe<BeforeStartEvent>((@event, ct) =>
    {
        const string EmulatorConfigJsonPath = "/Eventhubs_Emulator/ConfigFiles/Config.json";

        var mountAnnotation = builder.Resource.Annotations.OfType<ContainerMountAnnotation>().FirstOrDefault(v => v.Target == EmulatorConfigJsonPath);

        if (mountAnnotation is null)
        {
            return Task.CompletedTask;
        }

        const UnixFileMode fileMode644 = UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.GroupRead | UnixFileMode.OtherRead;

        if (!OperatingSystem.IsWindows())
        {
            File.SetUnixFileMode(mountAnnotation.Source!, fileMode644);
        }

        return Task.CompletedTask;
    });

    return builder;
}

And invoke it "after" calling AddHub()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants