runtime-tools: log container adjustments.#268
runtime-tools: log container adjustments.#268klihub wants to merge 2 commits intocontainerd:mainfrom
Conversation
klihub
force-pushed
the
devel/audit-logging
branch
2 times, most recently
from
a6cac4e to
c0863cc
Compare
klihub
changed the title
klihub
changed the title
klihub
force-pushed
the
devel/audit-logging
branch
from
c0863cc to
9953d06
Compare
|
@mikebrow @samuelkarp PTAL |
|
@samuelkarp I have a few questions. Related to the approach taken here, is this roughly what you had in mind ? Related to event details, how detailed events do we want to log, and do we want to log them unconditionally ? The PR now logs unconditionally and detailed events, except in a few extreme cases where details could get really verbose. But should it be configurable, or should details be logged at a different logging level ? About the logged events/messages. The main messages are now exposed consts, with the idea that someone might want to build some tooling where it can come handy to have them exported. But I don't know if this really makes sense. Any thoughts ? |
|
Thanks for jumping on this, @klihub. You raised some important questions in your comment that I think we should probably settle on before finalizing the implementation. Since o11y is a key requirement for GA, could we open a GitHub issue to agree on the specific design goals and requirements first? We can treat this PR as a PoC to inform that discussion, but I'd feel more comfortable if we aligned on the "what" and "why" in a design issue before we iterate further on the "how" here. |
I created issue #270 for that. |
klihub
force-pushed
the
devel/audit-logging
branch
3 times, most recently
from
61e4368 to
95bd1fb
Compare
Add an option for setting an external audit event logger and use any configured logger to emit audit events as we adjust the OCI Spec. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Expose owning plugins for adjustments returned by CreateContainer. Include plugin in errors which originate from processing a request by a plugin. Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
klihub
force-pushed
the
devel/audit-logging
branch
from
95bd1fb to
ef60131
Compare
klihub
changed the title
| g.log(AuditAddProcessEnv, | ||
| Fields{ | ||
| "value.name": m.Key, | ||
| "value.value": m.Value, |
There was a problem hiding this comment.
I think we may want to omit logging env var values since this could potentially leak secrets.
| } | ||
|
|
||
| func (f *FieldOwners) compoundOwnerMap(field int32) (map[string]string, bool) { | ||
| func (f *FieldOwners) CompoundOwnerMap(field int32) (map[string]string, bool) { |
There was a problem hiding this comment.
I don't think this method is used in the generator so it could remain unexported.
| } | ||
|
|
||
| // Fields can be used to pass extra information for logged messages. | ||
| type Fields = map[string]any |
There was a problem hiding this comment.
If this is a map, how do we ensure deterministic ordering of logged fields? Is that an exercise left for the Logger?
| if key, marked := m.IsMarkedForRemoval(); !marked { | ||
| g.log(AuditAddProcessEnv, | ||
| Fields{ | ||
| "value.name": m.Key, |
There was a problem hiding this comment.
It depends on the Logger implementation, but this value. prefix will produce longer lines. Is the idea here for the Logger to parse this prefix in order to potentially handle these fields a certain way?
|
|
||
| // CreateContainer relays the corresponding CRI request to plugins. | ||
| func (r *Adaptation) CreateContainer(ctx context.Context, req *CreateContainerRequest) (*CreateContainerResponse, error) { | ||
| func (r *Adaptation) CreateContainer(ctx context.Context, req *CreateContainerRequest) (*CreateContainerResponse, *api.OwningPlugins, error) { |
There was a problem hiding this comment.
Can you provide more context why we need to return the OwningPlugins?
3 participants
This PR implements NRI audit logging for OCI Spec adjustments, which has been identified as one of the missing things we need to add (be)for(e) a v1.0. This patch
Here are updated trees for contained and CRI-O: