-
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
nodePortAccess, experimental spec override flag #3336
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,9 @@ type CreateClusterOptions struct { | |
MasterSecurityGroups []string | ||
AssociatePublicIP *bool | ||
|
||
// Overrides allows settings values direct in the spec | ||
Overrides []string | ||
|
||
// Channel is the location of the api.Channel to use for our defaults | ||
Channel string | ||
|
||
|
@@ -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") | ||
} | ||
|
||
if featureflag.VSphereCloudProvider.Enabled() { | ||
// vSphere flags | ||
cmd.Flags().StringVar(&options.VSphereServer, "vsphere-server", options.VSphereServer, "vsphere-server is required for vSphere. Set vCenter URL Ex: 10.192.10.30 or myvcenter.io (without https://)") | ||
|
@@ -860,6 +867,10 @@ func RunCreateCluster(f *util.Factory, out io.Writer, c *CreateClusterOptions) e | |
cluster.Spec.SSHAccess = c.SSHAccess | ||
} | ||
|
||
if err := setOverrides(c.Overrides, cluster, instanceGroups); err != nil { | ||
return err | ||
} | ||
|
||
err = cloudup.PerformAssignments(cluster) | ||
if err != nil { | ||
return fmt.Errorf("error populating configuration: %v", err) | ||
|
@@ -1039,3 +1050,22 @@ func parseCloudLabels(s string) (map[string]string, error) { | |
} | ||
return m, nil | ||
} | ||
|
||
// setOverrides sets override values in the spec | ||
func setOverrides(overrides []string, cluster *api.Cluster, instanceGroups []*api.InstanceGroup) error { | ||
for _, override := range overrides { | ||
kv := strings.SplitN(override, "=", 2) | ||
if len(kv) != 2 { | ||
return fmt.Errorf("unhandled override: %q", override) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this actually override is in the wrong format? How about
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
// For now we have hard-code the values we want to support; we'll get test coverage and then do this properly... | ||
switch kv[0] { | ||
case "cluster.spec.nodePortAccess": | ||
cluster.Spec.NodePortAccess = append(cluster.Spec.NodePortAccess, kv[1]) | ||
default: | ||
return fmt.Errorf("unhandled override: %q", override) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would probably not understand this error. How about:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right - hopefully this won't be a permanent state of affairs - and I certainly don't intend to hard-code every field :-) |
||
} | ||
} | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is supposed to be a parallel to kubernetesApiAccess and sshAccess, but open to better ideas. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense. |
||
// HTTPProxy defines connection information to support use of a private cluster behind an forward HTTP Proxy | ||
EgressProxy *EgressProxySpec `json:"egressProxy,omitempty"` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It this changing in this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, but I spotted that this comment was wrong and I figured it wasn't entirely unrelated ;-) |
||
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"` | ||
|
||
// HTTPProxy defines connection information to support use of a private cluster behind an forward HTTP Proxy | ||
EgressProxy *EgressProxySpec `json:"egressProxy,omitempty"` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,83 @@ | ||
apiVersion: kops/v1alpha2 | ||
kind: Cluster | ||
metadata: | ||
creationTimestamp: 2017-01-01T00:00:00Z | ||
name: overrides.example.com | ||
spec: | ||
api: | ||
dns: {} | ||
authorization: | ||
alwaysAllow: {} | ||
channel: stable | ||
cloudProvider: aws | ||
configBase: memfs://tests/overrides.example.com | ||
etcdClusters: | ||
- etcdMembers: | ||
- instanceGroup: master-us-test-1a | ||
name: a | ||
name: main | ||
- etcdMembers: | ||
- instanceGroup: master-us-test-1a | ||
name: a | ||
name: events | ||
iam: | ||
legacy: false | ||
kubernetesApiAccess: | ||
- 0.0.0.0/0 | ||
kubernetesVersion: v1.7.5 | ||
masterPublicName: api.overrides.example.com | ||
networkCIDR: 172.20.0.0/16 | ||
networking: | ||
kubenet: {} | ||
nodePortAccess: | ||
- 1.2.3.4/32 | ||
- 10.20.30.0/24 | ||
nonMasqueradeCIDR: 100.64.0.0/10 | ||
sshAccess: | ||
- 0.0.0.0/0 | ||
subnets: | ||
- cidr: 172.20.32.0/19 | ||
name: us-test-1a | ||
type: Public | ||
zone: us-test-1a | ||
topology: | ||
dns: | ||
type: Public | ||
masters: public | ||
nodes: public | ||
|
||
--- | ||
|
||
apiVersion: kops/v1alpha2 | ||
kind: InstanceGroup | ||
metadata: | ||
creationTimestamp: 2017-01-01T00:00:00Z | ||
labels: | ||
kops.k8s.io/cluster: overrides.example.com | ||
name: master-us-test-1a | ||
spec: | ||
image: kope.io/k8s-1.7-debian-jessie-amd64-hvm-ebs-2017-07-28 | ||
machineType: m3.medium | ||
maxSize: 1 | ||
minSize: 1 | ||
role: Master | ||
subnets: | ||
- us-test-1a | ||
|
||
--- | ||
|
||
apiVersion: kops/v1alpha2 | ||
kind: InstanceGroup | ||
metadata: | ||
creationTimestamp: 2017-01-01T00:00:00Z | ||
labels: | ||
kops.k8s.io/cluster: overrides.example.com | ||
name: nodes | ||
spec: | ||
image: kope.io/k8s-1.7-debian-jessie-amd64-hvm-ebs-2017-07-28 | ||
machineType: t2.medium | ||
maxSize: 2 | ||
minSize: 2 | ||
role: Node | ||
subnets: | ||
- us-test-1a |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
ClusterName: overrides.example.com | ||
Zones: | ||
- us-test-1a | ||
Cloud: aws | ||
KubernetesVersion: v1.7.5 | ||
Overrides: | ||
- cluster.spec.nodePortAccess=1.2.3.4/32 | ||
- cluster.spec.nodePortAccess=10.20.30.0/24 |
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.
nit pick. Are these
overrides
? Plural?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.
The idea is that you pass the flag repeatedly. So there are many overrides, but only one per flag.