-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
r/ecs_service: Add NetworkConfiguration #2299
Conversation
ae5992c#diff-51a2b869d60207ffb83c18f0daef501dR56
|
Add |
ae5992c
to
703a792
Compare
any idea when this could be merged ? thanks for the quick PR |
@radeksimko @Ninir It'd be great if you could review this PR:bow: |
This conflicts with #2483 so please review in advance:bow: @radeksimko @Ninir |
The error message at https://github.com/atsushi-ishibashi/terraform-provider-aws/blob/703a7927ac3f2135845f248896b5a96fe4888df5/aws/resource_aws_ecs_task_definition.go#L126 is still missing // Current
errors = append(errors, fmt.Errorf("ECS Task Definition network_mode %q is invalid, must be `bridge`, `host` or `none`", value))
// Correct
errors = append(errors, fmt.Errorf("ECS Task Definition network_mode %q is invalid, must be `awsvpc`, `bridge`, `host` or `none`", value)) |
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.
I left you some comments there, nothing too critical. The most important ones are about TypeList vs TypeSet.
I'm interested in getting this merged asap as there's high community interest in Fargate support, so I'd like to unblock that other PR. Let me know if you need any help here.
aws/resource_aws_ecs_service.go
Outdated
@@ -353,9 +374,36 @@ func resourceAwsEcsServiceRead(d *schema.ResourceData, meta interface{}) error { | |||
log.Printf("[ERR] Error setting placement_constraints for (%s): %s", d.Id(), err) | |||
} | |||
|
|||
if err := d.Set("network_configuration", flattenEcsNetworkConfigration(service.NetworkConfiguration)); err != nil { | |||
log.Printf("[ERR] Error setting network_configuration for (%s): %s", d.Id(), err) |
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.
Any reason why we should silently fail here instead of returning error?
@@ -118,6 +118,7 @@ func validateAwsEcsTaskDefinitionNetworkMode(v interface{}, k string) (ws []stri | |||
validTypes := map[string]struct{}{ | |||
"bridge": {}, | |||
"host": {}, | |||
"awsvpc": {}, | |||
"none": {}, |
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.
It's not necessarily a blocker for this PR, but I'm pondering with an idea of removing this validation completely as theoretically the list may grow again (knowing how complex and big the container networking "market" is). What do you think?
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.
In my opinion, it's better to prevent error as early as possible. When constructing state is better than when requesting API.
So I disagree with you.
Of course, I understood your concern but it won't release new network mode as soon as modifying codes coudn't follow it.
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.
My opinion comes from users' perspective but you have to consider maintainability more than me...
Finally I respect your decision👍
aws/resource_aws_ecs_service.go
Outdated
Elem: &schema.Resource{ | ||
Schema: map[string]*schema.Schema{ | ||
"security_groups": { | ||
Type: schema.TypeList, |
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.
As the ordering of SGs is not significant shouldn't this field be TypeSet
to avoid potential spurious diffs in case the list of SGs comes back in a different order from the API?
aws/resource_aws_ecs_service.go
Outdated
Elem: &schema.Schema{Type: schema.TypeString}, | ||
}, | ||
"subnets": { | ||
Type: schema.TypeList, |
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.
As the ordering of subnets is not significant shouldn't this field be TypeSet
to avoid potential spurious diffs in case the list of subnets comes back in a different order from the API?
@radeksimko Ok👍
|
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.
Thanks, LGTM. 👍
thanks @radeksimko and @atsushi-ishibashi |
`network_configuration` support the following: | ||
* `subnets` - (Required) The subnets associated with the task or service. | ||
* `security_groups` - (Optional) The security groups associated with the task or service. If you do not specify a security group, the default security group for the VPC is used. | ||
For more information, see [Task Networking](http://docs.aws.amazon.com/AmazonECS/latest/developerguidetask-networking.html) |
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 link is wrong, correct url is:
https://docs.aws.amazon.com/AmazonECS/latest/developerguide/task-networking.html
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 documentation has already been fixed in #2694
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.
ok, but it is still wrong, in fact, the link is https.
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.
@ozbillwang Sure. Thanks!
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Required: #2295
Closes #2294