-
Notifications
You must be signed in to change notification settings - Fork 475
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
App model rework around resource types and connection string emission for containers #1165
Conversation
Made some changes for Postgres. Specifically, we no longer use the trust auth method either locally for for inner loop. Instead with use the scram-sha-256 password exchange method. It stops the password being transmitted via clear text (using a challenge mechanism). The connection still isn't TLS though - but its better than auth mode being trust if someone is deploying onto an internal container network. |
Co-authored-by: Eric Erhardt <[email protected]>
…into abstract-resources
@davidfowl @ellismg @prom3theu5 ... big push! Let me hear your thoughts and feedback. |
Why does the nodeapp have the full physical path to app.js, that seems broken. Are we resolving the full path and putting it in args? I'll look into that. One thing we should consider is not having containers require a custom manifest but instead using annotations. That would let the manifest writing happen without so much custom logic in each container (I worry about the amount of copy pasta required for new containers) |
It does look better now. Quite a lot of rework in aspirate for third party component support for things like Postgres and Redis now though, but hopefully the amount of data about what comprises a container in the manifest is enough to construct custom deployments for them etc The different there is when using the component type it was each to have a custom processor for that component that pulled separate templates for the components. So for redis it deployed a stateful set of a standard redis instance based on a separate template. for Postgres it wasn't a stateful set but a deployment with custom health checks etc, and the "processors" were handled by the component type. I'll make it work though, I agree this is a change for the better. |
Why? Those resources are still abstract resources that the deployment tool needs to handle. This PR just makes container resources more explanatory. |
@mitchdenny do you plan to update the templates to use |
Just because of how I implemented components is all Each component had its own processor like https://github.com/prom3theu5/aspirational-manifests/blob/main/src/Aspirate.Processors/Redis/RedisProcessor.cs I agree it simplifies a lot of that just needs a more “intelligent” processor for the container type. The way I implemented component types was with their own processors and own set of templates so the processor for redis It explicitly chose a redis template with lower resource limits set etc. now each type will just be container.v0 I can use the type to control the selection of which template. Now on a container if you change the custom image for redis to joeblogs:about-stuff there is no way to know that it's redis. Components like redis don't technically need a service as well as a deployment defining. Don't worry. I'll just produce a container processor and rework all the custom templates into one unified templated template. Worst case everything will just end up with a matching service. Besides this container type is in addition to the other resource types isn't it? |
@davidfowl up for discussion. I think it would prefer to use the non container variants. @prom3theu5 keep your existing logic for redis etc, those resource types still exist. The sample should be updated to use them. |
@davidfowl template updated. @prom3theu5 I've updated the sample AppHost to go back to using the abstract resource types (AddRedis instead of AddRedisContainer). This means the sample manifest outputs the following which should be exactly the same as what you are used to dealing with: {
"resources": {
"postgres": {
"type": "postgres.server.v0"
},
"catalogdb": {
"type": "postgres.database.v0",
"parent": "postgres"
},
"basketcache": {
"type": "redis.v0"
},
... The changes made in this PR mean that when you do something like AddRedisContainer you'll get this: {
"resources": {
"postgres": {
"type": "postgres.server.v0"
},
"catalogdb": {
"type": "postgres.database.v0",
"parent": "postgres"
},
"basketcache": {
"type": "container.v0",
"image": "redis:latest",
"bindings": {
"tcp": {
"scheme": "tcp",
"protocol": "tcp",
"transport": "tcp",
"containerPort": 6379
}
},
"connectionString": "{basketcache.bindings.tcp.host}:{basketcache.bindings.tcp.port}"
},
... Note that the |
We are already pretty late, so if you are happy to do so can we defer that to preview 3. I think we would probably need an |
Lets file a follow up issue. This needs to be backported to preview2. Also, this is a breaking change since everyone is using AddRedisContainer today 😄 |
… for containers (#1165) * Abstract resources and templated connection strings. * Add abstract redis for testing. * Moved MySql to input request model. * Postgres implementation. * Update PostgresBuilderExtensions.cs Co-authored-by: Eric Erhardt <[email protected]> * Migrate RabbitMQ to new model. * Migrate SQL to new model. * Test case updates. * Update mongo resource to the new model. * Update manfiests. * Update template. --------- Co-authored-by: Eric Erhardt <[email protected]>
@davidfowl backport PR: #1270 |
… for containers (#1165) (#1270) * Abstract resources and templated connection strings. * Add abstract redis for testing. * Moved MySql to input request model. * Postgres implementation. * Update PostgresBuilderExtensions.cs * Migrate RabbitMQ to new model. * Migrate SQL to new model. * Test case updates. * Update mongo resource to the new model. * Update manfiests. * Update template. --------- Co-authored-by: Eric Erhardt <[email protected]>
{ | ||
var sqlServerConnection = new SqlServerConnectionResource(name, connectionString); | ||
var password = Guid.NewGuid().ToString("N") + Guid.NewGuid().ToString("N").ToUpper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason to repeat the guid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason to repeat the GUID is that the default password Policy for the SQL container needs upper case and lower case characters with special cases. This is just a cheats way of using GUIDs to do that.
@mitchdenny The sql client doesn't work anymore after this PR, the password is invalid for sa when resolving SqlConnection. Or is there something else to do than |
What does your app host look like so I can repro. |
I am not sure there is a problem here, I just tested it with tests/testprogram/TestProgram.AppHost. Here was my methodology:
|
var sqlserverAbstract = AppBuilder.AddSqlServerContainer("sqlserverabstract"); | ||
var mysqlAbstract = AppBuilder.AddMySqlContainer("mysqlabstract"); | ||
var redisAbstract = AppBuilder.AddRedisContainer("redisabstract"); | ||
var postgresAbstract = AppBuilder.AddPostgresContainer("postgresabstract"); | ||
var rabbitmqAbstract = AppBuilder.AddRabbitMQContainer("rabbitmqabstract"); | ||
var mongodbAbstract = AppBuilder.AddMongoDB("mongodbabstract"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitchdenny - were these "Abstract" resources all not supposed to have Container
on them? As this stands, this code spins up 2 SQL Server containers in the exact same way. Same for MySql, Redis, Postgres, RabbitMQ.
Fixes:
#1164
#1088
This PR is a fairly major change to all the resource types built into
Aspire.Hosting
to support a model where calls likeAddRedisContainer
actually outputs a container resource to the manifest (container.v0
) with enough configuration such that it can run within a given orchestrator.It introduces three new key concepts:
connectionString
field in the manifest. When a resource connection string is referenced this field can be used.connectionString
cannot be determined at manifest publishing time it now contains placeholder values that the deployment tool is expected to process and replace as appropriate.inputs
(optional) which tells the deployment tool when it should prompt the user (or generate) inputs such as a password.The following JSON manifest is produces from
tests/testproject/TestProject.AppHost
and contains the built-in resource types that employ this model.Note that as part of this change, we have removed
AddXYZConnection(...)
methods and associated resource types as we found they added minimal value beyond just adding an environment variable. Also note that we've added a non-container version of most of the resource types (e.g.AddRedis(...)
vs.AddRedisContainer(...)
). Locally these methods do much the same thing, but when the manifest is produced the first option does not produce acontainer.v0
resource, it producesredis.v0
like it previously did.Ideally folks will use these more abstract builder methods (e.g.
AddRedis(...)
) instead of the container orientated counterpart. Deployment tools that target cloud environments should preference the use of managed services with better operational support, particularly in production deployment scenarios.Deployment tools may wish to warn users that they are using container builder methods vs. abstract resource type builder methods when deploying to production, although the containerized versions should still continue to work.