Add top level keys for policy definition into specs #3048
Conversation
michel-laterman
force-pushed
the
oapi-policy-def
branch
from
970c729 to
201087e
Compare
michel-laterman
changed the title
| @@ -72,29 +72,36 @@ | |||
| }, | |||
| "data": { | |||
| "description": "The opaque payload.", | |||
| "type": "object", | |||
There was a problem hiding this comment.
Our custom go json schema generator has some strange behaviour
If type: object is found it will define a go struct with that name then use it as the inline type for the attribute, If format: raw is found it will define the inline type as json.RawMessage (what we want for these cases).
If both are used, it leads to an unused (empty) struct definition.
Also names/attributes can collide, so if we have a top level object named agent, and an attribute named agent (defined with type: object and format:raw), things go wrong.
I removed type: object from attributes we want as a json.RawMessage in order to avoid this behaviour in the future
| "type": "string" | ||
| }, | ||
|
|
||
| "checkin": { |
There was a problem hiding this comment.
This was not scoped correctly (as defined in the draft-07 definition) so the go structs for checkin were not created.
The openapi spec for checkin requests has less strict definition, @cmacknz should we update the openapi spec to check for the checkin as it would be defined here?
| @@ -490,7 +490,42 @@ components: | |||
| yaml: "timeout" | |||
| signed: | |||
| $ref: "#/components/schemas/actionSignature" | |||
|
|
|||
| policyData: | |||
There was a problem hiding this comment.
No go code has been generated for this because it is not linked as part of an action payload at the moment.
We can likely use oneOf to generate fleet-server's structs in order to be backwards compatible if that's required
model/schema.json
Outdated
| "outputs": { | ||
| "description": "A map of all outputs that the agent running the policy can use to send data to.", | ||
| "format": "raw" |
There was a problem hiding this comment.
@juliaElastic, If we define additional keys as part of the policy definition it should remove some of the boilerplate around casting for the remote output definition.
But we wound need to decide on how much detail we want to get into (should fleet-server care or be aware of attributes it does not use? should we use the additionalProperties to hide some of these?)
The internal policy handler always treats outputs and inputs as some form of map[string]interface{}, so at the very least we may want to change the top level keys from json.RawMessage (which is not unmarshalled) to a map or slice (for secret_references)
There was a problem hiding this comment.
I did a little expirementing and was unable to get the json schema generator tool we use to output generic objects (type map[string]interface), but it's easily able to do lists of type []interface{}, or just assign it the generic interface{} type.
That means if we want to have a stricter definition in fleet-server then a json.RawMessage or interface{} for outputs we would need to define the properties that exist in the policy in Elasticsearch (the schema we return from our openapi endpoint can differ by removing the secret_token for instance)
There was a problem hiding this comment.
I think it would be nice to define at least the properties that are being used in fleet-server.
There was a problem hiding this comment.
Outputs is tracked as a map[string]map[string]interface{} to make it easy to iterate in the fleet-server.
I didn't define attributes in outputs as it can get complex (ssl, proxies, keys, etc may be involved)
Use the generated model.PolicyData struct for operations within the fleet-server.
| } | ||
|
|
||
| // Validate that default was found | ||
| if pp.Default.Name != "other" { |
There was a problem hiding this comment.
I'm not sure how this test was passing as the minified policy had a single output named "default"
| // The role is required to do api key management | ||
| if p.Role == nil { | ||
| zlog.Error(). | ||
| Msg("policy does not contain required output permission section") | ||
| return ErrNoOutputPerms | ||
| } | ||
| if _, ok := outputMap[p.Name]; !ok { |
There was a problem hiding this comment.
fail fast here instead of a possible panic on line 245
|
|
||
| r := policy.RevisionFromPolicy(pp.Policy) | ||
| // FIXME use better action definition from open api spec |
There was a problem hiding this comment.
This will be a part of the pr to define actions/acks (when we use the policyData defined as part of the openapi spec)
| // findDefaultName returns the name of the 1st output with the "elasticsearch" type or falls back to behaviour that relies on deprecated fields. | ||
| // | ||
| // Previous fleet-server and elastic-agent released had a default output which was removed Sept 2021. | ||
| func findDefaultOutputName(outputs map[string]map[string]interface{}) (string, error) { |
There was a problem hiding this comment.
From what I can tell ParsedPolicy.Default is only set in the fleet-server, nothing fleet-server runs uses it (and it's not transmitted as part of a policy_change action. Do we still want to keep this?
@blakerouse I believe you wrote this, do you remember any additional context?
|
SonarQube Quality Gate
|
| return err | ||
| } | ||
|
|
||
| outputMap[p.Name]["api_key"] = output.APIKey |
There was a problem hiding this comment.
@michel-laterman it seems to me it is going to mutate the parsedPolicy output here and potentially impact concurrent checkin no? think previously we where parsing the fields for each checkin and mutating the parsed data
What is the problem this PR solves?
Policy is only defined as a generic
interface{}orjson.RawMessage.How does this PR solve the problem?
Define agent policy structure as part of the openapi and json schema specs.
Use the generated json schema struct when fleet-server interacts with Elasticsearch.
Future work will use the struct generated from openapi as part of responses