ingress: rename backend to defaultBackend for ingress v1 #80179

cmluciano · 2019-07-15T20:48:48Z

Signed-off-by: Christopher M. Luciano cmluciano@us.ibm.com

What type of PR is this?
/kind api-change

What this PR does / why we need it:
Per the Ingress v1 KEP, we are renaming the backend field in IngressSpec to
clarify its intended use. Appropriate defaults are added to ensure backend is
converted to the new field type.

Which issue(s) this PR fixes:
Related kubernetes/enhancements#758

Special notes for your reviewer:

Update APIs
Add conversion logic for v1beta1 & v1
Add conversion logic tests

Does this PR introduce a user-facing change?:

ingress: rename backend to defaultBackend for ingress v1

cmluciano · 2019-07-15T20:49:03Z

cc @aledbf @bowei

bowei · 2019-07-17T17:58:25Z

thanks for picking this up :-)

cmluciano · 2019-07-18T17:17:17Z

/priority important-soon
/milestone 1.16

k8s-ci-robot · 2019-07-18T17:17:18Z

@cmluciano: You must be a member of the kubernetes/milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/priority important-soon
/milestone 1.16

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cmluciano · 2019-07-18T21:51:37Z

Just need to add tests tomorrow and then will flip this to "ready"

cmluciano · 2019-11-14T18:28:08Z

Thanks @josiahbjorgaard we plan for this to be combined with other PRs for 1.18

thockin · 2019-12-23T23:29:41Z

Revisiting this, I still have zero idea how to solve this.

cmluciano · 2020-01-02T20:15:06Z

@thockin Jordan shared some examples at KubeCon with me. I am taking a look at how this is done in the admissionregistration package and will pull some examples out of there.

thockin · 2020-01-06T22:21:48Z

I spent time over holidays poking at this. A "real" fix might be long in coming, but @deads2k shared a cute trick.

Take a look at https://github.com/kubernetes/kubernetes/compare/master...thockin:hack-ingress-versioned-validation?expand=1

If we plumb that down, we can use it to choose a name for the error

cmluciano · 2020-01-08T01:28:35Z

@thockin Just saw this now, thanks! I believe this is what my commit is missing to fix the tests.

cmluciano · 2020-02-03T22:34:59Z

@liggitt I think I'm pretty close on this one. The last failure is seems to be the storage test. I think the problem might be that it is missing requestInfo details like I have in strategy.

make test WHAT=./pkg/registry/networking/ingress/storage/...
+++ [0203 16:49:40] Running tests without code coverage
I0203 16:49:43.116015   11059 client.go:361] parsed scheme: "endpoint"
I0203 16:49:43.116161   11059 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:84153270101107355820  <nil> 0 <nil>}]
I0203 16:49:43.118499   11059 client.go:361] parsed scheme: "endpoint"
I0203 16:49:43.118579   11059 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:84153270101107355820  <nil> 0 <nil>}]
I0203 16:49:43.119787   11059 client.go:361] parsed scheme: "endpoint"
I0203 16:49:43.119852   11059 endpoint.go:68] ccResolverWrapper: sending new addresses to cc: [{unix://localhost:84153270101107355820  <nil> 0 <nil>}]
--- FAIL: TestCreate (0.09s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x38 pc=0x15d075c]

goroutine 145 [running]:
testing.tRunner.func1(0xc000506300)
	/usr/local/go/src/testing/testing.go:874 +0x3a3
panic(0x1ba77c0, 0x323af60)
	/usr/local/go/src/runtime/panic.go:679 +0x1b2
k8s.io/kubernetes/pkg/registry/networking/ingress.ingressStrategy.Validate(0x215d0e0, 0xc0001e3960, 0x21517c0, 0x3281680, 0x219cec0, 0xc0007045d0, 0x2156e20, 0xc000483080, 0x0, 0x0, ...)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/networking/ingress/strategy.go:79 +0x6c
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest.BeforeCreate(0x21ba680, 0xc0001544c0, 0x219cec0, 0xc0007045d0, 0x2156e20, 0xc000483080, 0x48, 0x1cb8440)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest/create.go:105 +0x2de
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/generic/registry.(*Store).Create(0xc000299b00, 0x219cec0, 0xc0007045d0, 0x2156e20, 0xc000483080, 0x1f1df58, 0xc0004840f0, 0xc00068a700, 0xc00068a400, 0x20, ...)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/generic/registry/store.go:338 +0x81
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest/resttest.(*Tester).testCreateHasMetadata(0xc0007043f0, 0x2156e20, 0xc000483080)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go:392 +0x1bb
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest/resttest.(*Tester).TestCreate(0xc0007043f0, 0x2156e20, 0xc000482c00, 0xc00082dd38, 0xc00082dd28, 0xc00082de40, 0x2, 0x2)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go:148 +0x109
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/generic/testing.(*Tester).TestCreate(0xc000c6e150, 0x2156e20, 0xc000482c00, 0xc00082de40, 0x2, 0x2)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/registry/generic/testing/tester.go:79 +0xb3
k8s.io/kubernetes/pkg/registry/networking/ingress/storage.TestCreate(0xc000506300)
	/home/cmluciano/go/src/k8s.io/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/registry/networking/ingress/storage/storage_test.go:139 +0x411
testing.tRunner(0xc000506300, 0x1f1b500)
	/usr/local/go/src/testing/testing.go:909 +0xc9
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:960 +0x350
FAIL	k8s.io/kubernetes/pkg/registry/networking/ingress/storage	0.131s
FAIL
Makefile:185: recipe for target 'test' failed
make: *** [test] Error 1

liggitt · 2020-02-04T03:10:48Z

I'd recommend something like this in ValidateUpdate and Validate:

var requestGV schema.GroupVersion
if requestInfo, ok := request.RequestInfoFrom(ctx); ok {
	requestGV = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
}

liggitt · 2020-02-04T03:18:12Z

pkg/apis/networking/validation/validation.go

+		}
+		if requestGV == networkingv1beta1.SchemeGroupVersion {
+			allErrs = append(allErrs, field.Invalid(fldPath, spec.Rules, "either `backend` or `rules` must be specified"))
+		}


Assuming we are better at naming this field now than we were four years ago, something like this that assumes future versions will continue using the v1 field name might make more sense:

var defaultBackendFieldName string switch requestGV { case networkingv1beta1.SchemeGroupVersion, extensionsv1beta1.SchemeGroupVersion: defaultBackendFieldName = "backend" default: defaultBackendFieldName = "defaultBackend" } allErrs := field.ErrorList{} if spec.DefaultBackend != nil { allErrs = append(allErrs, validateIngressBackend(spec.DefaultBackend, fldPath.Child(defaultBackendFieldName))...) } else if len(spec.Rules) == 0 { allErrs = append(allErrs, field.Invalid(fldPath, spec.Rules, fmt.Sprintf("either `%s` or `rules` must be specified", defaultBackendFieldName))) }

The worse-case failure mode of that approach if we change the field name in a future version and forget to update this is that you get the wrong field name in an error message. The failure mode of the current code if we add a version and forget to update this is that we get unvalidated fields!

When I tried it this way it doesn't seem to trigger the validation error that is expected in strategy_test.go. I'm scouring the relevant files under pkg/registry/networking/ingress/* to see if I am accidentally using the old backend type but I have not found a mistake yet.

Nvm copy/paste error, I found my mistake

Actually even after fixing an indenting problem, this still doesnt work because it passes only a string to the ingressbackend validation. I kept the if/else for now.

I kept the if/else for now.

We need a fallback case that still performs validation, not one that skips validation on unknown versions

also, this code doesn't currently handle extensions/v1beta1

cmluciano · 2020-02-06T21:05:13Z

/retest

robscott · 2020-02-10T16:50:58Z

/assign

cmluciano · 2020-02-10T17:35:56Z

/retest

Per the Ingress v1 KEP, we are renaming the backend field in IngressSpec to clarify its intended use. Appropriate defaults are added to ensure backend is converted to the new field type. ingress: validate backend based on schema.GroupVersion Signed-off-by: Christopher M. Luciano <cmluciano@us.ibm.com>

k8s-ci-robot · 2020-02-11T20:53:57Z

@cmluciano: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-verify	`95edbd5`	link	`/test pull-kubernetes-verify`
pull-kubernetes-e2e-gce	`95edbd5`	link	`/test pull-kubernetes-e2e-gce`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

liggitt · 2020-02-21T16:44:59Z

closing in favor of #88041

/close

k8s-ci-robot · 2020-02-21T16:45:03Z

@liggitt: Closed this PR.

In response to this:

closing in favor of #88041

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 15, 2019

k8s-ci-robot requested review from caseydavenport and thockin July 15, 2019 20:50

cmluciano force-pushed the cml/ingdefaultbackend branch from 8228787 to 09e6ec5 Compare July 17, 2019 16:47

cmluciano force-pushed the cml/ingdefaultbackend branch from 09e6ec5 to b2fe1ca Compare July 18, 2019 17:13

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 18, 2019

cmluciano force-pushed the cml/ingdefaultbackend branch from b2fe1ca to d03f4ed Compare July 18, 2019 21:50

cmluciano force-pushed the cml/ingdefaultbackend branch from d03f4ed to 417115b Compare July 19, 2019 17:36

k8s-ci-robot modified the milestones: v1.17, v1.18 Nov 13, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 23, 2019

cmluciano force-pushed the cml/ingdefaultbackend branch 2 times, most recently from e2c493a to c4d43b8 Compare January 15, 2020 19:38

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 15, 2020

cmluciano force-pushed the cml/ingdefaultbackend branch from 7d9ce52 to 50aa7c4 Compare February 3, 2020 17:49

liggitt reviewed Feb 4, 2020

View reviewed changes

cmluciano force-pushed the cml/ingdefaultbackend branch 2 times, most recently from 83450d1 to c0e4c31 Compare February 6, 2020 17:15

k8s-ci-robot assigned robscott Feb 10, 2020

cmluciano mentioned this pull request Feb 10, 2020

Graduate Ingress to V1 kubernetes/enhancements#1453

Closed

cmluciano force-pushed the cml/ingdefaultbackend branch from e26ebea to 95edbd5 Compare February 11, 2020 19:49

k8s-ci-robot closed this Feb 21, 2020

liggitt removed the api-review Categorizes an issue or PR as actively needing an API review. label Feb 21, 2020

liggitt removed this from Changes requested in API Reviews Feb 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingress: rename backend to defaultBackend for ingress v1 #80179

ingress: rename backend to defaultBackend for ingress v1 #80179

cmluciano commented Jul 15, 2019 •

edited

cmluciano commented Jul 15, 2019

bowei commented Jul 17, 2019

cmluciano commented Jul 18, 2019

k8s-ci-robot commented Jul 18, 2019

cmluciano commented Jul 18, 2019

cmluciano commented Nov 14, 2019

thockin commented Dec 23, 2019

cmluciano commented Jan 2, 2020

thockin commented Jan 6, 2020

cmluciano commented Jan 8, 2020

cmluciano commented Feb 3, 2020

liggitt commented Feb 4, 2020

liggitt Feb 4, 2020 •

edited

cmluciano Feb 4, 2020

cmluciano Feb 4, 2020

cmluciano Feb 6, 2020

liggitt Feb 6, 2020

liggitt Feb 6, 2020

cmluciano commented Feb 6, 2020

robscott commented Feb 10, 2020

cmluciano commented Feb 10, 2020

k8s-ci-robot commented Feb 11, 2020

liggitt commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

ingress: rename backend to defaultBackend for ingress v1 #80179

ingress: rename backend to defaultBackend for ingress v1 #80179

Conversation

cmluciano commented Jul 15, 2019 • edited

cmluciano commented Jul 15, 2019

bowei commented Jul 17, 2019

cmluciano commented Jul 18, 2019

k8s-ci-robot commented Jul 18, 2019

cmluciano commented Jul 18, 2019

cmluciano commented Nov 14, 2019

thockin commented Dec 23, 2019

cmluciano commented Jan 2, 2020

thockin commented Jan 6, 2020

cmluciano commented Jan 8, 2020

cmluciano commented Feb 3, 2020

liggitt commented Feb 4, 2020

liggitt Feb 4, 2020 • edited

Choose a reason for hiding this comment

cmluciano Feb 4, 2020

Choose a reason for hiding this comment

cmluciano Feb 4, 2020

Choose a reason for hiding this comment

cmluciano Feb 6, 2020

Choose a reason for hiding this comment

liggitt Feb 6, 2020

Choose a reason for hiding this comment

liggitt Feb 6, 2020

Choose a reason for hiding this comment

cmluciano commented Feb 6, 2020

robscott commented Feb 10, 2020

cmluciano commented Feb 10, 2020

k8s-ci-robot commented Feb 11, 2020

liggitt commented Feb 21, 2020

k8s-ci-robot commented Feb 21, 2020

cmluciano commented Jul 15, 2019 •

edited

liggitt Feb 4, 2020 •

edited