nodePortAccess, experimental spec override flag #3336

justinsb · 2017-09-04T18:12:55Z

This will allow us to set CIDRs for nodeport access, which in turn will
allow e2e tests that require nodeport access to pass.

Then add a feature-flagged flag to kops create cluster to allow
arbitrary setting of spec values; currently the only value supported is
cluster.spec.nodePortAccess

This will allow us to set CIDRs for nodeport access, which in turn will allow e2e tests that require nodeport access to pass. Then add a feature-flagged flag to `kops create cluster` to allow arbitrary setting of spec values; currently the only value supported is cluster.spec.nodePortAccess

justinsb · 2017-09-06T22:38:02Z

/retest

chrislovecnm

Looks great. Just naming stuff and user ux. This is under a feature flag, so most of the suggestions can be in a follow-up. The only sticker that I have is the API value name. Need some feedback.

chrislovecnm · 2017-09-06T23:04:02Z

cmd/kops/create_cluster.go

+		case "cluster.spec.nodePortAccess":
+			cluster.Spec.NodePortAccess = append(cluster.Spec.NodePortAccess, kv[1])
+		default:
+			return fmt.Errorf("unhandled override: %q", override)


I would probably not understand this error.

How about:

Only cluster.Spec.NodePortAccess is supported, and %s was set More support is planned for the future.

Right - hopefully this won't be a permanent state of affairs - and I certainly don't intend to hard-code every field :-)

chrislovecnm · 2017-09-06T23:06:53Z

pkg/apis/kops/cluster.go

@@ -92,6 +92,8 @@ type ClusterSpec struct {
 	NonMasqueradeCIDR string `json:"nonMasqueradeCIDR,omitempty"`
 	// SSHAccess is a list of the CIDRs that can access SSH.
 	SSHAccess []string `json:"sshAccess,omitempty"`
+	// NodePortAccess is a list of the CIDRs that can access the node ports range (30000-32767).
+	NodePortAccess []string `json:"nodePortAccess,omitempty"`


nit pick, but the name does not make sense to me ... It is an API value so we need to have it named well. @blakebarnett / @alrs / @KashifSaadat thoughts? NodePortAccess sounds like a true / false. Not a list of CIDRS

NodePortSecGroupRanges?

It is supposed to be a parallel to kubernetesApiAccess and sshAccess, but open to better ideas.

That makes sense.

chrislovecnm · 2017-09-06T23:07:34Z

pkg/apis/kops/v1alpha2/cluster.go

@@ -109,9 +109,11 @@ type ClusterSpec struct {
 	NonMasqueradeCIDR string `json:"nonMasqueradeCIDR,omitempty"`

 	// SSHAccess determines the permitted access to SSH
-	// Currently only a single CIDR is supported (though a richer grammar could be added in future)


It this changing in this PR?

No, but I spotted that this comment was wrong and I figured it wasn't entirely unrelated ;-)

chrislovecnm · 2017-09-06T23:13:51Z

cmd/kops/create_cluster.go

+	for _, override := range overrides {
+		kv := strings.SplitN(override, "=", 2)
+		if len(kv) != 2 {
+			return fmt.Errorf("unhandled override: %q", override)


unhandled override

Is this actually override is in the wrong format? How about

--override must be in key=value format. For example cluster.spec.nodePortAccess=0.0.0.0/0

TBH I'm not yet sure what the overrides should look like. They could be JSON, for example.

I figured I would guard it behind a feature flag, we can start using it to get test coverage, and we can see what "feels" right.

chrislovecnm · 2017-09-06T23:14:20Z

cmd/kops/create_cluster.go

@@ -284,6 +287,10 @@ 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'")

+	if featureflag.SpecOverrideFlag.Enabled() {
+		cmd.Flags().StringSliceVar(&options.Overrides, "override", options.Overrides, "Directly configure values in the spec")


nit pick. Are these overrides? Plural?

The idea is that you pass the flag repeatedly. So there are many overrides, but only one per flag.

chrislovecnm · 2017-09-07T00:45:44Z

@justinsb did you want to update the fmt.Errorf in this PR or a followup?

justinsb · 2017-09-07T04:37:43Z

Let's do a follow-up - I'm hoping to find a trick to make it easy to set those, and it is guarded behind a feature flag for now.

chrislovecnm · 2017-09-07T05:08:16Z

/lgtm please file an issue for the follow-up PR

chrislovecnm · 2017-09-07T05:08:25Z

/lgtm

k8s-github-robot · 2017-09-07T05:08:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm, 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~~ [chrislovecnm,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-09-07T05:08:58Z

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

chrislovecnm · 2017-09-07T05:54:50Z

Certain irony about an enhancement that is meant to help e2e to fail when we cannot create loadbalancers, and while the enhancement is not enabled, e2e fails because of a flake.

/retest

justinsb · 2017-09-07T16:18:52Z

/retest

krzyzacy · 2017-09-07T18:27:36Z

/test pull-kops-e2e-kubernetes-aws

krzyzacy · 2017-09-07T20:19:24Z

/test pull-kops-e2e-kubernetes-aws

krzyzacy · 2017-09-07T20:19:40Z

ref kubernetes/test-infra#4441

krzyzacy · 2017-09-07T20:25:45Z

whoops - kops-k8s presubmit goes less smoothly than k8s-kops I'll need to take another look

BenTheElder · 2017-09-07T23:58:30Z

/test pull-kops-e2e-kubernetes-aws

krzyzacy · 2017-09-08T00:34:27Z

(that looks like some legit ❄️ )

alrs · 2017-09-08T00:35:38Z

/test pull-kops-e2e-kubernetes-aws

…

On Thu, Sep 7, 2017 at 5:34 PM, Sen Lu ***@***.***> wrote: (that looks like some legit ❄️ ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3336 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABva_bowzoKuNDdwShGm3nQ5sdxJxBGks5sgIuYgaJpZM4PMKcM> .

justinsb · 2017-09-08T02:50:48Z

/retest

justinsb · 2017-09-08T13:13:18Z

/retest

(Fixed test, I believe)

krzyzacy · 2017-09-08T16:52:48Z

ResumableUploadAbortException: 403 211744435552-compute@developer.gserviceaccount.com does not have storage.objects.create access to bucket kops-ci

Grrr... I'm debugging with kubernetes/test-infra#4461

krzyzacy · 2017-09-08T19:23:03Z

/retest

krzyzacy · 2017-09-08T21:30:32Z

/retest

k8s-github-robot · 2017-09-08T22:06:47Z

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

krzyzacy · 2017-09-08T22:08:48Z

hey it works :-)

k8s-github-robot · 2017-09-08T22:40:01Z

Automatic merge from submit-queue

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 4, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 4, 2017

justinsb force-pushed the nodeportaccess branch from 17587a4 to 9d31ed1 Compare September 4, 2017 18:27

chrislovecnm approved these changes Sep 6, 2017

View reviewed changes

k8s-ci-robot assigned chrislovecnm Sep 7, 2017

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

krzyzacy mentioned this pull request Sep 7, 2017

Fix add_env call in kops-build kubernetes/test-infra#4455

Merged

justinsb mentioned this pull request Sep 8, 2017

Have pull-kops-e2e-kubernetes-aws extract ci/latest kubernetes/test-infra#4462

Merged

k8s-github-robot merged commit ae51cfe into kubernetes:master Sep 8, 2017

bismarck mentioned this pull request Jan 24, 2018

NodePort services in AWS deploy not working #4328

Closed

nodePortAccess, experimental spec override flag #3336

nodePortAccess, experimental spec override flag #3336

Conversation

justinsb commented Sep 4, 2017

justinsb commented Sep 6, 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

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 Sep 7, 2017

justinsb commented Sep 7, 2017

chrislovecnm commented Sep 7, 2017

chrislovecnm commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

k8s-github-robot commented Sep 7, 2017

chrislovecnm commented Sep 7, 2017

justinsb commented Sep 7, 2017

krzyzacy commented Sep 7, 2017

krzyzacy commented Sep 7, 2017

krzyzacy commented Sep 7, 2017

krzyzacy commented Sep 7, 2017

BenTheElder commented Sep 7, 2017

krzyzacy commented Sep 8, 2017

alrs commented Sep 8, 2017 via email

justinsb commented Sep 8, 2017

justinsb commented Sep 8, 2017

krzyzacy commented Sep 8, 2017

krzyzacy commented Sep 8, 2017

krzyzacy commented Sep 8, 2017

k8s-github-robot commented Sep 8, 2017

krzyzacy commented Sep 8, 2017

k8s-github-robot commented Sep 8, 2017