Skip to content
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.

Sign up for GitHub

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

Make Kubernetes aware of the LoadBalancer behaviour #118895

Merged
merged 4 commits into from
Jul 17, 2023
Merged

Make Kubernetes aware of the LoadBalancer behaviour #118895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e686375
Add IP mode field
RyanAoh Jun 9, 2023
7c6e399
Generated code for IP mode field
RyanAoh Jul 17, 2023
7eab0d7
Proxy changes for IP mode field
RyanAoh Jul 6, 2023
b185049
Integration tests for IP mode field
RyanAoh Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions api/openapi-spec/swagger.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions api/openapi-spec/v3/api__v1_openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3102,6 +3102,10 @@
"description": "IP is set for load-balancer ingress points that are IP based (typically GCE or OpenStack load-balancers)",
"type": "string"
},
"ipMode": {
"description": "IPMode specifies how the load-balancer IP behaves, and may only be specified when the ip field is specified. Setting this to \"VIP\" indicates that traffic is delivered to the node with the destination set to the load-balancer's IP and port. Setting this to \"Proxy\" indicates that traffic is delivered to the node or pod with the destination set to the node's IP and node port or the pod's IP and port. Service implementations may use this information to adjust traffic routing.",
"type": "string"
},
"ports": {
"description": "Ports is a list of records of service ports If used, every port defined in the service should have an entry in it",
"items": {
Expand Down
21 changes: 21 additions & 0 deletions pkg/apis/core/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4018,6 +4018,15 @@ type LoadBalancerIngress struct {
// +optional
Hostname string

// IPMode specifies how the load-balancer IP behaves, and may only be specified when the ip field is specified.
// Setting this to "VIP" indicates that traffic is delivered to the node with
// the destination set to the load-balancer's IP and port.
// Setting this to "Proxy" indicates that traffic is delivered to the node or pod with
// the destination set to the node's IP and node port or the pod's IP and port.
// Service implementations may use this information to adjust traffic routing.
// +optional
IPMode *LoadBalancerIPMode

// Ports is a list of records of service ports
// If used, every port defined in the service should have an entry in it
// +optional
Expand Down Expand Up @@ -6090,3 +6099,15 @@ type PortStatus struct {
// +kubebuilder:validation:MaxLength=316
Error *string
}

// LoadBalancerIPMode represents the mode of the LoadBalancer ingress IP
type LoadBalancerIPMode string

const (
// LoadBalancerIPModeVIP indicates that traffic is delivered to the node with
// the destination set to the load-balancer's IP and port.
LoadBalancerIPModeVIP LoadBalancerIPMode = "VIP"
// LoadBalancerIPModeProxy indicates that traffic is delivered to the node or pod with
// the destination set to the node's IP and port or the pod's IP and port.
LoadBalancerIPModeProxy LoadBalancerIPMode = "Proxy"
)
13 changes: 13 additions & 0 deletions pkg/apis/core/v1/defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,19 @@ func SetDefaults_Service(obj *v1.Service) {
obj.Spec.AllocateLoadBalancerNodePorts = pointer.Bool(true)
}
}

if obj.Spec.Type == v1.ServiceTypeLoadBalancer {
thockin marked this conversation as resolved.
Show resolved Hide resolved
if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) {
ipMode := v1.LoadBalancerIPModeVIP

for i, ing := range obj.Status.LoadBalancer.Ingress {
if ing.IP != "" && ing.IPMode == nil {
obj.Status.LoadBalancer.Ingress[i].IPMode = &ipMode
}
}
}
}
thockin marked this conversation as resolved.
Show resolved Hide resolved

}
func SetDefaults_Pod(obj *v1.Pod) {
// If limits are specified, but requests are not, default requests to limits
Expand Down
68 changes: 68 additions & 0 deletions pkg/apis/core/v1/defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1221,6 +1221,74 @@ func TestSetDefaultServiceSessionAffinityConfig(t *testing.T) {
}
}

func TestSetDefaultServiceLoadbalancerIPMode(t *testing.T) {
modeVIP := v1.LoadBalancerIPModeVIP
modeProxy := v1.LoadBalancerIPModeProxy
testCases := []struct {
name string
ipModeEnabled bool
svc *v1.Service
expectedIPMode []*v1.LoadBalancerIPMode
}{
{
name: "Set IP but not set IPMode with LoadbalancerIPMode disabled",
ipModeEnabled: false,
svc: &v1.Service{
Spec: v1.ServiceSpec{Type: v1.ServiceTypeLoadBalancer},
Status: v1.ServiceStatus{
LoadBalancer: v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{{
IP: "1.2.3.4",
}},
},
}},
expectedIPMode: []*v1.LoadBalancerIPMode{nil},
}, {
name: "Set IP but bot set IPMode with LoadbalancerIPMode enabled",
ipModeEnabled: true,
svc: &v1.Service{
Spec: v1.ServiceSpec{Type: v1.ServiceTypeLoadBalancer},
Status: v1.ServiceStatus{
LoadBalancer: v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{{
IP: "1.2.3.4",
}},
},
}},
expectedIPMode: []*v1.LoadBalancerIPMode{&modeVIP},
}, {
name: "Both IP and IPMode are set with LoadbalancerIPMode enabled",
ipModeEnabled: true,
svc: &v1.Service{
Spec: v1.ServiceSpec{Type: v1.ServiceTypeLoadBalancer},
Status: v1.ServiceStatus{
LoadBalancer: v1.LoadBalancerStatus{
Ingress: []v1.LoadBalancerIngress{{
IP: "1.2.3.4",
IPMode: &modeProxy,
}},
},
}},
expectedIPMode: []*v1.LoadBalancerIPMode{&modeProxy},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled)()
obj := roundTrip(t, runtime.Object(tc.svc))
svc := obj.(*v1.Service)
for i, s := range svc.Status.LoadBalancer.Ingress {
got := s.IPMode
expected := tc.expectedIPMode[i]
if !reflect.DeepEqual(got, expected) {
t.Errorf("Expected IPMode %v, got %v", tc.expectedIPMode[i], s.IPMode)
}
}
})
}
}

func TestSetDefaultSecretVolumeSource(t *testing.T) {
s := v1.PodSpec{}
s.Volumes = []v1.Volume{
Expand Down
2 changes: 2 additions & 0 deletions pkg/apis/core/v1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions pkg/apis/core/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6997,6 +6997,10 @@ func ValidatePodLogOptions(opts *core.PodLogOptions) field.ErrorList {
return allErrs
}

var (
supportedLoadBalancerIPMode = sets.NewString(string(core.LoadBalancerIPModeVIP), string(core.LoadBalancerIPModeProxy))
)

// ValidateLoadBalancerStatus validates required fields on a LoadBalancerStatus
func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand All @@ -7007,6 +7011,17 @@ func ValidateLoadBalancerStatus(status *core.LoadBalancerStatus, fldPath *field.
allErrs = append(allErrs, field.Invalid(idxPath.Child("ip"), ingress.IP, "must be a valid IP address"))
}
}

if utilfeature.DefaultFeatureGate.Enabled(features.LoadBalancerIPMode) && ingress.IPMode == nil {
if len(ingress.IP) > 0 {
allErrs = append(allErrs, field.Required(idxPath.Child("ipMode"), "must be specified when `ip` is set"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe defaulting only occurs when creating new objects, but validation will happen when updating existing objects, so this would prevent you from updating existing old Service objects that use IP but don't have IPMode set.

I think overall it might be better to make IPMode be required, and do default-on-read to handle existing objects where it's not set... (?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the backticks here? Error messages aren't Markdown.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK status is only updated

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#spec-and-status

When a new version of an object is POSTed or PUT, the spec is updated and available immediately. Over time the system will work to bring the status into line with the spec.

but I do not think we should default, do we? https://github.com/kubernetes/kubernetes/pull/118895/files#r1247552003

@thockin @liggitt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe defaulting only occurs when creating new objects

Nope! it magically happens any time a versioned object is created, including upon reading from storage. Occasionally this is bad, but mostly it is good for us. In this case it is good! The defaultOnRead() stuff was added because:

// defaultOnRead sets interlinked fields that were not previously set on read.
// We can't do this in the normal defaulting path because that same logic
// applies on Get, Create, and Update, but we need to distinguish between them.

Why the backticks here?

We wrote this down as a convention a looooooong time back, in an effort to make error messages clearer. It's not to render markdown, but to denote "this is a field name". I'm happy to revisit the convention (seriously!) but we should fix all the places that do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wrote this down as a convention a looooooong time back, in an effort to make error messages clearer. It's not to render markdown, but to denote "this is a field name". I'm happy to revisit the convention (seriously!) but we should fix all the places that do it.

Is that convention documented somewhere user visible? Would be nice.

I think changing it might be a KEP… - possible, but more than trivial effort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that convention documented somewhere user visible?

Probably not - we never considered the error messages to be API and made no guarantees on them. As such, I don't think a KEP would be needed, just a clearly better convention.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

x-ref #119452 and #119454

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pacoxu has found some test failures that appear to be related to this (links above). I'm not sure what the solution is, but this specific validation appears to be backwards incompatible in practice.

}
} else if ingress.IPMode != nil && len(ingress.IP) == 0 {
allErrs = append(allErrs, field.Forbidden(idxPath.Child("ipMode"), "may not be specified when `ip` is not set"))
} else if ingress.IPMode != nil && !supportedLoadBalancerIPMode.Has(string(*ingress.IPMode)) {
allErrs = append(allErrs, field.NotSupported(idxPath.Child("ipMode"), ingress.IPMode, supportedLoadBalancerIPMode.List()))
}

if len(ingress.Hostname) > 0 {
for _, msg := range validation.IsDNS1123Subdomain(ingress.Hostname) {
allErrs = append(allErrs, field.Invalid(idxPath.Child("hostname"), ingress.Hostname, msg))
Expand Down
84 changes: 84 additions & 0 deletions pkg/apis/core/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22997,3 +22997,87 @@ func TestValidateDynamicResourceAllocation(t *testing.T) {
}
})
}

func TestValidateLoadBalancerStatus(t *testing.T) {
ipModeVIP := core.LoadBalancerIPModeVIP
ipModeProxy := core.LoadBalancerIPModeProxy
ipModeDummy := core.LoadBalancerIPMode("dummy")

testCases := []struct {
name string
ipModeEnabled bool
tweakLBStatus func(s *core.LoadBalancerStatus)
numErrs int
}{
/* LoadBalancerIPMode*/
{
name: "valid vip ipMode",
ipModeEnabled: true,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IP: "1.2.3.4",
IPMode: &ipModeVIP,
}}
},
numErrs: 0,
}, {
name: "valid proxy ipMode",
ipModeEnabled: true,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IP: "1.2.3.4",
IPMode: &ipModeProxy,
}}
},
numErrs: 0,
}, {
name: "invalid ipMode",
ipModeEnabled: true,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IP: "1.2.3.4",
IPMode: &ipModeDummy,
}}
},
numErrs: 1,
}, {
name: "missing ipMode with LoadbalancerIPMode enabled",
ipModeEnabled: true,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IP: "1.2.3.4",
}}
},
numErrs: 1,
}, {
name: "missing ipMode with LoadbalancerIPMode disabled",
ipModeEnabled: false,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IP: "1.2.3.4",
}}
},
numErrs: 0,
}, {
name: "missing ip with ipMode present",
ipModeEnabled: true,
tweakLBStatus: func(s *core.LoadBalancerStatus) {
s.Ingress = []core.LoadBalancerIngress{{
IPMode: &ipModeProxy,
}}
},
numErrs: 1,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.LoadBalancerIPMode, tc.ipModeEnabled)()
s := core.LoadBalancerStatus{}
RyanAoh marked this conversation as resolved.
Show resolved Hide resolved
tc.tweakLBStatus(&s)
errs := ValidateLoadBalancerStatus(&s, field.NewPath("status"))
if len(errs) != tc.numErrs {
t.Errorf("Unexpected error list for case %q(expected:%v got %v) - Errors:\n %v", tc.name, tc.numErrs, len(errs), errs.ToAggregate())
}
})
}
}
5 changes: 5 additions & 0 deletions pkg/apis/core/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 8 additions & 0 deletions pkg/features/kube_features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -885,6 +885,12 @@ const (
//
// Enables In-Place Pod Vertical Scaling
InPlacePodVerticalScaling featuregate.Feature = "InPlacePodVerticalScaling"

// owner: @Sh4d1,@RyanAoh
// kep: http://kep.k8s.io/1860
// alpha: v1.29
// LoadBalancerIPMode enables the IPMode field in the LoadBalancerIngress status of a Service
LoadBalancerIPMode featuregate.Feature = "LoadBalancerIPMode"
)

func init() {
Expand Down Expand Up @@ -1124,6 +1130,8 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS

PodIndexLabel: {Default: true, PreRelease: featuregate.Beta},

LoadBalancerIPMode: {Default: false, PreRelease: featuregate.Alpha},

// inherited features from generic apiserver, relisted here to get a conflict if it is changed
// unintentionally on either side:

Expand Down
7 changes: 7 additions & 0 deletions pkg/generated/openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions pkg/proxy/conntrack/cleanup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ func deleteStaleServiceConntrackEntries(isIPv6 bool, exec utilexec.Interface, sv
for _, extIP := range svcInfo.ExternalIPStrings() {
conntrackCleanupServiceIPs.Insert(extIP)
}
for _, lbIP := range svcInfo.LoadBalancerIPStrings() {
for _, lbIP := range svcInfo.LoadBalancerVIPStrings() {
conntrackCleanupServiceIPs.Insert(lbIP)
}
nodePort := svcInfo.NodePort()
Expand Down Expand Up @@ -100,7 +100,7 @@ func deleteStaleEndpointConntrackEntries(exec utilexec.Interface, svcPortMap pro
klog.ErrorS(err, "Failed to delete endpoint connections for externalIP", "servicePortName", epSvcPair.ServicePortName, "externalIP", extIP)
}
}
for _, lbIP := range svcInfo.LoadBalancerIPStrings() {
for _, lbIP := range svcInfo.LoadBalancerVIPStrings() {
err := ClearEntriesForNAT(exec, lbIP, endpointIP, v1.ProtocolUDP)
if err != nil {
klog.ErrorS(err, "Failed to delete endpoint connections for LoadBalancerIP", "servicePortName", epSvcPair.ServicePortName, "loadBalancerIP", lbIP)
Expand Down
8 changes: 4 additions & 4 deletions pkg/proxy/iptables/proxier.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1032,7 +1032,7 @@ func (proxier *Proxier) syncProxyRules() {
// create a firewall chain.
loadBalancerTrafficChain := externalTrafficChain
fwChain := svcInfo.firewallChainName
usesFWChain := hasEndpoints && len(svcInfo.LoadBalancerIPStrings()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0
usesFWChain := hasEndpoints && len(svcInfo.LoadBalancerVIPStrings()) > 0 && len(svcInfo.LoadBalancerSourceRanges()) > 0
if usesFWChain {
activeNATChains[fwChain] = true
loadBalancerTrafficChain = fwChain
Expand Down Expand Up @@ -1124,7 +1124,7 @@ func (proxier *Proxier) syncProxyRules() {
}

// Capture load-balancer ingress.
for _, lbip := range svcInfo.LoadBalancerIPStrings() {
for _, lbip := range svcInfo.LoadBalancerVIPStrings() {
if hasEndpoints {
natRules.Write(
"-A", string(kubeServicesChain),
Expand All @@ -1149,7 +1149,7 @@ func (proxier *Proxier) syncProxyRules() {
// Either no endpoints at all (REJECT) or no endpoints for
// external traffic (DROP anything that didn't get short-circuited
// by the EXT chain.)
for _, lbip := range svcInfo.LoadBalancerIPStrings() {
for _, lbip := range svcInfo.LoadBalancerVIPStrings() {
filterRules.Write(
"-A", string(kubeExternalServicesChain),
"-m", "comment", "--comment", externalTrafficFilterComment,
Expand Down Expand Up @@ -1327,7 +1327,7 @@ func (proxier *Proxier) syncProxyRules() {
// will loop back with the source IP set to the VIP. We
// need the following rules to allow requests from this node.
if allowFromNode {
for _, lbip := range svcInfo.LoadBalancerIPStrings() {
for _, lbip := range svcInfo.LoadBalancerVIPStrings() {
natRules.Write(
args,
"-s", lbip,
Expand Down
Loading