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

Add validation that PodCIDR(s) matches the network specified in the net-conf.json #1556

Merged
merged 5 commits into from
May 12, 2022

Conversation

AlexPykavy
Copy link
Contributor

Faced issue that iptables forwarding rules have been created for the default subnet from the net-conf.json config while actual PodCIDR was different.

@AlexPykavy AlexPykavy force-pushed the master branch 2 times, most recently from af2b115 to f45c162 Compare April 8, 2022 11:47
@AlexPykavy
Copy link
Contributor Author

Hi @manuelbuil , unfortunately e2e tests failed due to timeout. I couldn't find any errors in the log, so assume that the failure was caused by the external load on the server when the build was running. Could you please restart that pipeline?

@manuelbuil
Copy link
Collaborator

Hi @manuelbuil , unfortunately e2e tests failed due to timeout. I couldn't find any errors in the log, so assume that the failure was caused by the external load on the server when the build was running. Could you please restart that pipeline?

Yes, let's try again :)

@AlexPykavy
Copy link
Contributor Author

AlexPykavy commented Apr 13, 2022

As it turned out, the problem was on my side and the tests helped to detect it. However troubleshooting them was quite difficult, so I decided to make some improvements.

@@ -453,3 +461,9 @@ func (ksm *kubeSubnetManager) setNodeNetworkUnavailableFalse(ctx context.Context
_, err = ksm.client.CoreV1().Nodes().PatchStatus(ctx, ksm.nodeName, patch)
return err
}

func ContainsCIDR(ipnet1, ipnet2 *net.IPNet) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you write a unit test around this function please

@luthermonson
Copy link
Contributor

@AlexPykavy write a unit test around your ContainsCIDR func and this is able to be merged.

…net-conf.json`

Because otherwise flannel will be not configured correctly.
For instance, in our case `iptables` forwarding rules have been created for
the default subnet from the `net-conf.json` config while actual PodCIDR
was different.

Creds to:
- https://stackoverflow.com/questions/40406230/net-ipnet-inside-other-net-ipnet
Because many users won't delve into documentation when
they see a one-line installation command in the `README.md`.
To make `/opt/bin/flanneld` the main process inside the container
and thereby be able to check its status and stop tests & extract logs
in case of the failure.
To make sure that it works as expected.
This will also enable `Test_newAnnotations` tests besides mine.
@AlexPykavy
Copy link
Contributor Author

@luthermonson , completely agree with you) Added as suggested, so could you please trigger a build to make sure that it works as we expect?

@AlexPykavy
Copy link
Contributor Author

@manuelbuil, @luthermonson, tests pass. Can we merge this?

@manuelbuil
Copy link
Collaborator

@manuelbuil, @luthermonson, tests pass. Can we merge this?

I'm fine. Waiting on @luthermonson

@luthermonson luthermonson merged commit da2a836 into flannel-io:master May 12, 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