Skip to content
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.

Sign up for GitHub

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

Jump to bottom

IPContext merged in begin chain element (#1231) #1234

Closed
wants to merge 1 commit into from
Closed

IPContext merged in begin chain element (#1231) #1234

wants to merge 1 commit into from

Conversation

LionelJouin
Copy link
Member

Description

Using the Client chain, the network service client cannot update the IPContext without the mergeConnectionContext function implemented. A NSC might want to update it if it needs to add, update or delete routes, src IP addresses (VIPs...), policies...

Issue link

#1231

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

I am not sure what would be the full impact in NSM, but in my environment, I managed to update IPs and policies without any issue.

Types of changes

  • Bug fix
  • New functionality
  • Documentation
  • Refactoring
  • CI

@denis-tingaikin
Copy link
Member

@LionelJouin Could you resolve the linter issue and also sign the commit to fix DCO?

@denis-tingaikin
Copy link
Member

Also, I feel we need to add a unit test for this case. See at unit tests for begin pkg https://github.com/networkservicemesh/sdk/blob/main/pkg/networkservice/common/begin/serialize_both_test.go

@LionelJouin LionelJouin force-pushed the issue/1231 branch 2 times, most recently from aa1e171 to f054762 Compare March 3, 2022 13:57
@LionelJouin
IPContext merged in begin chain element (networkservicemesh#1231)
7a95650 
Using the Client chain, the network service client cannot update the
IPContext without the mergeConnectionContext function implemented. A NSC
might want to update it if it needs to add, update or delete routes, src
IP addresses (VIPs...), policies...

Signed-off-by: Lionel Jouin <lionel.jouin@est.tech>
denis-tingaikin
denis-tingaikin reviewed Mar 8, 2022
View reviewed changes
pkg/networkservice/common/begin/merge.go
@@ -36,7 +37,8 @@ func mergeConnection(returnedConnection, requestedConnection, connection *networ
func mergeConnectionContext(returnedConnectionContext, requestedConnectionContext, connectioncontext *networkservice.ConnectionContext) *networkservice.ConnectionContext {
rv := proto.Clone(connectioncontext).(*networkservice.ConnectionContext)
if !proto.Equal(returnedConnectionContext, requestedConnectionContext) {
// TODO: IPContext, DNSContext, EthernetContext, do we need to do MTU?
// TODO: DNSContext, EthernetContext, do we need to do MTU?
rv.IpContext = requestedConnectionContext.IpContext
Copy link
Member

@denis-tingaikin denis-tingaikin Mar 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think we also need to do it for other contexts as you pointed in the comment.

@edwarnicke Thoughts?

Copy link
Member

@edwarnicke edwarnicke Mar 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LionelJouin Apologies for this taking so long to get back to you on. The net net is that what you did here was fine. I also realized why it took me so long to sort it out, and produced a better documented version here in #1238 that handles the merge more or less correctly (which you were doing) and also in a way that's hopefully clearer. I also provided testing. Could you have a look?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem, I will try it today, thank you

@LionelJouin LionelJouin mentioned this pull request Mar 10, 2022
Merged
@LionelJouin LionelJouin requested a review from ljkiraly March 11, 2022 16:19
@edwarnicke edwarnicke mentioned this pull request Mar 13, 2022
Open
9 tasks
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
613b055 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
9805c46 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
2c29e04 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
593c14e 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
0d25599 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Rework begin to simplify merge of Context
b358632 
This is a simplification and rework in response to networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 13, 2022
@edwarnicke
Improve merge of ConnectionContext with external requests
566543d 
Also improved greatly documentation in various places

Should resolve networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
@edwarnicke edwarnicke mentioned this pull request Mar 13, 2022
Merged
9 tasks
edwarnicke added a commit to edwarnicke/sdk that referenced this pull request Mar 14, 2022
@edwarnicke
Improve merge of ConnectionContext with external requests
81a36b5 
Also improved greatly documentation in various places

Should resolve networkservicemesh#1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
denis-tingaikin pushed a commit that referenced this pull request Mar 14, 2022
@edwarnicke
Improve merge of ConnectionContext with external requests (#1238)
7296982 
Also improved greatly documentation in various places

Should resolve #1234

Signed-off-by: Ed Warnicke <hagbard@gmail.com>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
0336c61 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
3d3d4b7 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
6e7ed76 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
410fad6 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-admission-webhook-k8s that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
5002368 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
47d9d29 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
754e954 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-remote-vlan that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
82c81c3 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
0b79f39 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Mar 14, 2022
Update go.mod and go.sum to latest version from networkservicemesh/sd…
b9096b0 
…k@main

PR link: networkservicemesh/sdk#1238

Commit: 7296982
Author: Ed Warnicke
Date: 2022-03-13 20:33:48 -0500
Message:
  - Improve merge of ConnectionContext with external requests (#1238)
Also improved greatly documentation in various places

Should resolve networkservicemesh/sdk#1234

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
This was referenced Mar 14, 2022
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Merged
@denis-tingaikin denis-tingaikin mentioned this pull request Mar 14, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denis-tingaikin denis-tingaikin denis-tingaikin left review comments

@edwarnicke edwarnicke Awaiting requested review from edwarnicke

@ljkiraly ljkiraly Awaiting requested review from ljkiraly

Assignees
No one assigned
Labels
None yet
Projects
Release 1.3.0
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@LionelJouin @denis-tingaikin @edwarnicke