fix kube-proxy panic because of nil sessionAffinityConfig #51500

m1093782566 · 2017-08-29T06:30:36Z

What this PR does / why we need it:

fix kube-proxy panic because of nil sessionAffinityConfig

Which issue this PR fixes: closes #51499

Special notes for your reviewer:

I apology that this bug is introduced by #49850 :(

Release note:

NONE

m1093782566 · 2017-08-29T06:31:00Z

/assign @thockin

thockin · 2017-08-29T06:37:57Z

pkg/proxy/iptables/proxier.go

@@ -196,7 +196,15 @@ func newServiceInfo(svcPortName proxy.ServicePortName, port *api.ServicePort, se
 	}
 	var stickyMaxAgeSeconds int
 	if service.Spec.SessionAffinity == api.ServiceAffinityClientIP {
-		stickyMaxAgeSeconds = int(*service.Spec.SessionAffinityConfig.ClientIP.TimeoutSeconds)
+		// We changed the API definition that sessionAffinityConfig is required when


Isn't that set in defaulting? When we read it back from API it should get default values. Before we merge this, I need to understand why that isn't working.

Isn't that set in defaulting? When we read it back from API it should get default values.

When we set the default value in pkg/api/v1/defaults.go, we meet the UT issues.

--- FAIL: TestRoundTripTypes (0.31s) roundtrip.go:326: Service: diff: object.Spec.Selector: a: map[string]string{} b: map[string]string(nil) object.Spec.ExternalIPs: a: []string{} b: []string(nil) object.Spec.SessionAffinityConfig: a: <nil> b: &api.SessionAffinityConfig{ClientIP:(*api.ClientIPConfig)(0xc420643f48)} object.Spec.LoadBalancerSourceRanges: a: []string{} b: []string(nil) object.Status.LoadBalancer.Ingress: a: []api.LoadBalancerIngress{} b: []api.LoadBalancerIngress(nil)

It looks like a testcase that goes through defaulting, which causes the diff to fail. Unfortunately, we don't have a great general solution to this (yet) except to patch up the test case to not depend on defaulting. Set that value explicitly.

So, we set the value explicitly which means the timeout value is required when session affinity type is client ip. For example, we set it explicitly in pkg/master/controller.go.

I guess your may refer to

// ClientIPConfig represents the configurations of Client IP based session affinity. type ClientIPConfig struct { // timeoutSeconds specifies the seconds of ClientIP type session sticky time. // The value must be >0 && <=86400(for 1 day) if ServiceAffinity == "ClientIP". // Default value is 10800(for 3 hours). // +optional TimeoutSeconds *int32 }

The comment message maybe not proper.

thockin · 2017-08-29T07:05:45Z

Sorry, I am lost. Defaulting causes round-trip failures, so you manually specify values in a test. OK. But the defaulting still is in place, right? How does that manifest as a nil pointer on the consumer?

…

On Mon, Aug 28, 2017 at 11:52 PM, DuJun ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/proxy/iptables/proxier.go <#51500 (comment)> : > @@ -196,7 +196,15 @@ func newServiceInfo(svcPortName proxy.ServicePortName, port *api.ServicePort, se } var stickyMaxAgeSeconds int if service.Spec.SessionAffinity == api.ServiceAffinityClientIP { - stickyMaxAgeSeconds = int(*service.Spec.SessionAffinityConfig.ClientIP.TimeoutSeconds) + // We changed the API definition that sessionAffinityConfig is required when Isn't that set in defaulting? When we read it back from API it should get default values. When we set the default value in pkg/api/v1/defaults.go, we meet the UT issues. --- FAIL: TestRoundTripTypes (0.31s) roundtrip.go:326: Service: diff: object.Spec.Selector: a: map[string]string{} b: map[string]string(nil) object.Spec.ExternalIPs: a: []string{} b: []string(nil) object.Spec.SessionAffinityConfig: a: <nil> b: &api.SessionAffinityConfig{ClientIP:(*api.ClientIPConfig)(0xc420643f48)} object.Spec.LoadBalancerSourceRanges: a: []string{} b: []string(nil) object.Status.LoadBalancer.Ingress: a: []api.LoadBalancerIngress{} b: []api.LoadBalancerIngress(nil) It looks like a testcase that goes through defaulting, which causes the diff to fail. Unfortunately, we don't have a great general solution to this (yet) except to patch up the test case to not depend on defaulting. Set that value explicitly. So, we set the value explicitly which means the timeout value is required when session affinity type is client ip. For example, we set it explicitly in pkg/master/controller.go. I guess your may refer to // ClientIPConfig represents the configurations of Client IP based session affinity. type ClientIPConfig struct { // timeoutSeconds specifies the seconds of ClientIP type session sticky time. // The value must be >0 && <=86400(for 1 day) if ServiceAffinity == "ClientIP". // Default value is 10800(for 3 hours). // +optional TimeoutSeconds *int32 } The comment message maybe not proper. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#51500 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVGdPZE_NhrwZ4hPDxtIIuNxfB-m8ks5sc7VCgaJpZM4PFb8X> .

m1093782566 · 2017-08-29T07:11:49Z

How does that manifest as a nil pointer on the consumer?

Because the service data is still stored in etcd - for example, service kubernetes. When we delete the legacy services whose session affinity type is Client IP, kube-proxy stops panic.

m1093782566 · 2017-08-29T07:14:12Z

See https://github.com/kubernetes/kubernetes/blob/master/pkg/master/controller.go#L245

KUBE-APISERVER currently doesn't support updating session affinity config for service kubernetes - it only updates service type and ports.

thockin · 2017-08-29T07:14:59Z

When we read it out of the API, I thought it should get defaulted again, as it transitions from internal to versioned. @lavalamp @cheftako ?

…

On Tue, Aug 29, 2017 at 12:12 AM, DuJun ***@***.***> wrote: How does that manifest as a nil pointer on the consumer? Because the service data is still stored in etcd - for example, service kubernetes. When we delete the legacy services whose session affinity type is Client IP, kube-proxy stops panic. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#51500 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVFKEy5EcFhM-lInXU1RfHDnqvWAbks5sc7nXgaJpZM4PFb8X> .

m1093782566 · 2017-08-29T07:26:54Z

But the defaulting still is in place, right?

Do you refer to

func SetDefaults_Service(obj *v1.Service) {
	if obj.Spec.SessionAffinity == "" {
		obj.Spec.SessionAffinity = v1.ServiceAffinityNone
	}
	if obj.Spec.Type == "" {
		obj.Spec.Type = v1.ServiceTypeClusterIP
	}
	for i := range obj.Spec.Ports {
		sp := &obj.Spec.Ports[i]
		if sp.Protocol == "" {
			sp.Protocol = v1.ProtocolTCP
		}
		if sp.TargetPort == intstr.FromInt(0) || sp.TargetPort == intstr.FromString("") {
			sp.TargetPort = intstr.FromInt(int(sp.Port))
		}
	}
	// Defaults ExternalTrafficPolicy field for NodePort / LoadBalancer service
	// to Global for consistency.
	if (obj.Spec.Type == v1.ServiceTypeNodePort ||
		obj.Spec.Type == v1.ServiceTypeLoadBalancer) &&
		obj.Spec.ExternalTrafficPolicy == "" {
		obj.Spec.ExternalTrafficPolicy = v1.ServiceExternalTrafficPolicyTypeCluster
	}
}

?

I am afraid I didn't set default values here.

m1093782566 · 2017-08-29T11:51:06Z

I think I have almost found out how to fix the defaulting issue - I need to hack pkg/api/fuzzer/fuzzer.go.

@thockin
Please see if the changes are fine. I apologize again for introducing the serious bug. Soooooorry...

m1093782566 · 2017-08-29T12:04:55Z

When we read it out of the API, I thought it should get defaulted again, as
it transitions from internal to versioned.

It's true.

thockin · 2017-08-29T17:58:03Z

pkg/api/fuzzer/fuzzer.go

@@ -439,6 +439,14 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
 					ss.Ports[i].TargetPort.StrVal = "x" + ss.Ports[i].TargetPort.StrVal // non-empty
 				}
 			}
+			if ss.SessionAffinity == api.ServiceAffinityClientIP && ss.SessionAffinityConfig == nil {


we probably should fuzz affinity explicitly. There are many examples to follow, but basically declare a slice of valid values, pick a random number mod the length of that slice, set the value and THEN do this fixup. Basically every enumerated field should go through this.

The chance of the fuzzer choosing a valid string is ~0

ACK. Please see if the changes are fine.

PS. Since there is a restriction on affinity type and affinity config - config is required when type is ClientIP. So, I employ a temporary struct,

struct { affinityType api.ServiceAffinity affinityConfig *api.SessionAffinityConfig }

to describe the relationship between them.

thockin · 2017-08-29T18:03:08Z

This was in some form of the original PR, Iknow I looked at it. Github tools ... ugh. Yes, defaulting is necessary

m1093782566 · 2017-08-30T06:15:38Z

/test pull-kubernetes-e2e-gce-bazel

m1093782566 · 2017-08-31T04:04:22Z

kindly ping @smarterclayton

Can you help reviewing this PR when you have a chance?

thockin · 2017-08-31T22:54:18Z

pkg/api/fuzzer/fuzzer.go

@@ -439,6 +439,28 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
 					ss.Ports[i].TargetPort.StrVal = "x" + ss.Ports[i].TargetPort.StrVal // non-empty
 				}
 			}
+
+			timeoutSeconds := api.DefaultClientIPServiceAffinitySeconds


if we really want to fuzz this, we should do:

possible := []string{ api.ServiceAffinityNone, api.ServiceAffinityClientIP } ss.SessionAffinity = possible[random % len(possible)] switch ss.SessionAffinity { case api.ServiceAffinityClientIP: ss.SessionAffinityConfig = new(api.SessionAffinityConfig) ss.SessionAffinityConfig.ClientIP = new(api.ClientIPConfig) ss.SessionAffinityConfig.ClientIP.TimeoutSeconds = random() % api.SessionAffinityMax }

Thanks for your good example :)

thockin · 2017-08-31T22:58:25Z

pkg/api/v1/defaults.go

@@ -101,6 +101,14 @@ func SetDefaults_Service(obj *v1.Service) {
 	if obj.Spec.SessionAffinity == "" {
 		obj.Spec.SessionAffinity = v1.ServiceAffinityNone
 	}
+	if obj.Spec.SessionAffinity == v1.ServiceAffinityClientIP && obj.Spec.SessionAffinityConfig == nil {


Don't forget || obj.Spec.SessionAffinityConfig.ClientIP == nil || obj.Spec.SessionAffinityConfig.ClientIP.TimeoutSeconds == 0

Yes, I agree. But, do you mean obj.Spec.SessionAffinityConfig.ClientIP.TimeoutSeconds == nil?

TimeoutSeconds is *int32 type and we treat *TimeoutSeconds == 0 as an invalid value, ref: https://github.com/kubernetes/kubernetes/blob/master/pkg/api/validation/validation.go#L1913

thockin · 2017-08-31T23:00:14Z

pkg/api/v1/defaults.go

@@ -101,6 +101,14 @@ func SetDefaults_Service(obj *v1.Service) {
 	if obj.Spec.SessionAffinity == "" {
 		obj.Spec.SessionAffinity = v1.ServiceAffinityNone


also check for ServiceAffinityNone and set SessionAffinityConfig == nil ?

Currently, we will ignore SessionAffinityConfig when ServiceAffinityNone, but I am fine to check it :)

thockin · 2017-08-31T23:01:06Z

And update tests

m1093782566 · 2017-09-01T07:24:33Z

@thockin

I pushed an update. PTAL.

m1093782566 · 2017-09-01T10:44:29Z

/retest

thockin · 2017-09-01T20:31:26Z

/lgtm
/approve

thockin · 2017-09-01T20:31:33Z

/retest

k8s-github-robot · 2017-09-01T20:31:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: m1093782566, thockin

Associated issue: 51499

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/api/OWNERS~~ [thockin]
~~pkg/master/OWNERS~~ [thockin]
~~pkg/proxy/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/core/OWNERS~~ [thockin]

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-03T22:00:13Z

Automatic merge from submit-queue (batch tested with PRs 51819, 51706, 51761, 51818, 51500)

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 29, 2017

k8s-github-robot assigned smarterclayton and freehan Aug 29, 2017

k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 29, 2017

k8s-ci-robot assigned thockin Aug 29, 2017

thockin reviewed Aug 29, 2017

View reviewed changes

m1093782566 force-pushed the fix-kube-proxy-panic branch 2 times, most recently from 6953697 to 820645c Compare August 29, 2017 11:38

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 29, 2017

m1093782566 force-pushed the fix-kube-proxy-panic branch from bf99220 to 4d50b4d Compare August 29, 2017 12:01

m1093782566 force-pushed the fix-kube-proxy-panic branch from 4d50b4d to 6c609b9 Compare August 29, 2017 14:10

thockin reviewed Aug 29, 2017

View reviewed changes

m1093782566 force-pushed the fix-kube-proxy-panic branch from 6c609b9 to cb7ba1e Compare August 30, 2017 03:41

pineking mentioned this pull request Aug 31, 2017

kube-proxy panic because of service sessionAffinityConfig is nil #51499

Closed

thockin reviewed Aug 31, 2017

View reviewed changes

m1093782566 force-pushed the fix-kube-proxy-panic branch from cb7ba1e to 782af87 Compare September 1, 2017 07:15

m1093782566 force-pushed the fix-kube-proxy-panic branch from 782af87 to 0e2549c Compare September 1, 2017 07:23

m1093782566 added 2 commits September 1, 2017 16:18

fix kube-proxy panic

1faa6f5

remove explictly set timeout value

617e6f4

m1093782566 force-pushed the fix-kube-proxy-panic branch from 0e2549c to 617e6f4 Compare September 1, 2017 08:21

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

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

madhanrm mentioned this pull request Sep 1, 2017

Fix Kubeproxy to work for Windows Kernel mode #51064

Merged

abgworrall modified the milestone: v1.8 Sep 2, 2017

k8s-github-robot merged commit a31bc44 into kubernetes:master Sep 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix kube-proxy panic because of nil sessionAffinityConfig #51500

fix kube-proxy panic because of nil sessionAffinityConfig #51500

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 commented Aug 29, 2017

thockin Aug 29, 2017

m1093782566 Aug 29, 2017

thockin commented Aug 29, 2017 via email

m1093782566 commented Aug 29, 2017

m1093782566 commented Aug 29, 2017

thockin commented Aug 29, 2017 via email

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 commented Aug 29, 2017

thockin Aug 29, 2017

thockin Aug 29, 2017

m1093782566 Aug 30, 2017

thockin commented Aug 29, 2017

m1093782566 commented Aug 30, 2017

m1093782566 commented Aug 31, 2017

thockin Aug 31, 2017

m1093782566 Sep 1, 2017

thockin Aug 31, 2017

m1093782566 Sep 1, 2017 •

edited

thockin Aug 31, 2017

m1093782566 Sep 1, 2017

thockin commented Aug 31, 2017

m1093782566 commented Sep 1, 2017 •

edited

m1093782566 commented Sep 1, 2017

thockin commented Sep 1, 2017

thockin commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

		@@ -101,6 +101,14 @@ func SetDefaults_Service(obj *v1.Service) {
		if obj.Spec.SessionAffinity == "" {
		obj.Spec.SessionAffinity = v1.ServiceAffinityNone

fix kube-proxy panic because of nil sessionAffinityConfig #51500

fix kube-proxy panic because of nil sessionAffinityConfig #51500

Conversation

m1093782566 commented Aug 29, 2017 • edited

m1093782566 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 29, 2017 via email

m1093782566 commented Aug 29, 2017

m1093782566 commented Aug 29, 2017

thockin commented Aug 29, 2017 via email

m1093782566 commented Aug 29, 2017 • edited

m1093782566 commented Aug 29, 2017 • edited

m1093782566 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 29, 2017

m1093782566 commented Aug 30, 2017

m1093782566 commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m1093782566 Sep 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Aug 31, 2017

m1093782566 commented Sep 1, 2017 • edited

m1093782566 commented Sep 1, 2017

thockin commented Sep 1, 2017

thockin commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 commented Aug 29, 2017 •

edited

m1093782566 Sep 1, 2017 •

edited

m1093782566 commented Sep 1, 2017 •

edited