-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
adding API to have shared security groups #3595
adding API to have shared security groups #3595
Conversation
This works for me 👍 We already have AdditionalSecurityGroups on the InstanceGroup, so we've already accepted the AWS-orientated naming. |
/retest |
@justinsb it flaked btw |
@justinsb ptal |
This exactly fits my use cases 👍 |
API LGTM here. My concern is that I'd like to see an implementation, just in case, but this LGTM! |
pkg/apis/kops/cluster.go
Outdated
AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` | ||
// SecurityGroup is the id of the shared security group to use for the InstanceGroupSpec | ||
// This API is currently under development. | ||
SecurityGroup *string `json:"securityGroup,omitempty"` |
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.
Looks good, but I think the naming or comment could potentially be reworded so it's clearer. Maybe (if I have understood this correctly):
// SecurityGroupID is the ID of a pre-existing security group to use on the resource. This ID
// will be referenced in rules within the InstanceGroupSpec Security Groups.
// This API is currently under development.
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.
Wordsmithing would be awesome
f8f09db
to
967a472
Compare
Going to mark for hold, we can merge at the same time as the implementation |
@chrislovecnm PR needs rebase |
967a472
to
41f6444
Compare
@chrislovecnm PR needs rebase |
41f6444
to
3a69972
Compare
3a69972
to
ad7f297
Compare
/assign @KashifSaadat PTAL - I have some items that are not done yet #4212, but I would like to follow-up with other PRs |
Got a bug with the instance profile ... yay, but this is ready for a look |
pkg/apis/kops/cluster.go
Outdated
IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"` | ||
// AdditionalSecurityGroups attaches additional security groups (e.g. sg-123456). | ||
AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` | ||
// SecurityGroup is the id of the shared security group to use for the InstanceGroupSpec. |
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.
Comment copy-pasted from InstanceGroup?
pkg/apis/kops/v1alpha1/cluster.go
Outdated
IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"` | ||
// AdditionalSecurityGroups attaches additional security groups (e.g. sg-123456). | ||
AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` | ||
// SecurityGroup is the id of the shared security group to use for the InstanceGroupSpec. |
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.
Update comment here (same applies to main & v1alpha2), applied to the Kube API Load Balancer (ELB).
One minor comment, otherwise LGTM. :) |
ad7f297
to
8b28089
Compare
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrislovecnm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
8b28089
to
cb67bc1
Compare
b0f1510
to
de9711e
Compare
de9711e
to
50aca53
Compare
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
This functionality is in #5744, closing |
@justinsb @KashifSaadat @geojaz
Let me know what you think