Draft: Management API Definitions for Review #574
Conversation
github-actions
bot
added
chore
oliveromahony
changed the title
oliveromahony
added
enhancement
|
Some general notes:
|
api/grpc/mpi/v1/file.proto
Outdated
|
|
||
| service FileService { | ||
| // Get the overview of files for a particular configuration version of an instance | ||
| rpc GetOverview(ConfigVersion) returns (FileOverview) {} |
There was a problem hiding this comment.
nit, i think would be nice to document the direction. While conventionally, we know the "Get" is from perspective of the Agent, it'll be good to be explicit.
There was a problem hiding this comment.
all these originate from the client?
There was a problem hiding this comment.
followed the reference here: https://protobuf.dev/programming-guides/style/#services
There was a problem hiding this comment.
not sure how your link apply here?
api/grpc/mpi/v1/file.proto
Outdated
| // Represents a collection of files | ||
| message FileOverview { | ||
| // A list of files | ||
| repeated File file = 1; |
There was a problem hiding this comment.
do we need the full file object inside the FileOverview? Seems like maybe only the FileMeta is sufficient? since the goal is to use the "version" to detect that there is a diff, then use the individual files hashes to figure out which files need to be updated?
There was a problem hiding this comment.
This was specified in the definition
Where is this recommendation from?
We don't have a naming convention for the package. I prefer the longer one since it shows where the source originated from |
|
api/grpc/mpi/v1/file.proto
Outdated
| // Get the file contents for a particular file | ||
| rpc GetFile(FileRequest) returns (FileContents) {} | ||
| // Update a file from the Agent to the Server | ||
| rpc UpdateFile(File) returns (FileMeta) {} |
There was a problem hiding this comment.
it can be used for both Add and Updating a file from the server perspective. We don't have a naming convention for this
There was a problem hiding this comment.
Ok, I saw SendFile in the comments, so I was curious which name we are using.
There was a problem hiding this comment.
I've updated the comment, thanks
|
Wanted to check in on what was decided in regards to the message index. I had proposed that we use millis since epoch or timestamp vs (or in addition to?) the index. Doing so would bake in the ability to troubleshoot messages by time and, if a message would eventually be consumed by a user, would provide a timestamp for audit purposes. |
don't think we should mix purposes here. Since we are intending to be an open standard, the assumption would be different for message_index vs message timestamp for different implementation. The current requirement for the message index is that it SHOULD be monotonically increasing, where as the message timestamp should be the timestamp with timezone of the sending system. Even if you want to send the timestamp as epoch, should still attach timezone, since at some point we would want to correlate the event with logs from the sending system, which would most likely be logged in local timezone. Mixing the two concerns just makes it so much harder to troubleshoot down the road. |
| // A list of features that the NGINX Agent has | ||
| repeated string features = 4; | ||
| // Message buffer size, maximum not acknowledged messages from the subscribe perspective | ||
| string message_buffer_size = 5; |
api/grpc/mpi/v1/command.proto
Outdated
| // triggers a series of rpc SendFile(File) for that instances | ||
| ConfigUploadRequest config_upload_request = 6; | ||
| // triggers a reconciliation of with a command_response for a particular action | ||
| ConfigSyncRequest config_sync_request = 7; |
There was a problem hiding this comment.
Why do we need sync if we have apply and upload?
There was a problem hiding this comment.
if the Agent has been offline for awhile or has local changes not shared with the Management Plane a sync is can be requested by the management plane to trigger an upload by the Agent?
There was a problem hiding this comment.
The workflow currently specifies that the Agent should upload a FileOverview after being offline, and then MP can send a ConfigUploadRequest if there's anything missing. I'd say we don't need Sync.
There was a problem hiding this comment.
I don't think we need it, so I've removed it after thinking about it again
| // Default value, no action | ||
| FILE_ACTION_UNSPECIFIED = 0; | ||
| // No changes to the file | ||
| FILE_ACTION_UNCHANGED = 1; |
There was a problem hiding this comment.
can probably remove UNCHANGED, and use UNSPECIFIED as the default, indicating no change.
There was a problem hiding this comment.
UNSPECIFIED is for an unspecified file FileAction. UNCHANGED is an explicit no file has changed. They have different purposes
| message InstanceConfig { | ||
| // provided actions associated with a particular instance. These are runtime based and provided by a particular version of the NGINX Agent | ||
| repeated InstanceAction actions = 1; | ||
| oneof config { |
There was a problem hiding this comment.
why is this oneof? seems like we'll have AgentConfig and one of the NGINX config types.
| } | ||
|
|
||
| // Perform an associated API action on an instance | ||
| message APIActionRequest { |
There was a problem hiding this comment.
thinking about NGINXaaS' azure-specific features, I think this "API" feature might be one option; NGINXaaS could run a local web server on the dataplane that the agent hits via these APIActionRequests to do our weird azure stuff without needing to pollute various enums here.
Are there intentions to expand this to allow specifying URLs and request bodies for the API action, and for the API response to make it back up to the management plane as part of a CommandResponse?
Or is the scope of APIActionRequest intended to be limited to an allowlist of N+ / Unit requests?
| // Command server settings | ||
| Command command = 1; | ||
| // Metrics server settings | ||
| Metrics metrics = 2; |
There was a problem hiding this comment.
Missing a File settings field for configuring the file upload/download endpoint. self is a valid value indicating the use of the current gRPC connection, grpc:// or https:// endpoints are also valid.
| // instance information associated with the NGINX Agent | ||
| Instance agent = 2; | ||
| // instance and infrastructure information associated with the NGINX Agent | ||
| Resource resource = 2; |
There was a problem hiding this comment.
Even though the type is Resource, maybe we still name the field agent ?
There was a problem hiding this comment.
it represents where the Agent is deployed and the associated information around that. Happy to adjust the name but just want to say that it includes Agent, any NGINX product(s), host or container information
Proposed changes
This PR represents the proto definitions for version 3 of the NGINX Agent. The Management Plane Interface (MPI) is a specification defining the functionality exposed by a Management Server in the form of gRPC services and HTTPS endpoints. The NGINX Agent connects to a Management Server in order to administer an NGINX Instance (OSS, Plus).
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTINGdocumentmake install-toolsand have attached any dependency changes to this pull requestREADME.md)