resources: Add SecurityGroup resource #97
Conversation
resources/aws/securitygroup.go
Outdated
| // Insecure apiserver | ||
| 80, | ||
| // apiserver | ||
| 443, |
There was a problem hiding this comment.
We were using 6443 in #70, and that's what I see elsewhere too. Which is it?
There was a problem hiding this comment.
Using 6443 was a temporary solution for #70 which IMO should be disregarded, but I wasn't aware that kubernetesd is using that too. Will add it.
There was a problem hiding this comment.
Done (it means, I included both 443 and 6443, just in case)
There was a problem hiding this comment.
We have these ports in the TPR, no? Does it make sense to use those?
There was a problem hiding this comment.
Not all of them, but probably everything except SSH. I can use what is available in TPR.
resources/aws/securitygroup.go
Outdated
| Description string | ||
| GroupName string | ||
| VpcID string | ||
| name string |
There was a problem hiding this comment.
Can't we call this id, since it contains the groupID?
There was a problem hiding this comment.
Then I need to create another interface, but OK, can do.
There was a problem hiding this comment.
It's not the most important thing
resources/aws/securitygroup.go
Outdated
| func (s *SecurityGroup) openPort(port int) error { | ||
| if _, err := s.Clients.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ | ||
| CidrIp: aws.String("0.0.0.0/0"), | ||
| GroupId: aws.String(s.Name()), |
There was a problem hiding this comment.
I think this should be name()
There was a problem hiding this comment.
No, name is a private field, Name is a public method.
resources/aws/securitygroup.go
Outdated
| if _, err := s.Clients.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ | ||
| CidrIp: aws.String("0.0.0.0/0"), | ||
| GroupId: aws.String(s.Name()), | ||
| IpProtocol: aws.String("tcp"), |
There was a problem hiding this comment.
I would prefer if this was capitalized.
There was a problem hiding this comment.
But it isn't and you can't do anything about that https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_AuthorizeSecurityGroupIngress.html
|
|
||
| func (s *SecurityGroup) openPort(port int) error { | ||
| if _, err := s.Clients.EC2.AuthorizeSecurityGroupIngress(&ec2.AuthorizeSecurityGroupIngressInput{ | ||
| CidrIp: aws.String("0.0.0.0/0"), |
There was a problem hiding this comment.
I think what you're doing here is allowing inbound connections on those ports from any IP. Do we want to be so permissive?
There was a problem hiding this comment.
Yes. What alse do you suggest? Maybe we could make it configurable, but IMO it's too early for that.
There was a problem hiding this comment.
I think if we leave it like this then we should definitely add an issue to remember to change it in the future.
There was a problem hiding this comment.
My gut feeling is that this is too open, but I'm not certain what would improve the situation. Issue is fine with me.
resources/aws/securitygroup.go
Outdated
| } | ||
|
|
||
| func (s *SecurityGroup) Delete() error { | ||
| return microerror.MaskAny(notImplementedMethodError) |
There was a problem hiding this comment.
Should we implement this instead? :)
nhlfr
force-pushed
the
nhlfr/security-group
branch
2 times, most recently
from
0578682
to
16fa4c3
Compare
resources/aws/securitygroup.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| portsToOpen []int = []int{ |
There was a problem hiding this comment.
I get a linter warning here saying you can omit the []int on the LHS.
nhlfr
force-pushed
the
nhlfr/security-group
branch
3 times, most recently
from
cbcef1a
to
e67002c
Compare
resources/aws/securitygroup.go
Outdated
| Description string | ||
| GroupName string | ||
| VpcID string | ||
| Cluster clustertpr.Cluster |
There was a problem hiding this comment.
I'm not so sure this type needs to have access to the whole cluster part of the TPR. I think a list of port would make more sense.
But I understand that this could be a compromise to get us running. In that case though, we should address this later and have an issue to track it.
There was a problem hiding this comment.
Where do you want to create that list of ports, in which function and module, and based on what input?
There was a problem hiding this comment.
- Where you create the struct (in our case,
service.go) - Based on the TPR
I'm just advocating for separating concerns.
There was a problem hiding this comment.
But yeah, it's not the most pressing concern, so you're also free to disregard :)
Security groups are needed both for VPCs and ELBs. Both of them will use this resource. Ref giantswarm#27 Ref giantswarm#69
nhlfr
force-pushed
the
nhlfr/security-group
branch
from
e67002c
to
31be24c
Compare
|
Please also create the issue wrt IP addresses. |
Security groups are needed both for VPCs and ELBs. Both of them will use this resource.
Ref #27
Ref #69