Allow additional SGs to be added to API loadbalancer #4036

almariah · 2017-12-11T14:30:38Z

Allow adding precreated additional security groups to the API loadbalancer using cluster spec:

spec:
  api:
    loadBalancer:
      type: Public
      additionalSecurityGroups:
      - sg-exampleid3
      - sg-exampleid4

Adding additionalSecurityGroups cluster spec
Adding validation for repeated security groups
Adding validation for API loadbalancer security groups
Integration test for API loadbalancer and its security groups
Update API docs and cluster.spec docs

k8s-ci-robot · 2017-12-11T14:30:41Z

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://github.com/kubernetes/kubernetes/wiki/CLA-FAQ 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: [email protected]

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.

almariah · 2017-12-11T14:44:10Z

I have signed CLA

almariah · 2017-12-11T14:52:20Z

/assign @andrewsykim

almariah · 2017-12-11T16:53:11Z

@gambol99 Can you check this pull request, or do I have to find specific reviewer?

chrislovecnm

Thanks for the PR, looks good overal. We are trying to not add anymore cli flags A integration test and updating the docs would be awesome. You can follow up with a pr branches on this for the docs, as we need to start moving towards using the api server generated docs.

Again thanks!

chrislovecnm · 2017-12-11T18:37:21Z

pkg/model/awsmodel/api_loadbalancer.go

+				ID:     fi.String(id),
+				Shared: fi.Bool(true),
+			}
+			if err := c.EnsureTask(t); err != nil {


The should be add task not ensure.

@chrislovecnm The reason for EnsureTask is the iteration over the list of security groups. If you added more security groups later and kept the old one, the iteration will be on the whole list and the will raise error for already added security groups if add task is used. EnsureTask will ignore already added security groups or tasks.

It is used also used in autoscalinggroup https://github.com/kubernetes/kops/blob/master/pkg/model/awsmodel/autoscalinggroup.go#L97-L107

I understand now. Excellent

chrislovecnm · 2017-12-11T18:38:09Z

cmd/kops/create_cluster.go

@@ -311,6 +314,8 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {

 	cmd.Flags().StringVar(&options.APILoadBalancerType, "api-loadbalancer-type", options.APILoadBalancerType, "Sets the API loadbalancer type to either 'public' or 'internal'")

+	cmd.Flags().StringSliceVar(&options.APILoadBalancerSecurityGroups, "api-loadbalancer-security-groups", options.APILoadBalancerSecurityGroups, "Add precreated additional security groups to the API loadbalancer.")


I am really trying to not get anymore cli flags and move people toward manifests. Thoughts?

@chrislovecnm You are right. I think having too much cli flags is sometimes annoying, but having flexibility to use them simplify some operations. If you create the cluster on demand or some other tool try to create the cluster, that would be more easier. In our case we have a jenkins job create the cluster using the cli completely. We were able to manage the whole deployment using the cli without editing or modifying any manifest.

If you have a long running and static setup, the manifests are better in that case. But, if your setup is dynamic and automated, then the cli is better.

Generally, in kops and kubernetes have the ability to configure and operate the cluster using cli and manifests is a great feature.

Generally, in kops and kubernetes have the ability to configure and operate the cluster using cli and manifests is a great feature.

Yes until you get too many cli options. We already have too many. I love the PR, but another CLI command line is making more of a huge number already ... we do have the override cli option if you want to sneak it in there. I am trying to find the darn thing. @justinsb where is the cli option to allow dynamic yaml via cli?

Let me chew on it, I may be ok since it will basically be parity with the other sec group options, but as a whole, I am trying to not have anymore.

We were able to manage the whole deployment using the cli without editing or modifying any manifest

I would recommend that you start using templated manifests anyways. We have dry-run now to build manifest, and templating to do crazy cool stuff with go templating.

Best practice with my clients is to use YAML. You will soon outgrow the cli. Trust me :)

Again thanks

@chrislovecnm Thanks for your explanation. I found the kops cluster templating using toolbox. It is interesting. I mean instead of doing such templating manually using sed or any other tool, using kops toolbox will be more better.

I will remove the cli arg and restore the cmd gen-cli-docs.

chrislovecnm · 2017-12-11T18:40:39Z

/ok-to-test

chrislovecnm · 2017-12-11T18:41:28Z

cmd/kops/create_cluster.go

@@ -942,6 +947,9 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e
 			}
 		}
 	}
+	if len(c.APILoadBalancerSecurityGroups) > 0 && cluster.Spec.API.LoadBalancer == nil {
+		return fmt.Errorf("unable to add security groups to nonexistent API loadbalancer")


This error handing should be moved into validate.go

@chrislovecnm This error handing is not for validating the cluster spec, it is for cli flags. If you by mistake addd APILoadBalancerSecurityGroups and there is no loadbalancer, it raises the error.

Your thought about a different file for validation is correct. If you run kops validate cluster, the error handing should be moved into validate.go. But this for kops create cluster with some non-logical or conflicting cli flags.

Was not clear enough https://github.com/kubernetes/kops/blob/master/pkg/apis/kops/validation/validation.go

If we choose to have the cli option, we can have it here, but most likely we will either use the option that allows YAML, but we do need validation.go to be updated with the API machinery validation.

After removing the cli option, we don't need such error handing. The security groups will be a part of the loadbalancer part of the cluster spec.

The part that is not clear for me is why do we need validation.go to be updated with the API machinery validation! Could you explain?

Thanks for your time!

We do need it in the api validation so we don’t get stuff like empty sg group names ;)

The api machinery validation does the same stuff like the cli validation you wrote, but at an api level. The plan is to eventually have kops at a controller, and we need more validation at the api level, and less at the cli level. It eventually ends up at the api anyways. The api is the golang representation of the cluster manifest/spec. If you want to do a separate PR that is fine as well.

chrislovecnm · 2017-12-11T23:04:04Z

/assign

chrislovecnm · 2017-12-11T23:05:24Z

Also, I will request integration tests. Update this one please https://github.com/kubernetes/kops/tree/master/tests/integration/update_cluster/complex

The test go that runs it in is the cmd/kops directory.

This reverts commit 60a90bf.

almariah · 2017-12-13T01:10:03Z

@chrislovecnm Could you review the pull request? I updated most of the issues we have discussed. A task list has been added for more details.

Thanks!

chrislovecnm · 2017-12-13T17:50:24Z

pkg/apis/kops/v1alpha1/cluster.go

@@ -263,8 +263,9 @@ const (
 )

 type LoadBalancerAccessSpec struct {


One last nitpick ... should have mentioned it earlier. Can we get some code level docs? Once we generate the api server docs they will show up like: https://github.com/kubernetes/kops/blob/master/docs/apireference/build/index.html ~ we need to get that hosted.

Here is a good guide, https://blog.golang.org/godoc-documenting-go-code, but I assume that you are an experienced gopher.

Docs here and in cluster would be AWESOME!!

chrislovecnm · 2017-12-13T17:50:45Z

One last thing ... then lgtm

almariah · 2017-12-14T02:14:07Z

@chrislovecnm The API docs has been updated.

justinsb · 2017-12-14T03:25:58Z

Awesome contribution @almariah - thank you!

/lgtm

If you do want to add CLI capabiltiy, --override is a nice mechanism to do that, that keeps us close to the API. So the user-facing syntax would be --override cluster.spec.api.loadBalancer.additionalSecurityGroups=sg-12345 --override cluster.spec.api.loadBalancer.additionalSecurityGroups=sg-67890 or maybe --override cluster.spec.api.loadBalancer.additionalSecurityGroups=sg-12345,sg-67890

Sadly I haven't yet found a nice language / library that lets us specify our API objects in this way, so currently it's behind a feature flag & I've started hard-coding the values:
https://github.com/kubernetes/kops/blob/master/cmd/kops/create_cluster.go#L1200

I figure once we've hard-coded a dozen values we can see how they "feel" and if there's a nice library/language we can use.

k8s-ci-robot · 2017-12-14T03:26:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [justinsb]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-12-14T09:25:45Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-12-14T10:25:40Z

Automatic merge from submit-queue.

Allow additional SGs to be added to API loadbalancer

1dbc606

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2017

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 Dec 11, 2017

Allow additional SGs to be added to API loadbalancer

60a90bf

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2017

k8s-ci-robot assigned andrewsykim Dec 11, 2017

chrislovecnm reviewed Dec 11, 2017

View reviewed changes

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 11, 2017

chrislovecnm reviewed Dec 11, 2017

View reviewed changes

k8s-ci-robot assigned chrislovecnm Dec 11, 2017

Revert "gen-cli-docs"

6e76277

This reverts commit 60a90bf.

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 12, 2017

almariah added 5 commits December 12, 2017 11:20

Remove --api-loadbalancer-security-groups cli arg

e1ccc33

awsValidateAdditionalSecurityGroups for API loadbalancer

5a46228

awsValidateAdditionalSecurityGroups for API loadbalancer

d181ecf

update_cluster complex integration test

16ed898

Update kubernetes.tf for API loadbalancer

22194c9

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 13, 2017

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 13, 2017

Update cluster.spec docs

2746b88

chrislovecnm reviewed Dec 13, 2017

View reviewed changes

almariah added 2 commits December 14, 2017 03:09

Add API docs

5b4c9e2

Add API docs

4b0aa1d

k8s-ci-robot assigned justinsb Dec 14, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 14, 2017

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2017

k8s-github-robot merged commit d533714 into kubernetes:master Dec 14, 2017

almariah deleted the feature-api-elb-security-groups branch December 14, 2017 11:01

eschneider1271 mentioned this pull request Feb 12, 2018

"additionalSecurityGroups" field not recognized in cluster config #4430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow additional SGs to be added to API loadbalancer #4036

Allow additional SGs to be added to API loadbalancer #4036

almariah commented Dec 11, 2017 •

edited

Loading

k8s-ci-robot commented Dec 11, 2017

almariah commented Dec 11, 2017

almariah commented Dec 11, 2017

almariah commented Dec 11, 2017

chrislovecnm left a comment

chrislovecnm Dec 11, 2017

almariah Dec 11, 2017

chrislovecnm Dec 11, 2017

chrislovecnm Dec 11, 2017

almariah Dec 11, 2017

chrislovecnm Dec 11, 2017

almariah Dec 12, 2017 •

edited

Loading

chrislovecnm commented Dec 11, 2017

chrislovecnm Dec 11, 2017

almariah Dec 11, 2017

chrislovecnm Dec 11, 2017

almariah Dec 12, 2017

chrislovecnm Dec 12, 2017

chrislovecnm Dec 12, 2017

chrislovecnm commented Dec 11, 2017

chrislovecnm commented Dec 11, 2017

almariah commented Dec 13, 2017

chrislovecnm Dec 13, 2017

chrislovecnm commented Dec 13, 2017

almariah commented Dec 14, 2017

justinsb commented Dec 14, 2017

k8s-ci-robot commented Dec 14, 2017

k8s-github-robot commented Dec 14, 2017

k8s-github-robot commented Dec 14, 2017

		@@ -311,6 +314,8 @@ func NewCmdCreateCluster(f util.Factory, out io.Writer) cobra.Command {

		cmd.Flags().StringVar(&options.APILoadBalancerType, "api-loadbalancer-type", options.APILoadBalancerType, "Sets the API loadbalancer type to either 'public' or 'internal'")

		cmd.Flags().StringSliceVar(&options.APILoadBalancerSecurityGroups, "api-loadbalancer-security-groups", options.APILoadBalancerSecurityGroups, "Add precreated additional security groups to the API loadbalancer.")

		@@ -263,8 +263,9 @@ const (
		)

		type LoadBalancerAccessSpec struct {

Allow additional SGs to be added to API loadbalancer #4036

Allow additional SGs to be added to API loadbalancer #4036

Conversation

almariah commented Dec 11, 2017 • edited Loading

k8s-ci-robot commented Dec 11, 2017

almariah commented Dec 11, 2017

almariah commented Dec 11, 2017

almariah commented Dec 11, 2017

chrislovecnm 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

almariah Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

chrislovecnm commented Dec 11, 2017

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

chrislovecnm commented Dec 11, 2017

chrislovecnm commented Dec 11, 2017

almariah commented Dec 13, 2017

Choose a reason for hiding this comment

chrislovecnm commented Dec 13, 2017

almariah commented Dec 14, 2017

justinsb commented Dec 14, 2017

k8s-ci-robot commented Dec 14, 2017

k8s-github-robot commented Dec 14, 2017

k8s-github-robot commented Dec 14, 2017

almariah commented Dec 11, 2017 •

edited

Loading

almariah Dec 12, 2017 •

edited

Loading