Skip to content

Update APIServer to make room for audit profile - #681

Merged
openshift-merge-robot merged 1 commit into
openshift:masterfrom
tkashem:audit-config
Jul 23, 2020
Merged

openshift-merge-robot merged 1 commit into
openshift:masterfrom
tkashem:audit-config

Conversation

@tkashem

@tkashem tkashem commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

No description provided.

@tkashem

tkashem commented Jul 6, 2020

Copy link
Copy Markdown
Contributor Author

work in progress

/hold

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jul 6, 2020
@tkashem tkashem changed the title [WIP] update APIServer to make room for audit profile update APIServer to make room for audit profile Jul 15, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020
@tkashem tkashem changed the title update APIServer to make room for audit profile Update APIServer to make room for audit profile Jul 15, 2020
@tkashem

tkashem commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

ready for review

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2020
Comment thread config/v1/types_apiserver.go Outdated
TLSSecurityProfile *TLSSecurityProfile `json:"tlsSecurityProfile,omitempty"`
// audit specifies the settings for audit configuration to be applied to all OpenShift-provided
// API servers in the cluster.
Audit Audit `json:"audit,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove omitempty. This has no effect.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We also need a default marker here afaik, with {"profile":"Default"}

// - WriteRequestBodies
// - AllRequestBodies
// If unset, the 'Default' profile is used as the default.
Profile AuditProfileType `json:"profile,omitempty"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

add kubebuilder marker for a default "Default"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

plus an enum marker for the three values

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

plus an enum marker for the three values

still missing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sttts I have put it where the enum is declared - https://github.com/openshift/api/pull/681/files#diff-2a98fd5f6a0f42f75cdc7d0bfbeb4333R62-R63

you want me to move it here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nope. You way is even better 👍

configuration to be deployed to all OpenShift-provided API servers
in the cluster \n We provide the following profiles - Default
- WriteRequestBodies - AllRequestBodies If unset, the 'Default'
profile is used as the default."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how about specifying them a bit more?

@sttts

sttts commented Jul 23, 2020

Copy link
Copy Markdown
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2020
@openshift-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sttts, tkashem

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2020
@openshift-merge-robot
openshift-merge-robot merged commit 89de688 into openshift:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants