Azure / service-fabric-mesh-preview

Service Fabric Mesh is the Service Fabric's serverless offering to enable developers to deploy containerized applications without managing infrastructure. Service Fabric Mesh , aka project “SeaBreeze” is currently available in private preview. This repository will be used for tracking bugs/feature requests as GitHub issues and for maintaining the latest documentation.
MIT License
82 stars 12 forks source link

Linux Environment variables with '=' are not passed correctly #264

Closed tomkerkhove closed 4 years ago

tomkerkhove commented 6 years ago

Disclaimer: This is initial thinking based on what I've seen but it might be a side-effect

I was running my tomkerkhove/containo sample on Service Fabric Mesh which uses Azure Storage but suddenly it started throwing the following exceptions:

FormatException: Settings must be of the form "name=value". Microsoft.WindowsAzure.Storage.CloudStorageAccount+<>c.b__97_0(string err)

Analysis

When running this locally with the same environment variables everything went fine so I had a feeling that the environment variables were not passed correctly.

When checking the service configuration with az mesh service show --name Containo-Orders-API --app-name Containo-Orders --resource-group containo-apps it shows me this: image

The service is deployed correctly and the connection string is valid.

Given I cannot get the code package logs (#263) nor get replica list (#115) I changed my code to throw an explicit exception which track the connection string that was read. (code | (docker image))

When running this adapted code it shows me that the following environment variable was used:

ApplicationException: Connection string - DefaultEndpointsProtocol Containo.Services.Orders.Storage.Repositories.OrdersRepository..ctor() in OrdersRepository.cs, line 27

Repro

It is documented here how you can deploy everything - https://github.com/tomkerkhove/containo/tree/master/deploy#deploying-containo-on-azure-service-fabric-mesh

Given the limitation of parameters and the nature of the issue I suggest to use this template which uses a temporary docker image which catches the exception and shows the connection string when calling http://40.118.7.65:1337/api/foo/orders/bar

tomkerkhove commented 6 years ago

In order to fully isolate everything, I've updated the troubleshoot image with an environment variable controller which just reads the value and gives it to the client.

Deployed this new version with the same template which you can try it here: http://51.145.135.185:1337/api/environment/TableStorage_ConnectionString

I will roll the secret afterwards so don't bother :)

tomkerkhove commented 6 years ago

Tagging @ChackDan as this is a pretty crucial bug at the moment

ChackDan commented 6 years ago

@BharatNarasimman and @VipulM-MSFT Could you please follow up on this

mattrowmsft commented 6 years ago

FYI I think we have identified issue. B\c of where the bug is in the stack it will probably take 1-2 weeks to get fix rolled out. If it's possible in your specific scenario to split the env variable value around the '=' character that could work for now.

tomkerkhove commented 6 years ago

Thanks for having a look - I'll wait until the fix is there and test it then. Feel free to let me know if I can help any further!

ChackDan commented 6 years ago

Thankyou Tom. Will do .

From: Tom Kerkhove notifications@github.com Sent: Sunday, August 26, 2018 3:41 AM To: Azure/service-fabric-mesh-preview service-fabric-mesh-preview@noreply.github.com Cc: Chacko Daniel chackdan@microsoft.com; Mention mention@noreply.github.com Subject: Re: [Azure/service-fabric-mesh-preview] Environment variables are not passed correctly (#264)

Thanks for having a look - I'll wait until the fix is there and test it then. Feel free to let me know if I can help any further!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fservice-fabric-mesh-preview%2Fissues%2F264%23issuecomment-416029140&data=02%7C01%7CChackDan%40microsoft.com%7C1be1cc646b534802e22408d60b405926%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636708768342069958&sdata=ygQMKUVph7qaYpYiZyIqOXLMH2KoENVtSdXmJIkLe1w%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAKq9ptMW3IxUJ08ZoefxYB9ifJaJAQhiks5uUnsfgaJpZM4WBGZ7&data=02%7C01%7CChackDan%40microsoft.com%7C1be1cc646b534802e22408d60b405926%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636708768342079966&sdata=46xFnffe8%2BNl%2Baklg1VchhQLb933WnsZqnCYtK3G6ME%3D&reserved=0.

tomkerkhove commented 5 years ago

Is there an update on this?

mattrowmsft commented 5 years ago

The next major update will switch Linux container runtime and we have verified that it doesn't have same issues. Tune in to Ignite.

tomkerkhove commented 5 years ago

I've tried this again and seem to be hitting the same issue. Is there an update on this @mattrowmsft?

tomkerkhove commented 5 years ago

@ChackDan Was 6.4 already released or should I wait a bit longer?

vipul-modi commented 5 years ago

@tomkerkhove - The clear containers runtime have the bug. We were hoping to switch to Kata containers but ran in to other issues. As a workaround you can do URL safe base64 encoding of the value before passing it. In the upcoming release of the Mesh users can pass the secrets through settings.

tomkerkhove commented 5 years ago

Thanks for the update, what is the estimated date for this fix?

The workaround could work but would still be good to have a fix for this as it will not always be a secret nor is passing base64 values ideal.

tomkerkhove commented 5 years ago

I can confirm that this has not been fixed with the refresh. Any ETA for this @ChackDan @VipulM-MSFT ?

tomkerkhove commented 5 years ago

Bumping - What's the status here?

mattrowmsft commented 5 years ago

I don't think there will be any switch to Kata runtime at this point solely for the reason of fixing this bug. For now I think Vipul's suggestions above are the best workarounds. It's a bit of a pain for black box containers where you can change logic to read from file (via settings) or decode base64 environment string. Eventually we would be moving away from Clear container runtime (think summer), but presently Kata is seen as too small of a step to invest in support.