You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This PR is to fix dualnic options to avoid overwritting vlanMaps
In the windows dualnic multitenancy case, we receive two interface infos from cns and should create two corresponding networks and endpoints. The first and second endpoints created by hns should have vlan id 1 and vlan id 2, respectively. It is CNI's job to parse the cns input and send a request to hns to create the endpoints.
However, during testing both endpoints have vlan id 2. This is because the ipamAddConfig's options field is unintentionally shared between each EndpointInfo struct. Modifying the vlan id of one options field in an endpoint info struct inadvertently modifies every other endpoint info struct's options. So, the vlan id reflected during endpoint creation is of the last endpoint info's vlan id only, as all endpoint info option fields point to the same underlying object in memory.
The proposed solution is to, for each endpoint info we create, we shallow copy the ipamAddConfig's options field. So, every endpoint info gets its own copy of the options field which it can modify and store without modifying the other endpoint infos.
If the options field of type map[string]interface{} ever holds anything other than primitives before we perform the shallow copy (ex: if we store pointers to another object as a value in the map) the copied pointer will still be pointing to the same object and we will have the same issue, but as of now we have only identified a single instance where we write to the options field in invoker_cns.go (ncPrimaryIP) where the value is of type string, which is immutable.
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add the vlan id to ep info pretty print (endpoint.go) or similar pretty string (should not panic/err if no vlan id is present/map is nil)?
Is it possible to add the vlan id to ep info pretty print (endpoint.go) or similar pretty string (should not panic/err if no vlan id is present/map is nil)?
we have already printed vlanID when setting options (key and value)
Is it possible to add the vlan id to ep info pretty print (endpoint.go) or similar pretty string (should not panic/err if no vlan id is present/map is nil)?
we have already printed vlanID when setting options (key and value)
At EndpointCreate, after we generate all epInfos, we print each endpointInfo. If the pretty string function printed the options map (similar to how we print the data map) this would catch the vlan id issue. The log statement in the setting options function would not catch this because it only prints the value for the current iteration and does not print out the previous values of epInfo vlan id to confirm that it has not been improperly modified.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change:
This PR is to fix dualnic options to avoid overwritting vlanMaps
In the windows dualnic multitenancy case, we receive two interface infos from cns and should create two corresponding networks and endpoints. The first and second endpoints created by hns should have vlan id 1 and vlan id 2, respectively. It is CNI's job to parse the cns input and send a request to hns to create the endpoints.
However, during testing both endpoints have vlan id 2. This is because the ipamAddConfig's
optionsfield is unintentionally shared between eachEndpointInfostruct. Modifying the vlan id of one options field in an endpoint info struct inadvertently modifies every other endpoint info struct's options. So, the vlan id reflected during endpoint creation is of the last endpoint info's vlan id only, as all endpoint info option fields point to the same underlying object in memory.The proposed solution is to, for each endpoint info we create, we shallow copy the
ipamAddConfig's options field. So, every endpoint info gets its own copy of the options field which it can modify and store without modifying the other endpoint infos.If the options field of type map[string]interface{} ever holds anything other than primitives before we perform the shallow copy (ex: if we store pointers to another object as a value in the map) the copied pointer will still be pointing to the same object and we will have the same issue, but as of now we have only identified a single instance where we write to the options field in
invoker_cns.go(ncPrimaryIP) where the value is of type string, which is immutable.Issue Fixed:
Requirements:
Notes:
To be tested by partner team(s) before being merged.