lf-edge / eve-api

Repository for eve-api code
Apache License 2.0
0 stars 13 forks source link

Add flag to enable VNC for shim VM #50

Closed europaul closed 7 months ago

europaul commented 7 months ago

We differentiate between the access to the edge application and the shim VM:

This differentiation is need, because currently shim VM doesn't support any user authentication, so anybody able to access the VNC port can access the shim VM. This way VNC access to the shim VM stays disabled by default unless explicitly enabled by the controller.

europaul commented 7 months ago

@eriknordmark yetus compalins because enableVncShimVM is not in snake_case format, but I think we should leave it like that so it looks more similar to enableVnc flag, which is also not in snake_case

eriknordmark commented 7 months ago

@eriknordmark yetus compalins because enableVncShimVM is not in snake_case format, but I think we should leave it like that so it looks more similar to enableVnc flag, which is also not in snake_case

I think it makes sense to use snake_case here even though it would be different. If you want you can actually change enableVnc to enable_vnc (the generated golang header file ends up with the same CamelCase).

I assume that when you're done you have two commits (one with proto file change and one with the generated files.)

europaul commented 7 months ago

If you want you can actually change enableVnc to enable_vnc (the generated golang header file ends up with the same CamelCase).

@eriknordmark I did a little research: changing the name from camelCase to snake_case like so

- EnableVnc  bool   `protobuf:"varint,16,opt,name=enableVnc,proto3" json:"enableVnc,omitempty"`
+ EnableVnc  bool   `protobuf:"varint,16,opt,name=enable_vnc,json=enableVnc,proto3" json:"enable_vnc,omitempty"

might not break the compatibility with protobuf, but will break the compatibility with JSON (options at the end of the line).

eriknordmark commented 7 months ago

might not break the compatibility with protobuf, but will break the compatibility with JSON (options at the end of the line).

I looked around in the controller and I don't see us using the enableVnc in json. We do refer to the fields in the EVE API using their golang struct names (thus e,g., generation_count in info.proto is referred to as generationCount in the controller).

So there shouldn't be a problem changing this.

europaul commented 7 months ago

@eriknordmark yetus complains about the name change due to this rule. But if JSON names are not used anywhere we can probably ignore this.

eriknordmark commented 7 months ago

@eriknordmark yetus complains about the name change due to this rule. But if JSON names are not used anywhere we can probably ignore this.

Yes, we can ignore that in this case.

But can you squash to two commits? (one for proto files and one for the rest)?

europaul commented 7 months ago

But can you squash to two commits? (one for proto files and one for the rest)?

Done. One more commit for adding .gitignore file.