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

Improve documentation for publishedService and IP options #9380

Merged
merged 4 commits into from Sep 29, 2022

Conversation

samip5
Copy link
Contributor

@samip5 samip5 commented Sep 27, 2022

What does this PR do?

Makes it more clear what the kubernetes-ingress options actually do. like ip and publishedService.

Motivation

Hackaeton and the long-lasting issue #8406.

Fixes #8406

More

  • Added/updated documentation

Closes traefik#8406.

Signed-off-by: Skyler Mäntysaari <samip5@users.noreply.github.com>
@samip5 samip5 marked this pull request as ready for review September 27, 2022 17:18
@rtribotte rtribotte added this to To review in v2 via automation Sep 27, 2022
@ldez ldez added this to the 2.8 milestone Sep 27, 2022
Copy link

@bwbrock bwbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes appear good.

docs/content/providers/kubernetes-ingress.md Outdated Show resolved Hide resolved
docs/content/providers/kubernetes-ingress.md Outdated Show resolved Hide resolved
@kevinpollet kevinpollet changed the title feat(docs/kubernetes-ingress): Make more clear what options do Improve documentation for publishedService and IP Kubernetes options Sep 28, 2022
@kevinpollet kevinpollet changed the title Improve documentation for publishedService and IP Kubernetes options Improve documentation for publishedService and IP options Sep 28, 2022
Co-authored-by: Romain <rtribotte@users.noreply.github.com>
docs/content/providers/kubernetes-ingress.md Outdated Show resolved Hide resolved
docs/content/providers/kubernetes-ingress.md Outdated Show resolved Hide resolved
Co-authored-by: Kevin Pollet <pollet.kevin@gmail.com>
Copy link
Member

@kevinpollet kevinpollet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

Copy link
Member

@rtribotte rtribotte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

Co-authored-by: mpl <mathieu.lonjaret@gmail.com>
@traefiker traefiker merged commit f75f636 into traefik:v2.8 Sep 29, 2022
v2 automation moved this from To review to Done Sep 29, 2022
@samip5
Copy link
Contributor Author

samip5 commented Feb 26, 2024

Hi @kevinpollet. I'm little sad that you modified the issue description to add the "fixes" as this doesn't in reality fix the underlying bug which is in using Endpoints instead of EndpointSlices. The bug itself is still in the code.

@kevinpollet
Copy link
Member

Hello @samip5, and sorry for that, I missed something.
Could you please open a detailed issue, describing the bug with a reproduction case?

@samip5
Copy link
Contributor Author

samip5 commented Mar 5, 2024

Hello @samip5, and sorry for that, I missed something. Could you please open a detailed issue, describing the bug with a reproduction case?

It's right there #8406 (comment) and has been for multiple years.

@kevinpollet
Copy link
Member

Hello @samip5,

If I am not mistaken, issue #8406 was about copying the LoadBalancer service IPs to the Ingress status. This is working as expected by using the publishedService option and confirmed in the following comment (#8406 (comment)). This is why I closed the issue, after the clarification you made in the documentation.

The following comment #8406 (comment) is about the IPs used for load balancing, which seems to contain only one IP Family. If there is an issue with IPv6 or dual stack clusters, it would be easier to open a new issue to be able to address it properly.

Could you please open this new issue describing the problem?

@samip5
Copy link
Contributor Author

samip5 commented Mar 6, 2024

Hello @samip5,

If I am not mistaken, issue #8406 was about copying the LoadBalancer service IPs to the Ingress status. This is working as expected by using the publishedService option and confirmed in the following comment (#8406 (comment)). This is why I closed the issue, after the clarification you made in the documentation.

The following comment #8406 (comment) is about the IPs used for load balancing, which seems to contain only one IP Family. If there is an issue with IPv6 or dual stack clusters, it would be easier to open a new issue to be able to address it properly.

Could you please open this new issue describing the problem?

You're unfortunately mistaken, as the original one was also about EndpointSlices (currently the copying only copies the first IP family due to the issue of not using EndpointSlices aka #8406 (comment)), not just copying the status thus it shouldn't have been closed when the PR was merged for docs.

@emilevauge
Copy link
Member

Hi @samip5
I propose that you open a specific issue to explain your problem.
Thank you

@samip5
Copy link
Contributor Author

samip5 commented Mar 8, 2024

Hi @samip5 I propose that you open a specific issue to explain your problem. Thank you

It already has been explained (multiple times, mind you, even in the Hackathon), but no matter how I try to explain it, it seems maintainers will not understand my point, so I will rather just not use Traefik.

Maybe @aojea wants to explain the problem, but I have already tried to no avail.

@emilevauge
Copy link
Member

emilevauge commented Mar 8, 2024

I'm sorry, but that's how most open source project works: to report problems through issues.
This is the only way to avoid confusion and understand your use case (which is not the case today).
Don't hesitate to come back later if you want to report bugs or contribute :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v2
Done
Development

Successfully merging this pull request may close these issues.

None yet

8 participants