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

[WIP] Create documentation and samples for the Secret Store configuration provider #262

Closed
wants to merge 1 commit into from

Conversation

cmendible
Copy link
Member

@cmendible cmendible commented Mar 19, 2020

Description

Create documentation and samples for the Secret Store configuration provider

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #256

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

Copy link
Member Author

@cmendible cmendible left a comment

Choose a reason for hiding this comment

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

@amanbha I also have thee dockerfile and the manifest files to deploy this sample to K8s, should I add them under a kubernetes folder as part of the project?

@amanbha
Copy link
Contributor

amanbha commented Mar 19, 2020

@amanbha I also have thee dockerfile and the manifest files to deploy this sample to K8s, should I add them under a kubernetes folder as part of the project?

Yes adding them to a k8s folder is fine.

@cmendible cmendible force-pushed the issue-256 branch 3 times, most recently from 49ba5cd to c92ebff Compare March 27, 2020 09:11
// Open the application port to trick Dapr into thinking it's ready
var ipadress = IPAddress.Parse(localhost);
var server = new TcpListener(ipadress, 80);
server.Start();
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good overall, few more questions: Is this server start still needed because you added a endpoint "/secret" to it, it can just use the WebHostBuilder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with the endpoint we cannot relay on WebHostBuilder cause using the configuration provider means that the app will attempt to load the secrets (through Dapr) before it's ready to listen (app port is not open yet in this stage).

If we do not open the port, Dapr keeps waiting for the app which also keeps waiting for Dapr and we hit a dead lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not too convinced on this workaround, Can it lazy load the secrets when needed and retry on exception. Starting this another server doesnt seem like the right thing to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, if I add a retry loop for the secrets, the app will keep waiting forever.

On the other hand I'm not sure how or if it's possible to implement lazy loading of configuration. Any ideas here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rynowak Do you have any suggestion here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok so if in the timer we do:

...
catch
{
	if (reloadCount > 5)
	{
		System.Threading.ThreadPool.QueueUserWorkItem(_ => { throw ex; });
	}
	reloadTimer.Start();
}
...

the exception bubbles up to the main thread as needed.

But the thing that keeps bothering me about this solution is that any secret needed in the ConfigureServices or Configure methods of the Startup class will not be available (i.e. a database connection string or password needed to register EF).

With the code as-is, based on the WaitForDapr method, users will not face the issue mentioned above.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmendible I see, but I don't think that starting a new server is the right thing to do. Perhaps the way secrets are implemented, they are not the right candidate here to integrate with configuration provider and apps can query the secrets on demand when needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@amanbha do you think we can explore the possibility of the secret api becoming available to the app container before dapr starts listening external calls? In ASP.NET Core we usually need connection strings and other secrets In the ConfigureServices of the Startup class. cc @yaron2

Copy link
Contributor

Choose a reason for hiding this comment

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

@amanbha do you think we can explore the possibility of the secret api becoming available to the app container before dapr starts listening external calls? In ASP.NET Core we usually need connection strings and other secrets In the ConfigureServices of the Startup class. cc @yaron2

Yes, that could be a possibility. @yaron2 whats your opninion on making secret api available in dapr sidecar before user app is ready?

Copy link
Contributor

Choose a reason for hiding this comment

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

@cmendible I have opened a proposal for this in dapr repo dapr/dapr#1493

@amanbha
Copy link
Contributor

amanbha commented Jul 22, 2020

Closing this PR for now based on this discussion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create documentation and samples for the Secret Store configuration provider implementation
2 participants