Add support for local machine ID #91

daniel-de-vera · 2023-11-07T20:56:48Z

This is in the context of issue https://github.com/signadot/signadot/issues/3837.
Runs on top of the following PRs: https://github.com/signadot/signadot/pull/3857, https://github.com/signadot/libconnect/pull/38, signadot/go-sdk#36

scott-cotton

started the review.

Some comments/questions:

Should we remove the api related stuff from internal/config/locald.go?
Should we remove the grpc/go-sdk model conversions?
Should we detect operator version and error out on old operator appropriately (since local is beta)?

daniel-de-vera · 2023-11-09T14:21:27Z

started the review.

Some comments/questions:

Should we remove the api related stuff from internal/config/locald.go?

Not sure, I understand what you mean by that, can you expand?

Should we remove the grpc/go-sdk model conversions?

Do you mean across the whole CLI? (what would look like a massive change to me).

Should we detect operator version and error out on old operator appropriately (since local is beta)?

I think that's a good idea.

scott-cotton · 2023-11-09T17:00:01Z

Should we remove the grpc/go-sdk model conversions?

Do you mean across the whole CLI? (what would look like a massive change to me).

I renamed these funcs:

23-11-09 scott@air cli % git diff | grep func
-func ToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error) {
+func xToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error) {
@@ -23,7 +23,7 @@ func ToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error)
-func ToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
+func xToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
@@ -32,7 +32,7 @@ func ToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
-func ToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
+func xToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
@@ -42,7 +42,7 @@ func ToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
-func ToModelsSandbox(grpcSandbox *structpb.Struct) (*models.Sandbox, error) {
+func xToModelsSandbox(grpcSandbox *structpb.Struct) (*models.Sandbox, error) {

in internal/locald/api/sandboxmanager/convert and it compiles fine, so they are now unused.

scott-cotton · 2023-11-09T17:03:54Z

Should we remove the api related stuff from internal/config/locald.go?

Not sure, I understand what you mean by that, can you expand?

Sorry, it was too vague. Was thinking that with the removal of sandbox apply from sandbox manager, we would probably not need the api key nor initialise APIConfig inside of anything in locald. I thought I remembered adding this CIConfig api support specifically for the sandbox apply proxy, and greping around it looks to me like we can remove APIKey from the CIConfig struct, as well as related code setting up the config for a *manager to communicate with the API.

wdyt?

daniel-de-vera · 2023-11-09T17:41:11Z

@scott-cotton, I implemented this:

Should we detect operator version and error out on old operator appropriately (since local is beta)?

In PR #92

daniel-de-vera · 2023-11-09T17:44:53Z

Should we remove the grpc/go-sdk model conversions?

Do you mean across the whole CLI? (what would look like a massive change to me).

I renamed these funcs:

23-11-09 scott@air cli % git diff | grep func
-func ToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error) {
+func xToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error) {
@@ -23,7 +23,7 @@ func ToModelsSandboxSpec(grpcSpec *structpb.Struct) (*models.SandboxSpec, error)
-func ToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
+func xToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
@@ -32,7 +32,7 @@ func ToGRPCSandbox(sb *models.Sandbox) (*structpb.Struct, error) {
-func ToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
+func xToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
@@ -42,7 +42,7 @@ func ToGRPCSandboxSpec(sbs *models.SandboxSpec) (*structpb.Struct, error) {
-func ToModelsSandbox(grpcSandbox *structpb.Struct) (*models.Sandbox, error) {
+func xToModelsSandbox(grpcSandbox *structpb.Struct) (*models.Sandbox, error) {

in internal/locald/api/sandboxmanager/convert and it compiles fine, so they are now unused.

Oh, I see! It's done now.

daniel-de-vera · 2023-11-09T17:46:14Z

Should we remove the api related stuff from internal/config/locald.go?

Not sure, I understand what you mean by that, can you expand?

Sorry, it was too vague. Was thinking that with the removal of sandbox apply from sandbox manager, we would probably not need the api key nor initialise APIConfig inside of anything in locald. I thought I remembered adding this CIConfig api support specifically for the sandbox apply proxy, and greping around it looks to me like we can remove APIKey from the CIConfig struct, as well as related code setting up the config for a *manager to communicate with the API.

wdyt?

Got it, yes, looks like a good idea.
I will check it and remove unneeded things.

daniel-de-vera · 2023-11-09T19:41:09Z

Should we remove the api related stuff from internal/config/locald.go?

Not sure, I understand what you mean by that, can you expand?

Sorry, it was too vague. Was thinking that with the removal of sandbox apply from sandbox manager, we would probably not need the api key nor initialise APIConfig inside of anything in locald. I thought I remembered adding this CIConfig api support specifically for the sandbox apply proxy, and greping around it looks to me like we can remove APIKey from the CIConfig struct, as well as related code setting up the config for a *manager to communicate with the API.

wdyt?

@scott-cotton, this is done now, in eaf51d8

scott-cotton · 2023-11-13T10:51:34Z

internal/locald/sandboxmanager/sandbox_controller.go

@@ -43,6 +43,8 @@ func newSBController(log *slog.Logger, sandbox *tunapiv1.WatchLocalSandboxesResp
 	}
 	// run the controller
 	go ctrl.run()
+	// trigger a reconcile
+	ctrl.triggerReconcile()


I would put this in ctrl.run() so it is more self-contained.

I addressed this in #94

scott-cotton · 2023-11-13T10:54:54Z

internal/locald/sandboxmanager/sandbox_controller.go

+	select {
+	case <-revtun.rtToClose:
+	default:
+		ctrl.log.Debug("sandbox controller closing revtun", "local", xwName)


I think that, in order for the select to protect against double closures, we would need to put the call to ctrl.log.Debug after the close()

I addressed this in #94

scott-cotton · 2023-11-13T11:02:39Z

internal/locald/sandboxmanager/sandboxes_watcher.go

+		sbMon.stop()
+	}
+	sbw.sbMu.Unlock()
+


This code was hard to understand, maybe some comments would help:

the grpcserver has called graceful stop, so no new sandboxes will be processed during a call to this function

the delFn is called at the end of the sb monitor run, so the wg will wait for all monitors to stop running.

I addressed this in #94

scott-cotton

Approving, left some minor suggestions in second review round.

* Implementation of sandboxes watcher status * PR feeback * Include machine id to local status

* Add support for old operators (< 0.14.1) * Addition of operatorInfoUpdater * Minor fix * Bug fix * Point libconnect to main * Point github.com/signadot/go-sdk to main version * PR feedback

daniel-de-vera added 3 commits November 6, 2023 14:16

WIP: checkpoint

af498ef

Release candidate changes

58fd74b

Bug fix

96c0d06

daniel-de-vera requested a review from scott-cotton November 7, 2023 20:58

Another fix

3bb0bc0

scott-cotton reviewed Nov 9, 2023

View reviewed changes

Reflect changes in tunnel API spec

29778f8

Remove unneeded code.

bd37be9

Get rid of API and API key in locald command

eaf51d8

scott-cotton reviewed Nov 13, 2023

View reviewed changes

scott-cotton approved these changes Nov 15, 2023

View reviewed changes

daniel-de-vera and others added 4 commits November 15, 2023 09:27

Implementation of sandboxes watcher status (#92)

399b23e

* Implementation of sandboxes watcher status * PR feeback * Include machine id to local status

Add support for old operators (< 0.14.1) (#94)

47283b3

Include operator info as par of the status (#95)

54bc79c

* Add support for old operators (< 0.14.1) * Addition of operatorInfoUpdater * Minor fix * Bug fix * Point libconnect to main * Point github.com/signadot/go-sdk to main version * PR feedback

Merge remote-tracking branch 'origin/main' into sandbox-machineID

45d579d

daniel-de-vera merged commit 58cbd65 into main Nov 20, 2023

daniel-de-vera deleted the sandbox-machineID branch November 20, 2023 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for local machine ID #91

Add support for local machine ID #91

daniel-de-vera commented Nov 7, 2023

scott-cotton left a comment •

edited

Loading

daniel-de-vera commented Nov 9, 2023

scott-cotton commented Nov 9, 2023

scott-cotton commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

scott-cotton Nov 13, 2023

daniel-de-vera Nov 15, 2023

scott-cotton Nov 13, 2023

daniel-de-vera Nov 15, 2023

scott-cotton Nov 13, 2023

daniel-de-vera Nov 15, 2023

scott-cotton left a comment

Add support for local machine ID #91

Add support for local machine ID #91

Conversation

daniel-de-vera commented Nov 7, 2023

scott-cotton left a comment • edited Loading

Choose a reason for hiding this comment

daniel-de-vera commented Nov 9, 2023

scott-cotton commented Nov 9, 2023

scott-cotton commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

daniel-de-vera commented Nov 9, 2023

scott-cotton Nov 13, 2023

Choose a reason for hiding this comment

daniel-de-vera Nov 15, 2023

Choose a reason for hiding this comment

scott-cotton Nov 13, 2023

Choose a reason for hiding this comment

daniel-de-vera Nov 15, 2023

Choose a reason for hiding this comment

scott-cotton Nov 13, 2023

Choose a reason for hiding this comment

daniel-de-vera Nov 15, 2023

Choose a reason for hiding this comment

scott-cotton left a comment

Choose a reason for hiding this comment

scott-cotton left a comment •

edited

Loading