Allow using existing/shared Security Groups #5744

rdrgmnzs · 2018-09-03T20:25:08Z

This PR implements the ability to use an existing Security Groups for Instance Groups and Load Balancer on AWS.

This PR pulls in most of the work in #3562 #3595 #3625 while fixing a few issues. Credit for those goes to @justinsb and @chrislovecnm.

/assign @justinsb @geojaz @mikesplain

chrisz100 · 2018-09-05T20:39:03Z

What a huge thing... from going through it two times I'd say looks good, but second and third opinion YES PLEASE!
Thanks @rdrgmnzs !

/lgtm

geojaz

the PR works and lgtm technically, but I'd like to get some more ideas about how we structure this in the cluster spec.

just in case my excitement about getting this feature done wasn't apparent, I want this feature so bad and have been waiting for a long time. Let's get the details worked out and get it into 1.11 alpha

docs/security_groups.md

geojaz · 2018-09-11T14:34:49Z

pkg/apis/kops/cluster.go

+	// IdleTimeoutSeconds sets the timeout of the api loadbalancer.
+	IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"`
+	// SecurityGroup is the id of the security group to be use by the load balancer.
+	SecurityGroup *string `json:"securityGroup,omitempty"`


I need to think about the representation of this data a bit- I think we need to figure out how to have one field in these structs for SG. I wish we hadn't called the field AdditionalSecurityGroups to start with, but this is the hand we are dealt.

My suggestion is to have one field to manage all of the SG parameters that we'll end up needing. Below are just some thoughts that can definitely use some iteration/smart ideas. Please jump in and help us sort this out. any ideas @justinsb?

Would a data structure like this get us anything?

type SecurityGroupSpec struct { AdoptDefaultSG bool`json:"adoptDefaultSG,true"` AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` } // LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access type LoadBalancerAccessSpec struct { // Type of load balancer to create may Public or Internal. Type LoadBalancerType `json:"type,omitempty"` // IdleTimeoutSeconds sets the timeout of the api loadbalancer. IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"` SecurityGroupSpec SecurityGroupSpec // Deprecation in lieu of SecurirtGroupSPec to come. for now let's just copy the slice AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` // UseForInternalApi indicates wether the LB should be used by the kubelet UseForInternalApi bool `json:"useForInternalApi,omitempty"` // SSLCertificate allows you to specify the ACM cert to be used the the LB SSLCertificate string `json:"sslCertificate,omitempty"` }

Does that make life better, worse, or equivalent to a PR that's already done and works?

@geojaz as per the last Office Hour we decided to keep this as SecurityGroup correct?

geojaz · 2018-09-28T02:13:41Z

I think we decided to do either AdditonalSGs and SGOverride or AdditionalSGs and SecurityGroups. I prefer having the verbosity in the key. Justin was happy to do it either way as I remember, so pick what makes the most sense to you and let's run with it!

…

On Thu, Sep 27, 2018 at 10:27 AM Rodrigo Menezes ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/apis/kops/cluster.go <#5744 (comment)>: > @@ -311,11 +311,18 @@ const ( // LoadBalancerAccessSpec provides configuration details related to API LoadBalancer and its access type LoadBalancerAccessSpec struct { - Type LoadBalancerType `json:"type,omitempty"` - IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"` - AdditionalSecurityGroups []string `json:"additionalSecurityGroups,omitempty"` - UseForInternalApi bool `json:"useForInternalApi,omitempty"` - SSLCertificate string `json:"sslCertificate,omitempty"` + // Type of load balancer to create may Public or Internal. + Type LoadBalancerType `json:"type,omitempty"` + // IdleTimeoutSeconds sets the timeout of the api loadbalancer. + IdleTimeoutSeconds *int64 `json:"idleTimeoutSeconds,omitempty"` + // SecurityGroup is the id of the security group to be use by the load balancer. + SecurityGroup *string `json:"securityGroup,omitempty"` @geojaz <https://github.com/geojaz> as per the last Office Hour we decided to keep this as SecurityGroup correct? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5744 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJA3QHp0jPAXA_q1LXC_dhhXbh50n2Jmks5ufQqEgaJpZM4WYBcX> .

k8s-ci-robot · 2018-10-01T22:24:10Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2018-10-02T07:42:36Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Verbosely log when a user overwrites LB or IG security groups Change SecurityGroup to SecurityGroupOverride Allow using existing/shared Security Groups Update tests

rdrgmnzs · 2018-10-02T16:04:54Z

@geojaz SecurityGroupOverride it is!

justinsb

One blocker I think - the cilium removal, but otherwise looks good to me - I'm going to do a pass with some of my "I wonder if this would be cleaner" thoughts, but we can do those in follow-on PRs if they work.

pkg/model/awsmodel/api_loadbalancer.go

pkg/model/firewall.go

justinsb · 2018-10-02T07:09:32Z

pkg/model/firewall.go

-			Lifecycle:     b.Lifecycle,
-			SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleNode),
-			SourceGroup:   b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster),
+	if len(groups) == 0 {


I'm not sure this is right - what if there was one node IG using the standard SG and another node IG using an override?

Tackled in #5862

justinsb · 2018-10-02T07:10:36Z

pkg/model/firewall.go

+		name := "masters." + b.ClusterName()
+		baseGroup = &awstasks.SecurityGroup{
+			Name:        s(name),
+			Lifecycle:   lifecycle,
 			VPC:         b.LinkToVPC(),
 			Description: s("Security group for masters"),
 			RemoveExtraRules: []string{


Note to self: It's odd that RemoveExtraRules are here but not in the override...

I think I convinced myself in #5862 that it doesn't make a ton of sense for a shared (unmanaged) security group...

justinsb · 2018-10-02T15:47:35Z

docs/security_groups.md

+# Security Groups
+
+## Use existing AWS Security Groups
+**Note: Use this at your own risk, when existing SGs are used Kops will NOT ensure they are properly configured.**


I do hope we can relax this (ExistsAndWarnIfChanges should warn!), but we'll see :-)

Yep, this already happens. The issue is that this displays a compressed object on the terminal and unless you are looking for it, it can be easy to miss. Hence the note here, however I'm fine with removing the note as well.

justinsb · 2018-10-02T15:49:49Z

pkg/model/awsmodel/api_loadbalancer.go

+			sgLink = &awstasks.SecurityGroup{
+				Name:   lbSpec.SecurityGroupOverride,
+				ID:     lbSpec.SecurityGroupOverride,
+				Shared: fi.Bool(true),


I would have thought it should suffice to build it using LinkToELBSecurityGroup, but just create it with a specified ID and Shared=true. I'll take a look though...

Tackled in #5863

justinsb · 2018-10-02T15:50:37Z

pkg/model/awsmodel/api_loadbalancer.go

+		lbSG.Tags = b.CloudTags(*lbSG.Name, false)
+
+		if lbSpec.SecurityGroupOverride != nil {
+			lbSG.Name = fi.String(*lbSpec.SecurityGroupOverride)


Ah maybe this is it, because the name is significant after all

Tackled in #5863

justinsb · 2018-10-02T15:51:53Z

pkg/model/awsmodel/api_loadbalancer.go

@@ -208,19 +229,28 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 		}
 	}

+	masterGroups, err := b.GetSecurityGroups(kops.InstanceGroupRoleMaster)


I'm wondering if we should make GetSecurityGroups return a richer data structure, like

type SecurityGroupMapping struct { ID string Suffix string Groups []*InstanceGroup } func GetSecurityGroups() ([]SecurityGroupMapping, error) ...

Again ... thoughts for future refactoring though :-)

Tackled in #5862

justinsb · 2018-10-02T16:13:49Z

pkg/model/external_access.go

-				FromPort:      i64(443),
-				ToPort:        i64(443),
-				CIDR:          s(apiAccess),
+			for masterGroupName, masterGroup := range masterGroups {


Note to self for future refactoring - I'm wondering if we could have a helper function that did a security-group for each group, instead of doing a loop every time

This was not really any clearer, because we have to map the group into SecurityGroup or Source. It might be a little simpler, but not a huge win

justinsb · 2018-10-02T16:17:02Z

pkg/model/firewall.go

+				t := &awstasks.SecurityGroupRule{
+					Name:          s(fmt.Sprintf("all-nodes-to-master%s", suffix)),
+					Lifecycle:     b.Lifecycle,
+					SecurityGroup: b.LinkToSecurityGroup(kops.InstanceGroupRoleMaster),


I think we need to expand out the groups here. We can do that as a follow-on, I think

Tackled in #5862

justinsb · 2018-10-02T18:45:48Z

Looking good! I have a follow-on for some of the edge-cases here (mixing security groups in a role, mixing override and default in a role) but I'll send a separate PR for those!

/approve
/lgtm

k8s-ci-robot · 2018-10-02T18:45:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrisz100, justinsb, rdrgmnzs

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:

~~OWNERS~~ [justinsb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Follow on for #5744

machbio · 2018-10-03T18:15:36Z

Thanks @justinsb , @chrislovecnm and @rdrgmnzs for the feature - what is the milestone release version for this feature ?

k8s-ci-robot assigned geojaz, justinsb and mikesplain Sep 3, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 3, 2018

rdrgmnzs force-pushed the security_groups branch 3 times, most recently from f3914d8 to 8a404d8 Compare September 4, 2018 02:15

k8s-ci-robot assigned chrisz100 Sep 5, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018

geojaz suggested changes Sep 11, 2018

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 27, 2018

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 29, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 1, 2018

rdrgmnzs force-pushed the security_groups branch from 3e6a9a5 to c071fb9 Compare October 1, 2018 23:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 1, 2018

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. and removed cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 2, 2018

Allow using existing/shared Security Groups

a82f548

Verbosely log when a user overwrites LB or IG security groups Change SecurityGroup to SecurityGroupOverride Allow using existing/shared Security Groups Update tests

rdrgmnzs force-pushed the security_groups branch from ece6266 to a82f548 Compare October 2, 2018 07:51

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 2, 2018

Update tests

4cd8dba

justinsb reviewed Oct 2, 2018

View reviewed changes

Fix blocker

87eec75

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2018

k8s-ci-robot merged commit c97ad57 into kubernetes:master Oct 2, 2018

This was referenced Oct 2, 2018

WIP: ELB custom SG #3562

Closed

Shared security group implementation for masters and nodes #3625

Closed

adding API to have shared security groups #3595

Closed

k8s-ci-robot added a commit that referenced this pull request Oct 3, 2018

Merge pull request #5862 from justinsb/follow_on_5744

3fe0287

Follow on for #5744

This was referenced Dec 10, 2018

Need to use shared security groups #3124

Closed

Specify Security Groups while NOT generating the built-in ones. #3126

Closed

bzuelke mentioned this pull request Jan 2, 2019

Ability to change default master/nodes security groups #5564

Closed

ZachEddy mentioned this pull request Feb 21, 2019

Kops keeps removing SecurityGroupRules from explicitly self-managed security groups #6333

Closed

rdrgmnzs deleted the security_groups branch August 9, 2019 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using existing/shared Security Groups #5744

Allow using existing/shared Security Groups #5744

rdrgmnzs commented Sep 3, 2018

chrisz100 commented Sep 5, 2018

geojaz left a comment •

edited

Loading

geojaz Sep 11, 2018

rdrgmnzs Sep 27, 2018

geojaz commented Sep 28, 2018 via email

k8s-ci-robot commented Oct 1, 2018

k8s-ci-robot commented Oct 2, 2018

rdrgmnzs commented Oct 2, 2018

justinsb left a comment

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

rdrgmnzs Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018 •

edited

Loading

justinsb Oct 2, 2018

justinsb Oct 2, 2018 •

edited

Loading

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb Oct 2, 2018

justinsb commented Oct 2, 2018

k8s-ci-robot commented Oct 2, 2018

machbio commented Oct 3, 2018

Allow using existing/shared Security Groups #5744

Allow using existing/shared Security Groups #5744

Conversation

rdrgmnzs commented Sep 3, 2018

chrisz100 commented Sep 5, 2018

geojaz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geojaz commented Sep 28, 2018 via email

k8s-ci-robot commented Oct 1, 2018

k8s-ci-robot commented Oct 2, 2018

rdrgmnzs commented Oct 2, 2018

justinsb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb commented Oct 2, 2018

k8s-ci-robot commented Oct 2, 2018

machbio commented Oct 3, 2018

geojaz left a comment •

edited

Loading

justinsb Oct 2, 2018 •

edited

Loading

justinsb Oct 2, 2018 •

edited

Loading