Store check type in catalog #6561
Conversation
There was a problem hiding this comment.
This looks awesome.
I tried to think of places you could possible have missed but I found all of them covered in here: api structs, agent endpoint and internal routines for adding from config.
The only one I don't see here that I think might need this is the /catalog/register endpoint that can register external checks directly with servers (e.g. used by ESM). Can you check if there is anything extra needed there to ensure the type is populated before it is stored?
|
Good catch @banks, the catalog registration is a separate path. Now I didn't remove the changes in the agent that populate |
There was a problem hiding this comment.
Changes LGTM. I'm a tiny bit nervous that we might have missed something but not super sure how to be more confident.
Would it be possible/desirable to ensure the type field is set in the state store instead of HTTP endpoints? I think there is only one path for actually persisting the check record in the state store so we'd be sure that it was impossible to store one without the field if we did it there. I guess you already thought of that and there was some reason it wouldn't work though?
agent/consul/catalog_endpoint.go
Outdated
| @@ -125,6 +125,13 @@ func (c *Catalog) Register(args *structs.RegisterRequest, reply *struct{}) error | |||
| check.Node = args.Node | |||
| } | |||
| checkPreApply(check) | |||
|
|
|||
| // Populate check type for cases when this is a direct reg with /catalog/register | |||
| // Typically this is populated when a check is registered with an agent | |||
There was a problem hiding this comment.
nit: if this endpoint is being used then by definition it's a direct reg with /catalog/register.
Agent Anti-entropy goes straight to RPC.
There was a problem hiding this comment.
What I'm trying to communicate is that the user registered the check directly to the catalog. Anti-entropy is different because of the indirection. Users add checks to the agent then the agent triggers a sync.
How about:
// Populate check type for cases when a check is registered in the catalog directly and not via anti-entropy
|
@banks Unfortunately we can't add the check in the state store. Anything that goes in there is synced via anti-entropy, so we don't pass the actual check definition down. (and the check definition is needed to infer the type) We've talked about pushing the definition to the state store but the concern is that users are storing sensitive data in check definitions under the assumption that they won't leave the agent. I'll take another pass through the paths a check might take, in case there's another one that isn't covered. |
|
Though, actually, an alternative could be to pass the check definitions down to the state store functions like addCheckLocked, but nil it out before storing the check. I'll make that change and see how it looks. |
|
@freddygv Ah OK I guess I was missing something here that we were doing this on agent side! That makes total sense and I'm OK with it as it is if that is the case. Changing what we send over Anti-entropy is probably not worth it. I was just thinking that the register RPC endpoint already had enough info and was where we were doing this. |
|
Very nice (and useful) change, thank you! |
This PR adds the type of check to the catalog.
The change is primarily to allow the UI to know the type of each check.
The new field can then be fetched via
/agentand/healthendpoints.