agent icon indicating copy to clipboard operation
agent copied to clipboard

Draft: Management API Definitions for Review

Open oliveromahony opened this issue 1 year ago • 5 comments

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.

  • [x] I have read the CONTRIBUTING document
  • [x] I have run make install-tools and have attached any dependency changes to this pull request
  • [x] If applicable, I have added tests that prove my fix is effective or that my feature works
  • [x] If applicable, I have checked that any relevant tests pass after adding my changes
  • [ ] If applicable, I have updated any relevant documentation (README.md)
  • [ ] If applicable, I have tested my cross-platform changes on Ubuntu 22, Redhat 8, SUSE 15 and FreeBSD 13

oliveromahony avatar Feb 22 '24 16:02 oliveromahony

Some general notes:

  • keep proto separate from generated bindings
  • using full path , i.e. f5.nginx.agent.api.grpc.mpi.v1.common.MessageRequest for imports? I think can be shorten to common.MessageRequest

nginx-nickc avatar Feb 22 '24 17:02 nginx-nickc

Some general notes:

  • keep proto separate from generated bindings

Where is this recommendation from?

  • using full path , i.e. f5.nginx.agent.api.grpc.mpi.v1.common.MessageRequest for imports? I think can be shorten to common.MessageRequest

We don't have a naming convention for the package. I prefer the longer one since it shows where the source originated from

oliveromahony avatar Feb 23 '24 11:02 oliveromahony

We don't have a naming convention for the package. I prefer the longer one since it shows where the source originated from

https://protobuf.dev/programming-guides/style/#packages

oliveromahony avatar Feb 23 '24 15:02 oliveromahony

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.

mtbChef avatar Feb 26 '24 16:02 mtbChef

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.

nginx-nickc avatar Feb 26 '24 17:02 nginx-nickc