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

A public ip on the farm public IP list cannot be the same as the gateway #869

Closed
muhamadazmy opened this issue Sep 25, 2023 · 7 comments · Fixed by #870
Closed

A public ip on the farm public IP list cannot be the same as the gateway #869

muhamadazmy opened this issue Sep 25, 2023 · 7 comments · Fixed by #870
Assignees
Labels
tfchain tfchain issue type_bug Something isn't working
Milestone

Comments

@muhamadazmy
Copy link
Member

Describe the bug

While investigating an issue with some node public ip not reachable we found out that the VM has been assigned a bad IP config where the Ip actually matches the gatway

image

This is an invalid setup IP validation must be hardened so that

  • The gateway IP must be in the same subnet as the IP
  • The gateway IP must not be same as the public ip
@muhamadazmy
Copy link
Member Author

Can we kick out bad ips that did not match this during migration ?

@renauter
Copy link
Contributor

2 things here:

  1. I checked in code and, indeed, when creating a farm (by calling create_farm()) the provided list of public ips is not checked in terms of validity what could have led to this situation
    So it requires some changes here

  2. Meanwhile, a validity check is provided here

    pub fn is_valid(&self) -> Result<(), PublicIpError> {

    and used when calling add_farm_ip()
    but is it enough for the following requested validation?

(a) The gateway IP must be in the same subnet as the IP

(b) The gateway IP must not be same as the public ip

I see in code that (a) is checked but not sure about (b)... what gateway IP must not be same as the public ip means exactly?

@renauter
Copy link
Contributor

Can we kick out bad ips that did not match this during migration ?

Perfectly doable yes.
I guess it is better to have nothing informed than to have some wrong data.

This make me think that we could simplify the creating farm flow.
Instead of being able to set public ips by calling create_farm(), inserting it as argument, and after being able to add public ips by calling add_farm_ip()...
Why not only allowing to add public ips by calling add_farm_ip() getting rid of this possibility in create_farm()?
@LeeSmet @muhamadazmy

@renauter renauter self-assigned this Sep 25, 2023
@muhamadazmy
Copy link
Member Author

I am not sure why IPs can also be provided during creations but validation of ips can also done in farm creation by passing the ip object to the validator method.

But i am also not sure if anything actually uses the create farm with the public IPs hence i can't decide if we can remove it without breaking some API

@renauter renauter added the type_bug Something isn't working label Sep 26, 2023
@renauter renauter added this to 3.12.x Sep 26, 2023
@renauter renauter moved this to In Progress in 3.12.x Sep 26, 2023
@renauter renauter added this to the 2.6.0 milestone Sep 26, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in 3.12.x Oct 3, 2023
@renauter renauter reopened this Oct 10, 2023
@renauter renauter moved this from Done to In Verification in 3.12.x Oct 10, 2023
@A-Harby
Copy link

A-Harby commented Oct 11, 2023

Verified, Devnet
addFarmIp(farmId, ip, gw) function have now a public ip validation,

The gateway IP must not be same as the public ip
image
The gateway IP must be in the same subnet as the IP
image

@A-Harby A-Harby closed this as completed Oct 11, 2023
@github-project-automation github-project-automation bot moved this from In Verification to Done in 3.12.x Oct 11, 2023
@A-Harby
Copy link

A-Harby commented Oct 11, 2023

I tried another case where the subnet is different, and IPs were added,
image
And I also tried adding public ips using createFarm(name, publicIps) and it was also added,
image
And same with addNodePublicConfig(farmId, nodeId, publicConfig).
image

@A-Harby A-Harby reopened this Oct 11, 2023
@A-Harby A-Harby moved this from Done to In Progress in 3.12.x Oct 11, 2023
@A-Harby A-Harby moved this from In Progress to In Verification in 3.12.x Oct 11, 2023
@A-Harby
Copy link

A-Harby commented Oct 12, 2023

I tried another case where the subnet is different, and IPs were added, image And I also tried adding public ips using createFarm(name, publicIps) and it was also added, image And same with addNodePublicConfig(farmId, nodeId, publicConfig). image

closing the issue as verified, since I formatted the subnet like this 125.25../16, all the gw with 125.25.. are contained in subnet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tfchain tfchain issue type_bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants