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.

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

examine nil pointer before applying backend protocol configurations #678

Merged
merged 1 commit into from Mar 15, 2019

Conversation

@freehan
Copy link
Member

commented Mar 12, 2019

Mitigates #675

@@ -62,6 +62,10 @@ const (
// backends, the port or named port specified in the Backend Service is
// used for health checking.
UseServingPortSpecification = "USE_SERVING_PORT"

newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 12, 2019

Member

what do you mean by exiting?

@@ -62,6 +62,10 @@ const (
// backends, the port or named port specified in the Backend Service is
// used for health checking.
UseServingPortSpecification = "USE_SERVING_PORT"

newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 12, 2019

Member

"..caused by an application..."


newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +
"Please revert the change on application protocol to avoid this error message."

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 12, 2019

Member

"...revert the change on the application..."

This comment has been minimized.

Copy link
@cmluciano

cmluciano Mar 12, 2019

Member
Suggested change
"Please revert the change on application protocol to avoid this error message."
"Please revert the change on the application's protocol to avoid this error message."

I don't have a lot of insight into how the ingress-gce works internally, but I think a TODO on handling this type of change would be helpful. I think progressing from HTTP-1.1 to HTTP-2 (soon HTTP-3) or introducing changes on the protocol to adopt a service mesh will be a common occurrence.

This comment has been minimized.

Copy link
@freehan

freehan Mar 12, 2019

Author Member

yeah. Agreed. In the long run we want to support this. But currently, due to GCE api changes, it was broken. So we need to put this short term fix in place so that it can be cherry picked.

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 14, 2019

Member

Can we add the TODO just so we remember to do it?

@@ -62,6 +62,10 @@ const (
// backends, the port or named port specified in the Backend Service is
// used for health checking.
UseServingPortSpecification = "USE_SERVING_PORT"

newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +

This comment has been minimized.

Copy link
@cmluciano

cmluciano Mar 12, 2019

Member
Suggested change
"This is usually caused by application protocol changed. " +
"This is usually caused by a change of the application's protocol." +

newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +
"Please revert the change on application protocol to avoid this error message."

This comment has been minimized.

Copy link
@cmluciano

cmluciano Mar 12, 2019

Member
Suggested change
"Please revert the change on application protocol to avoid this error message."
"Please revert the change on the application's protocol to avoid this error message."

I don't have a lot of insight into how the ingress-gce works internally, but I think a TODO on handling this type of change would be helpful. I think progressing from HTTP-1.1 to HTTP-2 (soon HTTP-3) or introducing changes on the protocol to adopt a service mesh will be a common occurrence.

@freehan freehan force-pushed the freehan:nil-pointer branch from 2b17999 to e55e043 Mar 12, 2019

@freehan

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

fixed.

@freehan freehan force-pushed the freehan:nil-pointer branch from e55e043 to c518487 Mar 12, 2019

@mariantalla

This comment has been minimized.

Copy link

commented Mar 13, 2019

Hi, is there anything missing for this to be /lgtmed?
cc @MrHohn @thockin

@mariantalla

This comment has been minimized.

Copy link

commented Mar 14, 2019

Assigning for lgtm (please reassign if I've done this wrong):
/assign @MrHohn

@MrHohn

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

This LGTM, but will defer to @rramkumar1 to make the final call.
/assign @rramkumar1

@@ -62,6 +62,10 @@ const (
// backends, the port or named port specified in the Backend Service is
// used for health checking.
UseServingPortSpecification = "USE_SERVING_PORT"

newHealthCheckErrorMessageTemplate = "the %v health check configuration on the existing health check %v is nil. " +
"This is usually caused by an application protocol changed. " +

This comment has been minimized.

Copy link
@bowei

bowei Mar 14, 2019

Member

can we give the name of the actual field involved, this error is a bit vague sounding. are we talking about the GCP object, the protocol used by the app itself, etc

This comment has been minimized.

Copy link
@freehan

freehan Mar 14, 2019

Author Member

HealthCheck object contains HTTPHealthCheck configurations. It has bunch of fields.

In this case, the controller thinks that HTTP2HealthCheck configuration is nil because it was not vendored.

v.HTTPHealthCheck = computealpha.HTTPHealthCheck(*hc.HttpsHealthCheck)
case annotations.ProtocolHTTP2:
if hc.Http2HealthCheck == nil {
err = fmt.Errorf(newHealthCheckErrorMessageTemplate, annotations.ProtocolHTTP2, hc.Name)

This comment has been minimized.

Copy link
@bowei

bowei Mar 14, 2019

Member

can we not return immediately (same in all case above)

@freehan freehan force-pushed the freehan:nil-pointer branch from c518487 to d297145 Mar 14, 2019

@freehan

This comment has been minimized.

Copy link
Member Author

commented Mar 14, 2019

Ready for next round

@@ -62,6 +62,10 @@ const (
// backends, the port or named port specified in the Backend Service is
// used for health checking.
UseServingPortSpecification = "USE_SERVING_PORT"

newHealthCheckErrorMessageTemplate = "the %v health check configurations on the existing health check %v is nil. " +

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 14, 2019

Member

"the %v health check configuration..."


newHealthCheckErrorMessageTemplate = "the %v health check configuration on the exiting health check %v is nil. " +
"This is usually caused by application protocol changed. " +
"Please revert the change on application protocol to avoid this error message."

This comment has been minimized.

Copy link
@rramkumar1

rramkumar1 Mar 14, 2019

Member

Can we add the TODO just so we remember to do it?

@rramkumar1 rramkumar1 changed the title examine nil pointer before applying configurations examine nil pointer before applying backend protocol configurations Mar 14, 2019

@freehan freehan force-pushed the freehan:nil-pointer branch from d297145 to f929fd3 Mar 14, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Mar 14, 2019

@freehan

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Done

@rramkumar1

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 15, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, rramkumar1

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit d2150fc into kubernetes:master Mar 15, 2019

3 checks passed

cla/linuxfoundation freehan authorized
Details
pull-ingress-gce-test Job succeeded.
Details
tide In merge pool.
Details

rramkumar1 added a commit that referenced this pull request Mar 15, 2019

Merge pull request #691 from freehan/cherrypick
Cherrypick #678 into release-1.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.