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

RouteStatusGatewayReference.Controller should have an omitempty JSON tag #669

Closed
skriss opened this issue May 17, 2021 · 3 comments · Fixed by #671
Closed

RouteStatusGatewayReference.Controller should have an omitempty JSON tag #669

skriss opened this issue May 17, 2021 · 3 comments · Fixed by #671
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@skriss
Copy link
Contributor

skriss commented May 17, 2021

What happened:
Contour code that updates HTTPRoute status produces errors like the following:

time="2021-05-05T17:02:01Z" level=error msg="unable to update status" context=StatusUpdateHandler error="HTTPRoute.networking.x-k8s.io \"http-filter-1\" is invalid: status.gateways.gatewayRef.controller: Invalid value: \"null\": status.gateways.gatewayRef.controller in body must be of type string: \"null\"" name=http-filter-1 namespace=gateway-005-request-header-modifier-rule

What you expected to happen:
Since the Controller field is optional, it should be omitted from JSON serialization if not specified.

How to reproduce it (as minimally and precisely as possible):
I can provide more detail on how to run the Contour E2E's locally to observe this error if that's actually useful for anyone.

Anything else we need to know?:
Happy to submit a PR, just need confirmation that this makes sense, and also whether to update this in v1alpha1, v1alpha2, or both.

@skriss skriss added the kind/bug Categorizes issue or PR as related to a bug. label May 17, 2021
@robscott
Copy link
Member

Thanks for raising this issue! I know we'd previously said that we'd freeze v1alpha1 and instead focus our development on v1alpha2, but maybe if there are enough bugs we should work on a 0.3.1 with a new release-0.3 branch to support a patch here.

I'll bring this up at our community meeting tomorrow. I think this is a bug, I'm just not sure how broadly we want to apply the patch.

@robscott
Copy link
Member

We discussed this at today's community meeting and decided that it did not meet the bar for a new v1alpha1 release. Given that, I think it makes sense to make this change in v1alpha2 only. As I've been thinking about it more though, I realized that this was only optional in v1alpha1 to ensure backwards compatibility. IIRC, the intent was to make this field required in v1alpha2. I'll open a PR to make that change so the lack of omitempty for this field actually makes sense in v1alpha2.

@skriss
Copy link
Contributor Author

skriss commented May 18, 2021

Sounds good, thanks for the update. We'll update Contour's code to provide a value for this field, to work around the v1alpha1 issue and be ready for v1alpha2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants