-
Notifications
You must be signed in to change notification settings - Fork 632
Allow IPv6 for network blackhole port fault API #4629
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
Conversation
@@ -141,6 +145,8 @@ func (h *FaultHandler) StartNetworkBlackholePort() func(http.ResponseWriter, *ht | |||
return | |||
} | |||
|
|||
isIPv6OnlyTask := isIPv6OnlyTask(taskMetadata) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is this variable necessary? Can we pass in isIPv6OnlyTask(taskMetadata)
directly on line 181?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I can do that in a follow up change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in a new rev.
@@ -542,7 +601,7 @@ func (h *FaultHandler) CheckNetworkBlackHolePort() func(http.ResponseWriter, *ht | |||
} | |||
} | |||
|
|||
// checkNetworkBlackHolePort will check if there's a running black hole port within the task network namespace based on the chain name and the passed in required request fields. | |||
// checkNetworkBlackHolePort will check if there's a running black hole port within the task network namespace based on the chain in IPv4 tables. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit(non-blocking): might be helpful to provide a reason why we can just rely on checking the Ipv4 route tables to see if a BHP fault is running.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will add that in a new rev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in a new rev.
|
||
func IsIPv6(s string) bool { | ||
parsedIP := net.ParseIP(s) | ||
return parsedIP != nil && parsedIP.To4() == nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want parsedIP.To4() == nil
and not parsedIP.To16() != nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To16 func will return non-empty value for IPv4 addr - https://go.dev/play/p/sBjowEPblpW
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wow
* Allow IPv6 for network blackhole port fault API * Fail network blackhole port API for IPv6 only tasks * Update comments
* Allow IPv6 for network blackhole port fault API * Fail network blackhole port API for IPv6 only tasks * Update comments
* Allow IPv6 for network blackhole port fault API * Fail network blackhole port API for IPv6 only tasks * Update comments
Summary
This change is to allow drop packets for both IPv6 and IPv4 in the network blackhole port API in TMDS.
For IPv4 only or dual stack tasks, we will make IPv6 updates on a best effort basis. For IPv6 only tasks, both IPv4 and IPv6 updates are required.
Implementation details
SourcesToFiler
field in the request bodyTesting
New tests cover the changes:
yes
manual testing
Description for the changelog
Additional Information
Does this PR include breaking model changes? If so, Have you added transformation functions?
No
Does this PR include the addition of new environment variables in the README?
No
Licensing
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.