Conversation
fspmarshall
left a comment
fspmarshall
left a comment
There was a problem hiding this comment.
LGTM overall. I suggest altering the naming of public methods/types to ConnectedServiceCounts (or similar). This doesn't tell us the total inventory counts, just the counts of instances connected to a given auth server. Very important distinction for most use cases.
There was a problem hiding this comment.
Could we also expose some sort of notification mechanism that triggers whenever some counts go from zero to non-zero and viceversa? I imagine it could be useful for other components in the auth server that depend on some agent to be connected but don't necessarily care about the exact number.
There was a problem hiding this comment.
Discussed a bit of this offline. This is useful but may not be immediately useful for the Okta service use case.
| } | ||
|
|
||
| // counts returns the count of each service seen in the counter. | ||
| func (s *serviceCounter) counts() map[types.SystemRole]uint64 { |
There was a problem hiding this comment.
Is it worth it to add a dedicated function to fetch the count for a single role?
There was a problem hiding this comment.
I was waffling about that. It makes sense, but it's also not a huge deal to just query the map. I'm ambivalent, I think. If you feel strongly I can add one.
There was a problem hiding this comment.
I just realized that serviceCounter has a get method that does that already (although it could be made to not actually add to the map if there's no need for it).
I wouldn't bother with wiring it up for the gRPC service (making a map is nothing compared to the overhead of protobuf encoding and network transmission, and saving like 5 strings and integers worth of data is kinda useless) but it might be useful as an exposed ConnectedServiceCount(types.SystemRole) method in Controller. The first usecase for this, after all, is going to be about checking if there's any Okta agents connected, and we won't care about any other count.
Inventory service counts have been introduced. These will be useful when trying to conditionally act on the presence of a particular service in a Teleport cluster. In particular, these will be used to disable Okta's access request reconciler when there are no Okta services connected to the cluster.
mdwn
force-pushed
the
mike.wilson/inventory-service-counts
branch
from
ffd6864 to
3391cb5
Compare
| } | ||
|
|
||
| // counts returns the count of each service seen in the counter. | ||
| func (s *serviceCounter) counts() map[types.SystemRole]uint64 { |
There was a problem hiding this comment.
I just realized that serviceCounter has a get method that does that already (although it could be made to not actually add to the map if there's no need for it).
I wouldn't bother with wiring it up for the gRPC service (making a map is nothing compared to the overhead of protobuf encoding and network transmission, and saving like 5 strings and integers worth of data is kinda useless) but it might be useful as an exposed ConnectedServiceCount(types.SystemRole) method in Controller. The first usecase for this, after all, is going to be about checking if there's any Okta agents connected, and we won't care about any other count.
… don't populate the underlying map on read.
3 participants
Inventory service counts have been introduced. These will be useful when trying to conditionally act on the presence of a particular service in a Teleport cluster. In particular, these will be used to disable Okta's access request reconciler when there are no Okta services connected to the cluster.