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

Provide parent name to parser service builder #2566

Closed
1 task
rainest opened this issue Jun 10, 2022 · 3 comments · Fixed by #3318
Closed
1 task

Provide parent name to parser service builder #2566

rainest opened this issue Jun 10, 2022 · 3 comments · Fixed by #3318
Assignees
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API nice-to-have size/small

Comments

@rainest
Copy link
Contributor

rainest commented Jun 10, 2022

Problem Statement

The parser service builder may need to log or return errors relevant to a single service only, e.g. if that service is comprised of multiple incompatible Kubernetes Services. For users to fix this misconfiguration effectively, they may wish to review which resources are pointing to the Service(s).

The parser handles some parts of Service translation in a batch job after processing all ingress rules, rather than processing Services as they're encountered in rules.

This job validates that all Services in a multi-Service backend use the same annotations. When it encounters a problem, it emits an error that lists the Services, but not the route that placed them in a multi-Service backend. To make an informed decision about which annotation value is correct, users may need to know the associated route, but cannot find it given the information in the log

Proposed Solution

We can provide this by adding an additional ingressRules.ServiceNameToParent and adding a GVK+NamespacedName to it at the same time we insert the service name to service mapping.

Additional information

For Gateway Routes (currently the only resource type that can encounter this class of error), we also want to indicate the issue in the Route object status with a false RouteConditionAccepted Condition and a Kong-specific reason (there's no appropriate reasons in the standard reasons and this failure is too specific to Kong to warrant a standard reason).

The parser cannot update Kubernetes resources to add Conditions. To add this, we would either need:

  • New infrastructure to trigger a reconcile event and ship information from the parser back to the reconcilers. The status update system is similar to what we'd need to add for this (see chore: remove KongIngress support for Gateway API Route objects and Services referenced by those Routes #2554 (comment)).
  • A check within the reconciler to check whether referenced Services have this problem condition that rejects the route and applies the condition up front. This approach may be ill-advised because it requires cross-resource checks, but that may be fine: if the controller requeues the problem resource, it will eventually clear the condition if the Services were only temporarily out of sync. Effective reuqueing probably requires something similar to the proposed change in Limit ingested Secrets to those actually used #2868, for xRoutes and referent Services rather than referent Secrets.

Given the complexity of handling the Conditions, I think we should limit scope to improving the logs for now.

Acceptance Criteria

  • When the controller logs an error about inconsistent Service annotations, it includes the name and namespace of the resource that placed those Services in the same backend.
@mflendrich
Copy link
Member

@rainest could you please edit this to match the template and include proposed solution & ACs?

@rainest rainest removed their assignment Oct 13, 2022
@mflendrich
Copy link
Member

@rainest please verify whether it has already been done and close if so.

@mflendrich mflendrich removed this from the KIC input for 2.9/2.10 milestone Jan 3, 2023
@rainest
Copy link
Contributor Author

rainest commented Jan 4, 2023

It has not, we were only emitting events for the problem Services, not the parent route that used them in a a backend.

#3318 would add an Event for the parent, but we don't have a system for updating the parent's status, which we arguably should do as well. That's a bit more complex to add, so WIP for the simpler part for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/gateway-api Relating to upstream Kubernetes SIG Networking Gateway API nice-to-have size/small
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants