Support passing multiple ips via pod network annotation #204
Conversation
Pull Request Test Coverage Report for Build 576
💛 - Coveralls |
| if err != nil { | ||
| return nil, logging.Errorf("failed to parse CIDR %q", ipRequest) | ||
| } | ||
| } else if net.ParseIP(ipRequest) == nil { |
There was a problem hiding this comment.
Can you please do something like this
if ! strings.Contains(ipRequest, "/") {
ipRequest += "/32"
}
_, _, err := net.ParseCIDR(ipRequest)
if err != nil {
return nil, logging.Errorf("failed to parse CIDR %q", ipRequest)
}There was a problem hiding this comment.
According to Dan's comment #195 (comment) , there is a use case for an ip with no prefix.
There was a problem hiding this comment.
@SchSeba yeah, some plugins (host-local) already have the prefix in other configuration info, so they don't require it in the IPRequest (because it would be redundant for them). We also need to update the NPWG spec to clarify behavior around the prefix.
So for now I think we need to keep the prefix optional in Multus, but a given sub-plugin could fail if it doesn't get one.
There was a problem hiding this comment.
@dcbw Is this behaviour is clarified in the spec regarding the prefix ?
| var ips string | ||
| for _, ipRequest := range delegate.IPsRequest { | ||
| // validate IP address | ||
| if strings.Contains(ipRequest, "/") { |
AlonaKaplan
force-pushed
the
support_multiple_ips
branch
from
092c436
to
40690cc
Compare
|
In general LGTM, just a few small comments. |
AlonaKaplan
force-pushed
the
support_multiple_ips
branch
from
40690cc
to
dc9dcdf
Compare
|
@dcbw can you please re-review? |
SchSeba
force-pushed
the
support_multiple_ips
branch
from
dc9dcdf
to
b838399
Compare
|
Hi @dcbw I think this PR is ready to be merge there is anything that is missing? |
|
Hi @dcbw can we merge this PR? |
|
Hi @s1061123 can you please review this PR and tell me if there is anything else that I need to add for this PR to be merge? |
s1061123
force-pushed
the
master
branch
2 times, most recently
from
1b0b39d
to
c319f6b
Compare
|
We believe that recent improvements have added similar functionality. |
…nsistency-openshift-4.16-multus-cni OCPBUGS-24863: Updating multus-cni-container image to be consistent with ART
This PR is a follow-up to #195
The PR changes the code to accept array of strings (and not a single string) for the "ips" on the pod network annotation, as the de-facto standard specifies.
The ips will be passed to the relevant cni plugin over CNI_ARGS with the following format - "IP=10.0.0.1,2001:db8::5"