-
Notifications
You must be signed in to change notification settings - Fork 9
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
Dualstack Network Support #549
base: master
Are you sure you want to change the base?
Conversation
…o ipv6-support
…o ipv6-support
5919c8f
to
0c53831
Compare
0c53831
to
2389965
Compare
…o dualstack-support
d49d2c0
to
b65c835
Compare
23a3d1e
to
89096c0
Compare
…o dualstack-support
a3571a2
to
d9667d3
Compare
if n == nil || n.network == nil { | ||
continue | ||
} | ||
if len(n.network.AddressFamilies) == 0 { |
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.
I don't know if this is possible but can this be dynamically evaluated where the network was retrieved?
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.
This simply guards if the network has not AF set
@@ -439,6 +456,79 @@ func validateAdditionalAnnouncableCIDRs(additionalCidrs []string, privateSuper b | |||
return nil | |||
} | |||
|
|||
func validatePrefixesAndAddressFamilies(prefixes, destinationPrefixes []string, defaultChildPrefixLength metal.ChildPrefixLength, privateSuper bool) (metal.Prefixes, metal.Prefixes, metal.AddressFamilies, error) { |
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.
Function signature does not imply the return values. Could be better to split validation functions from parsing functions to increase reusability.
if err != nil { | ||
return nil, nil, nil, err | ||
} | ||
// all DestinationPrefixes must be valid and from the same addressfamily |
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.
Is this still correct? Because I see different address families in the destination prefixes in our test environment.
|
||
prefixes = append(prefixes, *prefix) | ||
var childPrefixLength = metal.ChildPrefixLength{} | ||
for af, length := range requestPayload.DefaultChildPrefixLength { |
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.
Either this needs only to be done for private super or the validation must be extended to validate this field for non-private super networks, too.
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.
done
return nil, nil, nil, fmt.Errorf("private super network must always contain a defaultchildprefixlength") | ||
} | ||
|
||
for af, length := range defaultChildPrefixLength { |
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.
It would also be good to check if the address family is actually IPv4 or IPv6.
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.
also done
return nil, err | ||
} | ||
if childPrefix == nil { | ||
return nil, fmt.Errorf("could not allocate child prefix in parent network: %s for addressfamily: %s length:%d", parent.ID, af, childLength) |
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.
Hard to mitigate this but this is a potential leakage in case one child prefix could already be allocated.
Closes #164
Actions Required
Depends on:
replaces #544
TODO: