Audit policy config observer by tkashem · Pull Request #845 · openshift/library-go

tkashem · 2020-07-22T18:10:09Z

No description provided.

tkashem · 2020-07-22T20:15:14Z

holding it, as soon as the openshift/api PR openshift/api#681 merges we can remove the replace from go.mod

/hold

sttts · 2020-07-23T12:34:26Z

+	GetAuditPolicyPath(profile string) (string, error)
+
+	// GetDefaultAuditPolicyPath returns the path to the default audit policy.
+	GetDefaultAuditPolicyPath() string


why isn't that GetAuditPolicyPath("Default")?

to reduce coupling, library-go doesn't need to know the defaulting mechanism, it just needs to know:

the path to the default audit profle

given an audit profile, it needs to know the desired path.
on the other hand, the apiserver/component that defines the audit profiles should know well which audit profile is default.

I know we are defaulting at the CRD level, but i think the observer implementation should still guard for empty profile name and other error conditions.

This func is only called to default the value in the old config. This whole block of code is only needed for the event. Just compare to empty string. Same result.

It resorts to the default value if the existing/current config is not set. when the existing config is not set and the observer function encounters a lister error or any other before it can set the new/desired configuration, what should the observer return?

sttts · 2020-07-23T12:41:21Z

+			// If we are here, then we are not setting any default for observed audit profile.
+			// looks like returning the existing config is the best alternative here. This puts a constraint on the
+			// apiserver to have a default configuration in its bindata.
+			return existingConfig, append(errs, err)


this is wrong. You have to prune the existing config: the input has values from all observers. But you are only allowed to return your fields.

Compare pruning in the defer func in the cloud provider observer.

yes, I have added the pruner.

sttts · 2020-07-23T12:42:09Z

+				return defaultObservedConfig, errs
+			}
+
+			return defaultObservedConfig, append(errs, err)


on non-not-found errors we return the existing config. This here would change the config for the apiserver. We don't want that. It creates flapping on error situations.

on any lister error, the observer function is returning the current/existing config.

sttts · 2020-07-23T12:42:39Z

+
+		desiredProfile := string(apiServer.Spec.Audit.Profile)
+		if len(desiredProfile) == 0 {
+			return defaultObservedConfig, errs


// this should never happen because we have a default value on the field

the CRD is not co-located; the observer being a core func, it's a guard against if the CRD schema is accidentally changed.

sttts · 2020-07-23T12:43:28Z

+		}
+		if len(current) > 0 {
+			auditPolicyPath = current[0]
+		}


This should only be need further down where you compare with the new value and create an event on change. Move it there.

sttts · 2020-07-23T12:44:10Z

+
+		observedConfig := map[string]interface{}{}
+		if err := unstructured.SetNestedStringSlice(observedConfig, []string{desiredAuditPolicyPath}, apiServerArgumentsAuditPath...); err != nil {
+			return defaultObservedConfig, append(errs, fmt.Errorf("failed to set desired audit profile in observed config name=%s", desiredProfile))


return old config, not default. Avoids flapping.

it's returning the current config, I changed the variable name. I think it boils down to the question I posted here - #845 (comment).

tkashem · 2020-07-24T18:41:17Z

good for review

/hold cancel

sttts · 2020-07-24T18:42:52Z

+
+		desiredProfile := string(apiServer.Spec.Audit.Profile)
+		if len(desiredProfile) == 0 {
+			return existingConfig, errs


empty config

sttts · 2020-07-24T18:57:59Z

/lgtm
/approve

openshift-ci-robot · 2020-07-24T18:58:16Z

[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:

~~OWNERS~~ [sttts]

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

tkashem · 2020-07-24T19:20:07Z

/retest

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 22, 2020

openshift-ci-robot requested review from smarterclayton and sttts July 22, 2020 18:10

tkashem force-pushed the audit-config branch from f5d54ba to 09ec981 Compare July 22, 2020 19:43

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2020

tkashem mentioned this pull request Jul 22, 2020

Add audit policy configuration openshift/cluster-kube-apiserver-operator#899

Merged

sttts reviewed Jul 23, 2020

View reviewed changes

tkashem force-pushed the audit-config branch from 09ec981 to 0468e63 Compare July 23, 2020 14:11

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2020

tkashem force-pushed the audit-config branch from 0468e63 to f9464e7 Compare July 23, 2020 14:51

vendor openshift/api

dbb9a80

tkashem force-pushed the audit-config branch 5 times, most recently from 260ae1b to 1a2ed63 Compare July 24, 2020 18:13

tkashem changed the title ~~[WIP] Audit config~~ Audit policy config observer Jul 24, 2020

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 24, 2020

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 24, 2020

sttts reviewed Jul 24, 2020

View reviewed changes

tkashem force-pushed the audit-config branch from 1a2ed63 to 763535d Compare July 24, 2020 18:55

add an audit policy observer

4c05a68

tkashem force-pushed the audit-config branch from 763535d to 4c05a68 Compare July 24, 2020 18:56

openshift-ci-robot assigned sttts Jul 24, 2020

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

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2020

openshift-merge-robot merged commit 1ed21c4 into openshift:master Jul 24, 2020

tkashem deleted the audit-config branch July 24, 2020 19:26

Conversation

tkashem commented Jul 22, 2020

Uh oh!

tkashem commented Jul 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkashem Jul 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkashem commented Jul 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts commented Jul 24, 2020

Uh oh!

openshift-ci-robot commented Jul 24, 2020

Uh oh!

tkashem commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tkashem Jul 23, 2020 •

edited

Loading