abstract action creation, refactor proxy, add proxy tests by patrickdemers6 · Pull Request #308 · teslamotors/vehicle-command

patrickdemers6 · 2024-08-31T06:54:18Z

Description

Lots of refactoring with the overarching goal being to add unit tests for the HTTP proxy.

Move action creation out of vehicle. Now, a vehicle has two methods for actions: ExecuteAction and ExecuteUnsignedAction.
Now that go is upgraded to >= 1.22, use the routing enhancements.
Add unit tests for HTTP proxy.
General refactoring.

Type of change

Please select all options that apply to this change:

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Bug fix (non-breaking change which fixes an issue)
Documentation update

Checklist:

Confirm you have completed the following steps:

My code follows the style of this project.
I have performed a self-review of my code.
I have made corresponding updates to the documentation.
I have added/updated unit tests to cover my changes.

pkg/proxy/proxy.go

cmd/tesla-control/commands.go

cmd/tesla-http-proxy/main.go

sethterashima · 2024-09-10T21:23:26Z

cmd/tesla-control/commands.go

 		return errors.New("missing COMMAND")
 	}

-	info, err := checkReadiness(args[0], car != nil && car.PrivateKeyAvailable(), acct != nil, car != nil)


Shouldn't re-use the same symbol for a package and a variable. This would break if there's a conflict in exported names/methods.

On the other hand, I agree that car isn't ideal here. I'd suggest just using v instead; this shorthand for a locally-scoped variable is pretty common in Go.

Agreed with the non-awesomeness of shadowing the package with variables, but nothing will "break" though. Once shadowed, you just won't be able to use the underlying package while shadowed. For example:
https://go.dev/play/p/7VhEqRIMXYa
The main downside of shadowing is that you don't know what's what without really paying attention, and that can lead to confusion when reading the code.

I would recommend against v as a var name; this is common for receivers, but for one of many params in funcs can have have sizable scopes, it'd be better to have slightly more meaningful names.

In this particular case, adding an import alias like veh or vehicl or pvehicle or vehiclep or vehipkg or somethingthatsnotvehicle might work better, because it'll be very obvious that vehicle veh.Vehicle means veh is a package, and then the rest of the code reads nicely with vehicle as a subject to everything.

Actually I think I'd just keep car in here.

sethterashima · 2024-09-10T21:27:13Z

cmd/tesla-control/commands.go

 		requiresFleetAPI: false,
-		handler: func(ctx context.Context, acct *account.Account, car *vehicle.Vehicle, args map[string]string) error {
-			return car.Unlock(ctx)
+		handler: func(ctx context.Context, acct *account.Account, vehicle *vehicle.Vehicle, args map[string]string) error {


The API should be designed for the consumer, not the developer or the tester, and this API is cumbersome for the consumer.

If we want to abstract the creation of the protobufs, I'd suggest we wrap the code on the other direction:

func (v *Vehicle) Unlock(ctx context.Context) error { return v.executeAction(ctx, action.Unlock()) }

If the motive here is to improve testability, then this could also be accomplished by implementing a test connector.Connector that functions similarly to net/http/httptest in that it records requests/responses and you can verify they are as expected.

I agree with focusing on consumer vs. author for the API design.
What we lose with moving to this ExecuteAction model is that we don't have an easy way to know which actions are available to us, especially because ExecuteAction takes a loose interface{} type which prevents tools from inferring this statically.
Couple of thoughts:

Moving the actions out of Vehicle and into an ExecuteAction has the advantage of clearly separating the domains of what we can do with a vehicle (Connect, Disconnect, AddKey, etc) from all the actions. But we'd need vehicle (the package) to specify an Action interface to make it easy to statically understand what can be passed in. The implementation details of CarServer vs VSSEC should be kept hidden just like they were before.

If we keep all the actions straight on Vehicle (like Seth suggests), we can test either by having a recorder (like Seth suggests), and/or having an action provider with a big interface that would be swappable for tests. The difference here is that it wouldn't clutter the API from a consumer point of view, and would make things testable a bit more directly than with a recorder. (We could also have the private executeAction be a swappable func for tests).

sethterashima · 2024-09-10T21:34:45Z

pkg/vehicle/action.go

+// The action can be a *carserver.Action_VehicleAction or a *vcsec.UnsignedMessage.
+// Actions are created using the action package.
+func (v *Vehicle) ExecuteAction(ctx context.Context, action interface{}) error {
+	switch action := action.(type) {


sethterashima · 2024-09-10T21:35:45Z

pkg/vehicle/action.go

+		_, err := v.getCarServerResponse(ctx, action)
+		return err
+	case *vcsec.UnsignedMessage:
+		encodedPayload, err := proto.Marshal(action)


Nit: Mostly for aesthetic reasons, it'd be nice if getCarServerResponse and getVCSECResult had the same signature.

sethterashima · 2024-09-10T21:39:36Z

pkg/proxy/proxy.go

-	}, nil
+// Vehicles must have the public part of skey enrolled on their keychains.
+// (This is a command-authentication key, not a TLS key.)
+func New(ctx context.Context, skey protocol.ECDHPrivateKey, cacheSize int, accountProvider AccountProvider) *Proxy {


Consider returning an (always nil) error for the sake of API stability. This would also allow API stability going forward of the implementation changes in a way that re-introduces the possibility of failing.

sethterashima · 2024-09-10T21:57:34Z

pkg/action/rke_test.go

+			Expect(action).ToNot(BeNil())
+			Expect(action.SubMessage).ToNot(BeNil())
+			Expect(action.SubMessage).To(BeAssignableToTypeOf(&vcsec.UnsignedMessage_RKEAction{}))
+			Expect(action.SubMessage.(*vcsec.UnsignedMessage_RKEAction).RKEAction).To(Equal(vcsec.RKEAction_E_RKE_ACTION_AUTO_SECURE_VEHICLE))


The Golang protobuf code checks for nil receivers and returns default values, so I think you could just do:

action := action.AutoSecureVehicle() Expect(action.GetRKEAction()).To(Equal(vcsec.RKEAction_E_RKE_ACTION_AUTO_SECURE_VEHICLE))

Note that this wouldn't work for RKEAction_E_RKE_ACTION_UNLOCK, since the protobuf definition breaks the convention of reserving the 0 (default) value for UNKNOWN.

Copy/Paste this comment a bunch of times below.

sethterashima · 2024-09-10T22:09:20Z

mocks/proxy.go

@@ -0,0 +1,164 @@
+// Code generated by MockGen. DO NOT EDIT.


I know Go makes it hard to avoid including autogenerated code, but... any way to avoid this without breaking go test out of the box?

sethterashima · 2024-09-10T22:15:24Z

.github/workflows/build.yml


+    - name: Mocks up to date
+      run: |
+        go install go.uber.org/mock/mockgen@latest


Can we peg this to the version in our go.mod instead of latest?

vmallet · 2024-09-19T15:59:50Z

pkg/proxy/command.go

+		return action.ResetPIN(), nil
 	case "reset_valet_pin":
-		return func(v *vehicle.Vehicle) error { return v.ResetValetPin(ctx) }, nil
+		return action.ResetValetPin(), nil


Tangent: maybe Pin -> PIN to be consistent here.

vmallet · 2024-09-19T16:02:24Z

pkg/proxy/proxy.go

 }

-func (p *Proxy) markUnsupportedVIN(vin string) {
+func (p *Proxy) markSignedCommandsUnsupportedVIN(vin string) {


Why the VIN suffix? (The other one doesn't have it)

vmallet · 2024-09-19T16:03:10Z

pkg/proxy/proxy.go

 }

-func (p *Proxy) isNotSupported(vin string) bool {
+func (p *Proxy) signedCommandUnsupported(vin string) bool {


Previous one says Commands, this one says Command, but they're dealing with the same thing.

vmallet · 2024-09-19T16:06:12Z

pkg/proxy/proxy.go

-		proxyReq.Header.Set(xff, strings.Join(previous, ", "))
-	}
-	proxyReq.URL.Host = host
+	acct := req.Context().Value(accountContext).(Account)


Might be good to check the type assertion here.

vmallet · 2024-09-19T16:20:15Z

pkg/proxy/proxy.go

+	return p.accountProvider(token, proxyProtocolVersion)
+}
+
+func writeResponseError(w http.ResponseWriter, code int, err error) {


writeErrorResponse?

vmallet · 2024-09-19T16:22:56Z

pkg/action/doc.go

@@ -0,0 +1,2 @@
+// Package action contains functions for creating actions to send to vehicles.
+package action


Note: if kept, we could consider having action live under vehicle (so pkg/vehicle/action) (an idea).

...or internal/action? What's the value in exporting? Plus, assigning generic names like "action" to exported packages is frowned on.

vmallet · 2024-09-19T16:24:04Z

pkg/action/charge.go

+	"fmt"
+	"time"
+
+	carserver "github.com/teslamotors/vehicle-command/pkg/protocol/protobuf/carserver"


Note: we should remove all these redundant carserver aliases.

vmallet · 2024-09-19T16:25:29Z

cmd/tesla-control/commands.go

 		return errors.New("missing COMMAND")
 	}

-	info, err := checkReadiness(args[0], car != nil && car.PrivateKeyAvailable(), acct != nil, car != nil)


Actually I think I'd just keep car in here.

vmallet · 2024-09-19T16:44:11Z

cmd/tesla-control/commands.go

 		requiresFleetAPI: false,
-		handler: func(ctx context.Context, acct *account.Account, car *vehicle.Vehicle, args map[string]string) error {
-			return car.Unlock(ctx)
+		handler: func(ctx context.Context, acct *account.Account, vehicle *vehicle.Vehicle, args map[string]string) error {


I agree with focusing on consumer vs. author for the API design.
What we lose with moving to this ExecuteAction model is that we don't have an easy way to know which actions are available to us, especially because ExecuteAction takes a loose interface{} type which prevents tools from inferring this statically.
Couple of thoughts:

Moving the actions out of Vehicle and into an ExecuteAction has the advantage of clearly separating the domains of what we can do with a vehicle (Connect, Disconnect, AddKey, etc) from all the actions. But we'd need vehicle (the package) to specify an Action interface to make it easy to statically understand what can be passed in. The implementation details of CarServer vs VSSEC should be kept hidden just like they were before.

If we keep all the actions straight on Vehicle (like Seth suggests), we can test either by having a recorder (like Seth suggests), and/or having an action provider with a big interface that would be swappable for tests. The difference here is that it wouldn't clutter the API from a consumer point of view, and would make things testable a bit more directly than with a recorder. (We could also have the private executeAction be a swappable func for tests).

vmallet · 2024-09-19T16:46:24Z

pkg/vehicle/action.go

+//
+// The action can be a *carserver.Action_VehicleAction or a *vcsec.UnsignedMessage.
+// Actions are created using the action package.
+func (v *Vehicle) ExecuteAction(ctx context.Context, action interface{}) error {


If we keep this model, I'd vote for Execute(context.Context, Action) rather than ExecuteAction(context.Context, interface{}).

patrickdemers6 requested a review from sethterashima August 31, 2024 06:54

patrickdemers6 force-pushed the proxy-refactor-add-tests branch from 5553478 to 7487e17 Compare August 31, 2024 06:56

agbpatro reviewed Sep 6, 2024

View reviewed changes

pkg/proxy/proxy.go Outdated Show resolved Hide resolved

agbpatro reviewed Sep 6, 2024

View reviewed changes

pkg/proxy/proxy.go Outdated Show resolved Hide resolved

patrickdemers6 force-pushed the proxy-refactor-add-tests branch 2 times, most recently from ac3073b to e4c6c28 Compare September 8, 2024 03:12

patrickdemers6 changed the title ~~refactor http proxy and add unit tests~~ abstract action creation, refactor proxy, add proxy tests Sep 8, 2024

patrickdemers6 force-pushed the proxy-refactor-add-tests branch 3 times, most recently from ea5517a to bce1ebc Compare September 9, 2024 17:07