-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support network ingress on arbitrary ports #9454
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 |
---|---|---|
|
@@ -157,6 +157,12 @@ func ValidateMasterConfig(config *api.MasterConfig, fldPath *field.Path) Validat | |
} | ||
} | ||
} | ||
if len(config.NetworkConfig.IngressIPNetworkCIDR) > 0 { | ||
cidr := config.NetworkConfig.IngressIPNetworkCIDR | ||
if _, ipNet, err := net.ParseCIDR(cidr); err != nil || ipNet.IP.IsUnspecified() { | ||
validationResults.AddErrors(field.Invalid(fldPath.Child("networkConfig", "ingressIPNetworkCIDR").Index(0), cidr, "must be a valid CIDR notation IP range (e.g. 172.30.0.0/16)")) | ||
} | ||
} | ||
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. Check there is no IP overlap among IngressIPNetworkCIDRs, ExternalIPNetworkCIDRs, ClusterNetworkCIDR and ServiceNetworkCIDR. 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. Are you suggesting that I perform this validation here? As far as I know, this isn't being done at present, and am wondering if this should be done in this PR or separately. 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. Currently, we are checking for IP overlap between ClusterNetworkCIDR and ServiceNetworkCIDR inside the SDN plugin. Looks like this is the good place to ensure various cidrs are disjoint. Detecting these potential issues can avoid unnecessary debugging later. Could be done as a separate PR. |
||
|
||
validationResults.AddErrors(ValidateKubeConfig(config.MasterClients.OpenShiftLoopbackKubeConfig, fldPath.Child("masterClients", "openShiftLoopbackKubeConfig"))...) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,9 @@ const ( | |
|
||
InfraEndpointControllerServiceAccountName = "endpoint-controller" | ||
EndpointControllerRoleName = "system:endpoint-controller" | ||
|
||
InfraServiceIngressIPControllerServiceAccountName = "service-ingress-ip-controller" | ||
ServiceIngressIPControllerRoleName = "system:service-ingress-ip-controller" | ||
) | ||
|
||
type InfraServiceAccounts struct { | ||
|
@@ -766,4 +769,41 @@ func init() { | |
panic(err) | ||
} | ||
|
||
err = InfraSAs.addServiceAccount( | ||
InfraServiceIngressIPControllerServiceAccountName, | ||
authorizationapi.ClusterRole{ | ||
ObjectMeta: kapi.ObjectMeta{ | ||
Name: ServiceIngressIPControllerRoleName, | ||
}, | ||
Rules: []authorizationapi.PolicyRule{ | ||
// Listing and watching services | ||
{ | ||
APIGroups: []string{kapi.GroupName}, | ||
Verbs: sets.NewString("list", "watch"), | ||
Resources: sets.NewString("services"), | ||
}, | ||
// IngressIPController.persistSpec changes the spec of the service | ||
{ | ||
APIGroups: []string{kapi.GroupName}, | ||
Verbs: sets.NewString("update"), | ||
Resources: sets.NewString("services"), | ||
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. Resources: sets.NewString("services", "services/status") then L715-719 can be skipped (less duplication) 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. My reading of policy definition in this file is that being verbose allows for better association between methods and the permissions they require. For read operations, maybe this isn't necessary, but I thought that write operations might benefit from being more explicit. Is this thinking incorrect? 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. Be verbose |
||
}, | ||
// IngressIPController.persistStatus changes the status of the service | ||
{ | ||
APIGroups: []string{kapi.GroupName}, | ||
Verbs: sets.NewString("update"), | ||
Resources: sets.NewString("services/status"), | ||
}, | ||
// IngressIPController.recorder | ||
{ | ||
Verbs: sets.NewString("create", "update", "patch"), | ||
Resources: sets.NewString("events"), | ||
}, | ||
}, | ||
}, | ||
) | ||
if err != nil { | ||
panic(err) | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,14 +16,15 @@ const ExternalIPPluginName = "ExternalIPRanger" | |
|
||
func init() { | ||
kadmission.RegisterPlugin("ExternalIPRanger", func(client clientset.Interface, config io.Reader) (kadmission.Interface, error) { | ||
return NewExternalIPRanger(nil, nil), nil | ||
return NewExternalIPRanger(nil, nil, false), nil | ||
}) | ||
} | ||
|
||
type externalIPRanger struct { | ||
*kadmission.Handler | ||
reject []*net.IPNet | ||
admit []*net.IPNet | ||
reject []*net.IPNet | ||
admit []*net.IPNet | ||
allowIngressIP bool | ||
} | ||
|
||
var _ kadmission.Interface = &externalIPRanger{} | ||
|
@@ -51,11 +52,12 @@ func ParseRejectAdmitCIDRRules(rules []string) (reject, admit []*net.IPNet, err | |
} | ||
|
||
// NewConstraint creates a new SCC constraint admission plugin. | ||
func NewExternalIPRanger(reject, admit []*net.IPNet) *externalIPRanger { | ||
func NewExternalIPRanger(reject, admit []*net.IPNet, allowIngressIP bool) *externalIPRanger { | ||
return &externalIPRanger{ | ||
Handler: kadmission.NewHandler(kadmission.Create, kadmission.Update), | ||
reject: reject, | ||
admit: admit, | ||
Handler: kadmission.NewHandler(kadmission.Create, kadmission.Update), | ||
reject: reject, | ||
admit: admit, | ||
allowIngressIP: allowIngressIP, | ||
} | ||
} | ||
|
||
|
@@ -84,11 +86,30 @@ func (r *externalIPRanger) Admit(a kadmission.Attributes) error { | |
return nil | ||
} | ||
|
||
// Determine if an ingress ip address should be allowed as an | ||
// external ip by checking the loadbalancer status of the previous | ||
// object state. Only updates need to be validated against the | ||
// ingress ip since the loadbalancer status cannot be set on | ||
// create. | ||
ingressIP := "" | ||
retrieveIngressIP := a.GetOperation() == kadmission.Update && | ||
r.allowIngressIP && svc.Spec.Type == kapi.ServiceTypeLoadBalancer | ||
if retrieveIngressIP { | ||
old, ok := a.GetOldObject().(*kapi.Service) | ||
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. check both ok and old != nil, since someone can pass a nil api.Service pointer that would be ok = true 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. should really only be checking this in the update case 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. Both done |
||
ipPresent := ok && old != nil && len(old.Status.LoadBalancer.Ingress) > 0 | ||
if ipPresent { | ||
ingressIP = old.Status.LoadBalancer.Ingress[0].IP | ||
} | ||
} | ||
|
||
var errs field.ErrorList | ||
switch { | ||
// administrator disabled externalIPs | ||
case len(svc.Spec.ExternalIPs) > 0 && len(r.admit) == 0: | ||
errs = append(errs, field.Forbidden(field.NewPath("spec", "externalIPs"), "externalIPs have been disabled")) | ||
onlyIngressIP := len(svc.Spec.ExternalIPs) == 1 && svc.Spec.ExternalIPs[0] == ingressIP | ||
if !onlyIngressIP { | ||
errs = append(errs, field.Forbidden(field.NewPath("spec", "externalIPs"), "externalIPs have been disabled")) | ||
} | ||
// administrator has limited the range | ||
case len(svc.Spec.ExternalIPs) > 0 && len(r.admit) > 0: | ||
for i, s := range svc.Spec.ExternalIPs { | ||
|
@@ -97,7 +118,8 @@ func (r *externalIPRanger) Admit(a kadmission.Attributes) error { | |
errs = append(errs, field.Forbidden(field.NewPath("spec", "externalIPs").Index(i), "externalIPs must be a valid address")) | ||
continue | ||
} | ||
if NetworkSlice(r.reject).Contains(ip) || !NetworkSlice(r.admit).Contains(ip) { | ||
notIngressIP := s != ingressIP | ||
if (NetworkSlice(r.reject).Contains(ip) || !NetworkSlice(r.admit).Contains(ip)) && notIngressIP { | ||
errs = append(errs, field.Forbidden(field.NewPath("spec", "externalIPs").Index(i), "externalIP is not allowed")) | ||
continue | ||
} | ||
|
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.
If I specify CIDR "0.0.0.0/32" that means no ingress - do we check for that?
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.
Done. I've disallowed 0.0.0.0 (see validation/master.go).
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.
If we're going to default this to on and provide a value we have to be able to distinguish between unset and set. You can do that either by allowing 0.0.0.0/32 to mean "set, but deliberately empty", or turn this value into a pointer.
However, we can do that in a follow up.
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.
Given that this is a string, isn't "" enough to mean unset?
I'm working on a followup that sets a default and also validates against overlap between IngressIPNetworkCIDR, ExternalIPNetworkCIDRs, ClusterNetworkCIDR and ServiceNetworkCIDR.
Regarding the default, is this the right place to add it?
https://github.com/openshift/origin/blob/master/pkg/cmd/server/start/network_args.go
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.
If you set a default, you need a way to distinguish between default, and unset, so that if an admin doesn't want to use the default, they can say "no default".
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.
No, defaulting happens in pkg/cmd/server/api/v1/conversions.go, and there are a few other steps to handle "default for existing clusters" vs. "default for new clusters"