Initial cmd state #3

rejmond · 2022-05-10T12:09:29Z

Signed-off-by: Sergey Shlyanin sergey.shlyanin@xored.com

Description

Clone cmd-nse-icmp-responder repository content and rename package to nse-istio-proxy.

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

edwarnicke · 2022-05-10T13:39:09Z

main.go

+	ConnectTo             url.URL           `default:"unix:///var/lib/networkservicemesh/nsm.io.sock" desc:"url to connect to" split_words:"true"`
+	MaxTokenLifetime      time.Duration     `default:"10m" desc:"maximum lifetime of tokens" split_words:"true"`
+	ServiceNames          []string          `default:"istio-proxy-responder" desc:"Name of provided services" split_words:"true"`
+	Payload               string            `default:"ETHERNET" desc:"Name of provided service payload" split_words:"true"`


Payload should always be IP for this NSE.

Done. IP contants is used now in the registry client.

edwarnicke · 2022-05-10T13:40:11Z

main.go

+	Payload               string            `default:"ETHERNET" desc:"Name of provided service payload" split_words:"true"`
+	Labels                map[string]string `default:"" desc:"Endpoint labels"`
+	DNSConfigs            dnstools.Decoder  `default:"[]" desc:"DNSConfigs represents array of DNSConfig in json format. See at model definition: https://github.com/networkservicemesh/api/blob/main/pkg/api/networkservice/connectioncontext.pb.go#L426-L435" split_words:"true"`
+	CidrPrefix            []string          `default:"169.254.0.0/16" desc:"List of CIDR Prefix to assign IPv4 and IPv6 addresses from" split_words:"true"`


https://github.com/networkservicemesh/api/blob/main/pkg/api/networkservice/connectioncontext.pb.go#L426-L435 will move as main moves, use an absolute reference to a tag or commit for this comment.

edwarnicke · 2022-05-10T13:40:35Z

main.go

+	DNSConfigs            dnstools.Decoder  `default:"[]" desc:"DNSConfigs represents array of DNSConfig in json format. See at model definition: https://github.com/networkservicemesh/api/blob/main/pkg/api/networkservice/connectioncontext.pb.go#L426-L435" split_words:"true"`
+	CidrPrefix            []string          `default:"169.254.0.0/16" desc:"List of CIDR Prefix to assign IPv4 and IPv6 addresses from" split_words:"true"`
+	IdleTimeout           time.Duration     `default:"0" desc:"timeout for automatic shutdown when there were no requests for specified time. Set 0 to disable auto-shutdown." split_words:"true"`
+	RegisterService       bool              `default:"true" desc:"if true then registers network service on startup" split_words:"true"`


When would this be false?

Done. Removed this option.

edwarnicke · 2022-05-10T13:41:01Z

main.go

+	CidrPrefix            []string          `default:"169.254.0.0/16" desc:"List of CIDR Prefix to assign IPv4 and IPv6 addresses from" split_words:"true"`
+	IdleTimeout           time.Duration     `default:"0" desc:"timeout for automatic shutdown when there were no requests for specified time. Set 0 to disable auto-shutdown." split_words:"true"`
+	RegisterService       bool              `default:"true" desc:"if true then registers network service on startup" split_words:"true"`
+	PBRConfigPath         string            `default:"/etc/policy-based-routing/config.yaml" desc:"Path to policy based routing config file" split_words:"true"`


We aren't doing configurable PBR for this NSE.

Done. Removed all PBR related functions and variables.

edwarnicke · 2022-05-10T13:42:22Z

main.go

+	log.FromContext(ctx).Infof("the phases include:")
+	log.FromContext(ctx).Infof("1: get config from environment")
+	log.FromContext(ctx).Infof("2: retrieve spiffe svid")
+	log.FromContext(ctx).Infof("3: create icmp server ipam")


We should fix the text here as its not icmp server. I'd suggest just changing it to 'create ipam' (and below to 'create nse'

@rejmond Could you resolve the comment?

edwarnicke · 2022-05-10T13:43:09Z

main.go

+}
+
+// newPolicyRoutesGetter - watches the config file and dynamically updates policy routes.
+func newPolicyRoutesGetter(ctx context.Context, configPath string) *policyRoutesGetter {


This whole function is probably unneeded in this NSE.

edwarnicke · 2022-05-10T13:43:27Z

main.go

+	return p
+}
+
+func (p *policyRoutesGetter) Get() []*networkservice.PolicyRoute {


This whole function is probably unneeded in this NSE

edwarnicke · 2022-05-10T13:43:39Z

main.go

+	return p.policyRoutes.Load().([]*networkservice.PolicyRoute)
+}
+
+type policyRoutesGetter struct {


This is probably unneeded in this NSE

rejmond · 2022-05-10T15:29:48Z

@edwarnicke Thank you for your comments, I will check everything.

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

denis-tingaikin

@rejmond Please resolve @edwarnicke comment and this can be merged. Thanks.

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

denis-tingaikin · 2022-05-13T14:40:31Z

@edwarnicke Let us know if you still have any comments on this then we'll resolve them in separate PR.

Sergey Shlyanin added 2 commits May 10, 2022 13:35

Clone cmd-nse-icmp-responder repository

d9cf229

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

Rename icmp-responder to istio-proxy

f32fb78

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

edwarnicke reviewed May 10, 2022

View reviewed changes

rejmond marked this pull request as draft May 10, 2022 15:14

Sergey Shlyanin added 4 commits May 11, 2022 15:12

Remove unused configs

17470e0

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

Add dns module, update dependencies

fdce2ec

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

Update sdk-kernel version

a5e587e

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

Add license and comments for DNS package

6ee5940

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

rejmond force-pushed the icmp-clone branch 11 times, most recently from 1be608c to ce964d6 Compare May 13, 2022 14:00

Fix linter

9885017

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

rejmond force-pushed the icmp-clone branch from ce964d6 to 9885017 Compare May 13, 2022 14:05

rejmond marked this pull request as ready for review May 13, 2022 14:09

denis-tingaikin approved these changes May 13, 2022

View reviewed changes

Sergey Shlyanin added 2 commits May 13, 2022 21:25

Fixes

8139195

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

Fix default server name

edec936

Signed-off-by: Sergey Shlyanin <sergey.shlyanin@xored.com>

denis-tingaikin merged commit 1d61034 into networkservicemesh:main May 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial cmd state #3

Initial cmd state #3

rejmond commented May 10, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

denis-tingaikin May 13, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

edwarnicke May 10, 2022

rejmond May 13, 2022

rejmond commented May 10, 2022

denis-tingaikin left a comment •

edited

Loading

denis-tingaikin commented May 13, 2022

Initial cmd state #3

Initial cmd state #3

Conversation

rejmond commented May 10, 2022

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rejmond commented May 10, 2022

denis-tingaikin left a comment • edited Loading

Choose a reason for hiding this comment

denis-tingaikin commented May 13, 2022

denis-tingaikin left a comment •

edited

Loading