Duplicate resources when using implementation-specific annotations

[pottekkat]

What happened:

When converting Ingress resources with implementation-specific annotations, the output contains duplicated Gateway and HTTPRoute resources.

See #104 (comment) for details.

Also see: #104 (comment) and #104 (comment)

What you expected to happen:

How to reproduce it (as minimally and precisely as possible):

See #104 (comment) for reproducing.

Anything else we need to know?:

No.

[mlavacca]

/good-first-issue

[k8s-ci-robot]

@mlavacca:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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.

[pottekkat]

From a quick look, the issue seems to be because each provider calls the common ToGateway function. If that is the case, instead of changing the provider logic, we can just disable all providers by default and then user can enable any one provider, which would usually be the case.

[mlavacca]

From a quick look, the issue seems to be because each provider calls the common ToGateway function. If that is the case, instead of changing the provider logic, we can just disable all providers by default and then user can enable any one provider, which would usually be the case.

Similarly to what @levikobi suggested in their comment, I suggest adding the IngressClass name to the common converter function signature and letting the common machinery filter out resources for not matching ingress classes. @levikobi does it make sense to you?

[levikobi]

From a quick look, the issue seems to be because each provider calls the common ToGateway function. If that is the case, instead of changing the provider logic, we can just disable all providers by default and then user can enable any one provider, which would usually be the case.

Similarly to what @levikobi suggested in their comment, I suggest adding the IngressClass name to the common converter function signature and letting the common machinery filter out resources for not matching ingress classes. @levikobi does it make sense to you?

Absolutely @mlavacca, that's exactly what I meant 👍

[pottekkat]

In the provider code, we get all the gateway resources from the common ToGateway and the parse it and add implementation specific configuration to those resources which have custom annotations and return everything back. If the provider only gets a filtered gateway resources, then we will only return resources which has converted annotations? The rest will be filtered out because they aren't implementation specific?

Maybe I am missing something.

[levikobi]

@pottekkat pretty much yes, but after reading your comment I'm realizing it isn't as intuitive as it should be.

Maybe we should add a new function to the common package, which will filter ingresses based on class name. Every provider will call it before calling ToGateway.
It will be more understandable and also keeps ToGateway function with one responsibility.

Thoughts @mlavacca @pottekkat?

[mlavacca]

@pottekkat pretty much yes, but after reading your comment I'm realizing it isn't as intuitive as it should be.

Maybe we should add a new function to the common package, which will filter ingresses based on class name. Every provider will call it before calling ToGateway. It will be more understandable and also keeps ToGateway function with one responsibility.

Thoughts @mlavacca @pottekkat?

I agree with adding a dedicated filter function to the common package, but not on leaving implementations to call them. This is not optional, and I think it should not be on the providers' shoulders, but instead, something addressed without providers's awareness. For this reason, I think that the common.ToGateway function should call it, or at least introduce a mechanism to filter without asking providers do anything.

[levikobi]

I tend to agree with you @mlavacca, that's a good point IMO. I would also prefer not to let the implementations the extra "power" / "responsibility". Maybe we can call the filter function in the i2gw pkg, and only pass implementations their associated resources?

[LiorLieberman]

Thanks for flagging this. I think it is touching a topic we discussed on #99 with @dpasiukevich. More specifically this comment - #99 (comment).

The solution to this would be to also move ingresses to be fetched by the provider (perhaps every provider will query the ingresses with the relevant ingress classes it needs). Same as we do with CRDs.

Thoughts?
Any cons I did no think of if we are proceeding with this change?

[LiorLieberman]

/assign @LiorLieberman