Added ability to set --service-node-port-range #3333
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
Hi @robinpercy. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. 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
added
needs-ok-to-test
|
Thanks! /ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
pkg/apis/kops/componentconfig.go
Outdated
| @@ -185,6 +185,8 @@ type KubeAPIServerConfig struct { | |||
| AdmissionControl []string `json:"admissionControl,omitempty" flag:"admission-control"` | |||
| // ServiceClusterIPRange is the service address range | |||
| ServiceClusterIPRange string `json:"serviceClusterIPRange,omitempty" flag:"service-cluster-ip-range"` | |||
| // A port range to reserve for services with NodePort visibility. | |||
| ServiceNodePortRange string `json:"serviceNodePortRange,omitempty" flag:"service-node-port-range"` | |||
There was a problem hiding this comment.
Would it be good to included the expected format here? 100-3000 for instance
|
Does github auto close issues when you say addresses? I know it picks up closes and fixes. Double check that the two issues get closed when this merges please. |
|
@chrislovecnm thanks for the feedback, I'll fix up and squash later today. |
robinpercy
force-pushed
the
node-port-range
branch
from
df2aa3c to
e1b20c0
Compare
|
@chrislovecnm I've squashed and fixed up the comments. The failed e2e test looks like a flake to me. |
|
/test pull-kops-e2e-kubernetes-aws |
1 similar comment
|
/test pull-kops-e2e-kubernetes-aws |
|
We have been having problems with e2e ... framework is not happy |
|
/test pull-kops-e2e-kubernetes-aws |
|
@justinsb how does this interact with the nodeport PR you just put in?? Do we need a follow-up PR to set the security rules correctly. |
|
/lgtm I'll put in a follow up to use this range in the security group and assign it to you for review @robinpercy :-) This is the kubeAPIServer section which has pretty weak guarantees. Out of interest @robinpercy what are you setting here (and why)? Because you do have to be a little bit careful in the range you choose to avoid collisions! |
k8s-ci-robot
added
the
lgtm
|
[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:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Thanks @justinsb. I'm actually not using this feature, just noticed a few other folks were looking for it - and I needed a reintroduction to the codebase 😄 @felipejfc was asking for guidance on safe settings for this value in #3052 though. |
|
/test pull-kops-e2e-kubernetes-aws |
1 similar comment
|
/test pull-kops-e2e-kubernetes-aws |
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
|
That's awesome @robinpercy - thanks for this. I opened #3379 to use this value for the new support for configuring access to NodePort services (which we really want for the e2e test suite) |
Addresses: #3052 and #3326
From what I can tell,
--service-node-port-rangehas been a valid option forever, so I haven't worried about k8s versions.@justinsb @chrislovecnm let me know if any changes are required.