Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update audit API with missing pieces #46065

Merged
merged 3 commits into from
May 25, 2017
+636 −439
Merged

Update audit API with missing pieces #46065

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c98cab
Update audit API with missing pieces
tallclair May 19, 2017
7bc9b30
Generated code
tallclair May 23, 2017
4c54970
Update existing code for audit API changes
tallclair May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 31 additions & 2 deletions staging/src/k8s.io/apiserver/pkg/apis/audit/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ import (
"k8s.io/apimachinery/pkg/types"
)

// Header keys used by the audit system.
const (
// Header to hold the audit ID as the request is propagated through the serving hierarchy. The
// Audit-ID header should be set by the first server to receive the request (e.g. the federation
// server or kube-aggregator).
HeaderAuditID = "Audit-ID"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move the discussion of whether audit IDs should be 1 or many to here.

@deads2k - I think part of my reservation around multiple audit IDs is I don't understand what threat the audit trace would help to capture. Can you elaborate?

)

// Level defines the amount of information logged during auditing
type Level string

Expand All @@ -39,6 +47,22 @@ const (
LevelRequestResponse Level = "RequestResponse"
)

// Stage defines the stages in request handling that audit events may be generated.
type Stage string

// Valid audit stages.
const (
// The stage for events generated as soon as the audit handler receives the request, and before it
// is delegated down the handler chain.
StageRequestReceived = "RequestReceived"
// The stage for events generated once the response headers are sent, but before the response body
// is sent. This stage is only generated for long-running requests (e.g. watch).
StageResponseStarted = "ResponseStarted"
// The stage for events generated once the response body has been completed, and no more bytes
// will be sent.
StageResponseComplete = "ResponseComplete"
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts does the response started vs. completed distinction make sense for long running and short requests? I thought that having both send response completed, but reserve ResponseStarted for long running requests made more sense than the other way around.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started vs Complete does make sense. I'm not sure received vs started does? Can there be a received but not started, or that will hang for a while and then started?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • RequestReceived => meta data
  • ResponseStarted => header is complete, http status code is filled in, RequestObject is written
  • ResponseComplete => ResponseObject is written

Makes sense.

We will create these events:

  • short-running reading => ResponseComplete
  • short-running writing => RequestReceived + ResponseComplete
  • long-running reading => ResponseStarted + ResponseComplete
  • long-running writing (e.g. proxy, exec, ...) => all 3
  • if we don't know: all three (?)

Does this make sense? Do we need RequestResponseStarted also for short-running-reading?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I agree that RequestReceived is less useful for read-only requests, should we really be hardcoding that? Or do you think we need to extend the policy to include that?

@soltysh - Unfortunately the distinction between received & started is tied to the implementation (i.e. before the request is passed from the audit http handler to the delegate). From the user perspective, maybe this can be thought of as before the request is handled, vs before the request is handled but the response hasn't been sent? "Started" really only makes sense for long running requests anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestReceived seems like it'd be somewhat useful for aggregated API servers where the request would be forwarded to another server after RequestReceived and before ResponseComplete.

Regardless this doesn't actually impact the types, right? Just the implementation? I could imagine us being vague about what stages other than ResponseComplete actually get sent for the alpha implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ericchiang agreed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with a hard-coded logic as above for alpha. We can extend this later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the panic handler? Technically a panic can happen in any phase. We could add another stage for panic.

)

// Event captures all the information that can be included in an API audit log.
type Event struct {
metav1.TypeMeta
Expand All @@ -53,6 +77,9 @@ type Event struct {
Timestamp metav1.Time
// Unique audit ID, generated for each request.
AuditID types.UID
// Stage of the request handling when this event instance was generated.
Stage Stage
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stage? Phase? Reason? - I'm open to naming suggestions here, but would prefer an enum to a boolean.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for stage

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stage


// RequestURI is the request URI as sent by the client to a server.
RequestURI string
// Verb is the kubernetes verb associated with the request.
Expand Down Expand Up @@ -81,12 +108,12 @@ type Event struct {
// merging. It is an external versioned object type, and may not be a valid object on its own.
// Omitted for non-resource requests. Only logged at Request Level and higher.
// +optional
RequestObject runtime.Unknown
RequestObject *runtime.Unknown
// API object returned in the response, in JSON. The ResponseObject is recorded after conversion
// to the external type, and serialized as JSON. Omitted for non-resource requests. Only logged
// at Response Level.
// +optional
ResponseObject runtime.Unknown
ResponseObject *runtime.Unknown
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there's no precedent for using runtime.Unkown directly, so I changed to using a pointer type here /cc @sttts

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}

// EventList is a list of audit Events.
Expand Down Expand Up @@ -191,6 +218,8 @@ type ObjectReference struct {
APIVersion string
// +optional
ResourceVersion string
// +optional
Subresource string
}

// UserInfo holds the information about the user needed to implement the
Expand Down