types: Deprecate MachineCIDR, replace with MachineNetwork: [] #2829
Conversation
openshift-ci-robot
added
the
size/L
|
/assign @abhinavdahiya |
| case c.Platform.AWS != nil: | ||
| case c.Platform.Azure != nil: | ||
| case c.Platform.BareMetal != nil: | ||
| case c.Platform.None != nil: |
There was a problem hiding this comment.
There was a problem hiding this comment.
Is that easier to switch on? Then I have two constants?
|
I’m still cleaning this up a bit, just didn’t want to steer too far away api wise before getting an eyeball. |
smarterclayton
force-pushed
the
machine_network
branch
from
c4c62a9 to
9990481
Compare
openshift-ci-robot
added
size/XL
smarterclayton
force-pushed
the
machine_network
branch
from
9990481 to
c9d5e39
Compare
| } | ||
| presence = make(IPAddressTypeByField) | ||
| for k, ips := range addresses { | ||
| sort.Slice(ips, func(i, j int) bool { return bytes.Compare(ips[i], ips[j]) < 0 }) |
There was a problem hiding this comment.
golang sometimes represents IPv4 addresses as 4 bytes, and sometimes as 16 bytes using the IPv6-compatible representation of IPv4 addresses. While you probably only have addresses in the 16-byte format here, it might be better to guarantee that (eg by calling .To16() on each one as you fill in addresses above).
There was a problem hiding this comment.
is there any case where it won't be one or the other?
There was a problem hiding this comment.
(where it would be both)?
There was a problem hiding this comment.
net.ParseIP always returns 16-byte addresses, but other code between there and here might have decided to replace some of the 16-byte addresses with their .To4() forms. I don't have any specific reason for thinking that might have happened here, it's just that the direct byte comparison looks wrong to me since it's not safe in the fully-general case.
There was a problem hiding this comment.
In the net module Go is using To4 to check IPv4 ness
// DefaultMask returns the default IP mask for the IP address ip.
// Only IPv4 addresses have default masks; DefaultMask returns
// nil if ip is not a valid IPv4 address.
func (ip IP) DefaultMask() IPMask {
if ip = ip.To4(); ip == nil {
return nil
}
If someone gives us the IPv6 representation of an IPv4, (::hhhh:hhhh), they'd have a zero CIDR prefix. We could probably just check for that, but that sounds... rare.
There was a problem hiding this comment.
Sorry, what I'm trying to say is that net.IP has two different-but-equivalent representations of IPv4 addresses, and if you're looking at the []byte representation directly rather than using net.IP APIs, you might not get the answers you expect:
ip := net.ParseIP("1.2.3.4")
ip.Equal(ip.To4()) → true
bytes.Equal(ip, ip.To4()) → false
There was a problem hiding this comment.
him, according to this code and code we have in Kube To4() means "a 4 address" - so if those other places are wrong we need to fix them too.
smarterclayton
force-pushed
the
machine_network
branch
from
c9d5e39 to
e953538
Compare
|
Updated with all feedback |
| switch { | ||
| case hasIPv4 && hasIPv6: | ||
| if c.Networking.NetworkType == "OpenShiftSDN" { | ||
| allErrs = append(allErrs, field.Invalid(field.NewPath("networking", "networkType"), c.Networking.NetworkType, "dual-stack IPv4/IPv6 is not supported for this networking plugin")) |
There was a problem hiding this comment.
field.NewPath("networking",
might be usefult to dry this up a little...
There was a problem hiding this comment.
What did you have in mind?
For IPv6 dual-stack support we need to support multiple machine networks in order to register both an IPv4 machine prefix and an IPv6 prefix (Azure in particular requires the user to define a prefix which could conflict with the service or pod networks). Update core types and validation to react to the change from a single field to an array. Update the validation messages to be consistent with the field the user would see (a "machine network") instead of using "subnets" or "machineCIDR".
Like the change to the core, update validation messages to indicate the specified IPs must be within one of the networks in preparation for dual-stack support. Update the noproxy settings to include all machine and service networks, not just the first ones.
smarterclayton
force-pushed
the
machine_network
branch
from
e953538 to
96e39a0
Compare
Add validation that checks the networking section of install-config.yaml for consistent with regards to IPv6 and dual-stack support. Rules: * If any networking list (service, pod, or machine) has IPv6 and IPv4 CIDRs, all fields must have both sets of addresses * Except on AWS, which allows the machine field to only have a single IPv4 CIDR * Only BareMetal and None can provide dual-stack or ipv6 addresses * Will be changed by forthcoming PRs that add support for AWS and Azure * Other platforms may have a toleration mode for UPI that will be relaxed in 4.4 * Block OpenShiftSDN when IPv6 is requested since it is not implemented * ServiceNetwork must always have 1 or 2 entries, for single or dual stack These rules may be altered by subsequent changes if we need to relax requirements. A helper method is provided to determine which mode is requested for a given install-config which should be used during asset generation per platform. Relax validation on SubnetCIDR and move logic to this new function.
smarterclayton
force-pushed
the
machine_network
branch
from
96e39a0 to
832a6b5
Compare
|
@abhinavdahiya feedback addressed (or question responded) |
|
Can i request updating the docs https://github.com/openshift/installer/blob/master/docs/user/customization.md#platform-customization ? |
|
Sure |
|
Docs updated |
|
/test all |
Change the public documentation to match the new name.
|
Forgot to push the docs, now pushed /assign @jhixson74 for final review since abhinav is out |
|
/test e2e-aws |
|
/test e2e-azure |
|
/lgtm |
|
It looks like all identified concerns have been addressed. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74, sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhixson74, sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@smarterclayton: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Replace MachineCIDR in UPI documentation with MachineNetwork: [] Ref: openshift#2829
A previous change referenced the cluster network instead of the machine network. ref: openshift#2829 ref: openshift#2892
Better support future IPv6 and dual stack by allowing some platforms
to accept multiple machine CIDRs. Add validation logic that checks
for some known IPv6 behaviors at the same time.