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

network-status: add GW info #42

Merged

Conversation

maiqueb
Copy link
Contributor

@maiqueb maiqueb commented Nov 22, 2021

Hold this PR until default-route feature is fixed in spec (multi-net-spec) and multus-cni (comment by @s1061123, Nov 22, 2021)

--
The GW information is missing from the NetworkStatus annotation.

As such, it isn't possible to know which interface currently holds the default route.

@maiqueb
Copy link
Contributor Author

maiqueb commented Nov 22, 2021

/cc @s1061123 / @dougbtv / @phoracek

@s1061123
Copy link
Member

@maiqueb thank you for the PR, however can we hold this PR for now?
Currently we have some gap about default-route annotation, between spec (multi-net-spec) and implementation and we don't clarify how to fix it.

So we need to fix default-route annotation in spec and multus then proceed the PR.
Thank you so much for your help in advance.

@s1061123 s1061123 changed the title network-status: add GW info [hold]network-status: add GW info Nov 22, 2021
@maiqueb
Copy link
Contributor Author

maiqueb commented Nov 22, 2021

@maiqueb thank you for the PR, however can we hold this PR for now? Currently we have some gap about default-route annotation, between spec (multi-net-spec) and implementation and we don't clarify how to fix it.

So we need to fix default-route annotation in spec and multus then proceed the PR. Thank you so much for your help in advance.

Of course. I just wanted to kick this off the ground, and this seemed the easiest way ( to me ...) .

@dougbtv
Copy link
Member

dougbtv commented May 11, 2022

Turns out that we actually had the examples in the spec! I went to go make a spec update last week, and... Alas, it's already there.

Then... I realize Miguel already has the code here :D Amazing! So, I think we should probably be ready to merge.

@dougbtv
Copy link
Member

dougbtv commented May 11, 2022

I think we might also want to merge against the release-v1.1 branch, too

Copy link
Member

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

Actually, shoot, we need this

pkg/apis/k8s.cni.cncf.io/v1/types.go Outdated Show resolved Hide resolved
@maiqueb maiqueb changed the title [hold]network-status: add GW info network-status: add GW info May 11, 2022
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@maiqueb maiqueb force-pushed the add-gateway-to-network-status branch from 48e9d79 to 4df3b50 Compare May 11, 2022 16:28
@maiqueb maiqueb requested a review from dougbtv July 28, 2022 12:37
@maiqueb
Copy link
Contributor Author

maiqueb commented Jul 28, 2022

@dougbtv can we revisit this ? AFAIU, the IPv6 thing was addressed in multus.

If not, I would appreciate if this PR were updated with the reasons it is being held. Because honestly, right now ... I don't know why.

@dougbtv dougbtv merged commit 9679314 into k8snetworkplumbingwg:master Oct 20, 2022
@maiqueb maiqueb deleted the add-gateway-to-network-status branch October 20, 2022 14:19
@ykulazhenkov ykulazhenkov mentioned this pull request Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants