OCPBUGS-33060: enable audit log for oauth-openshift #3994

heliubj18 · 2024-05-07T17:22:06Z

What this PR does / why we need it:
It enables audit logs for oauth-openshift.
Currently, hypershift only supports audit logs for kube-apiserver, openshift-apiserver, and openshift-oauth-apiserver. However, oauth-openshift also supports audit logs in standalone OCP.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes # OCPBUGS-33060

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
[ *] This change includes unit tests.

openshift-ci-robot · 2024-05-07T17:22:12Z

@heliubj18: This pull request references Jira Issue OCPBUGS-33060, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

What this PR does / why we need it:
It enables audit logs for oauth-openshift.
Currently, hypershift only supports audit logs for kube-apiserver, openshift-apiserver, and openshift-oauth-apiserver. However, oauth-openshift also supports audit logs in standalone OCP.

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes # OCPBUGS-33060

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

[ *] This change includes unit tests.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-05-07T17:26:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: heliubj18
Once this PR has been reviewed and has the lgtm label, please assign csrwng for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

netlify · 2024-05-08T11:30:19Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`19f0cd2`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/663b6208efa2690008759664
😎 Deploy Preview	https://deploy-preview-3994--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

heliubj18 · 2024-05-08T15:57:11Z

/jira refresh

openshift-ci-robot · 2024-05-08T15:57:15Z

@heliubj18: This pull request references Jira Issue OCPBUGS-33060, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

heliubj18 · 2024-05-08T15:58:11Z

/jira refresh

openshift-ci-robot · 2024-05-08T15:58:14Z

@heliubj18: This pull request references Jira Issue OCPBUGS-33060, which is invalid:

expected the bug to target the "4.16.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

stevekuznetsov · 2024-05-09T15:25:37Z

control-plane-operator/controllers/hostedcontrolplane/oauth/deployment_test.go

 tc.serverParams.AvailabilityProberImage, tc.serverParams.NamedCertificates(), tc.serverParams.Socks5ProxyImage, tc.serverParams.NoProxy, &tc.configParams, hyperv1.IBMCloudPlatform)
 g.Expect(err).To(BeNil())
 g.Expect(expectedMinReadySeconds).To(Equal(oauthDeployment.Spec.MinReadySeconds))
+ for _, container := range oauthDeployment.Spec.Template.Spec.Containers {
+ if container.Name == oauthContainerMain().Name {


Is it a lot of work to move any of this to golden files? Test cases that do this are hard to reason about, getting a specific expectation on output per test case would be great.

Do you mean to create a testdata directory to store the expected parameters? I don't find any existing testdata in the repo. I'm not sure if it's necessary to include such parameter tests in the e2e testing. I could also remove them if it's okay to ignore them. Thanks!

We have a number of test fixtures - see uses of CompareWithFixture. This is a unit test, so it may be easy and quick to get those integrated instead of conditional validation logic. If you'd rather not do that, could you please refactor the unit test so that each test declares the validations it needs, instead of having a loosely-coupled conditional validation that applies to all in theory but only one in practice? Once you have expected-data-per-test, I think using CompareWithFixture() will be the fastest way to assert it.

done, thanks!

stevekuznetsov · 2024-05-09T15:25:53Z

control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go

 return func(c *corev1.Container) {
 c.Image = image
 c.Args = []string{
 "osinserver",
 fmt.Sprintf("--config=%s", path.Join(volumeMounts.Path(c.Name, oauthVolumeConfig().Name), OAuthServerConfigKey)),
+ "--audit-log-format=json",


Do we have provenance for these specific flags?

They are used in the oauth-openshift repo here: https://github.com/openshift/cluster-authentication-operator/blob/b415439ebab2829c8da1ea17c05f2ac75fe5dbe8/bindata/oauth-apiserver/deploy.yaml#L68

And I also checked a real oauth-openshift pod in the standalone OCP

$ oc get deploy -n openshift-authentication oauth-openshift -oyaml ... - args: - | if [ -s /var/config/system/configmaps/v4-0-config-system-trusted-ca-bundle/ca-bundle.crt ]; then echo "Copying system trust bundle" cp -f /var/config/system/configmaps/v4-0-config-system-trusted-ca-bundle/ca-bundle.crt /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem fi exec oauth-server osinserver \ --config=/var/config/system/configmaps/v4-0-config-system-cliconfig/v4-0-config-system-cliconfig \ --v=2 \ --audit-log-format=json \ --audit-log-maxbackup=10 \ --audit-log-maxsize=100 \ --audit-log-path=/var/log/oauth-server/audit.log \ --audit-policy-file=/var/run/configmaps/audit/audit.yaml command: - /bin/bash - -ec image: quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:9c5863100af97f244bc0af6c727785a7487eb8514ee549a41089bf6b06315e0f ... name: oauth-openshift

The only difference is --audit-log-maxbackup=1. Because in hypershift codes, it is set to 1 for the existing audit components (kas, oapi).

Awesome, thanks for the explanation!

heliubj18 · 2024-05-09T18:07:36Z

/jira refresh

openshift-ci-robot · 2024-05-09T18:07:41Z

@heliubj18: This pull request references Jira Issue OCPBUGS-33060, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.16.0) matches configured target version for branch (4.16.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @xingxingxia

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-05-09T21:14:56Z

@heliubj18: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

heliubj18 · 2024-05-13T11:15:03Z

@stevekuznetsov I checked the Konflux failed tasks, and I don't think they are related to my changes. The failures seem to be related to image builds (pp64le)

stevekuznetsov · 2024-05-13T22:30:46Z

Yep, those are fairly common and not actionable other than to

/retest

stevekuznetsov · 2024-05-13T22:33:46Z

/lgtm

xingxingxia · 2024-05-15T08:21:23Z

/label qe-approved

heliubj18 · 2024-05-23T07:43:21Z

@stevekuznetsov could you approve it? thanks!

Joseph-Goergen · 2024-05-24T14:24:25Z

@heliubj18 You may want to rebase to pull in #3972

hypershift/control-plane-operator/controllers/hostedcontrolplane/kas/deployment.go

Lines 970 to 986 in 82df7b3

 func RenderAuditLogScript(auditLogFilePath string) string { 

 var script = ` 

 set -o errexit 

 set -o nounset 

 set -o pipefail 

 function cleanup() { 

  kill -- -$$ 

  wait 

 } 

 trap cleanup SIGTERM 

 /usr/bin/tail -c+1 -F %s & 

 wait $! 

 ` 

 return fmt.Sprintf(script, auditLogFilePath) 

 }

hypershift/control-plane-operator/controllers/hostedcontrolplane/kas/deployment.go

Lines 239 to 247 in 82df7b3

 Name: "audit-logs", 

 Image: images.CLI, 

 ImagePullPolicy: corev1.PullIfNotPresent, 

 Command: []string{"/bin/bash"}, 

 Args: []string{ 

 "-c", 

 RenderAuditLogScript(fmt.Sprintf("%s/%s", volumeMounts.Path(kasContainerMain().Name, kasVolumeWorkLogs().Name), "audit.log")), 

 }, 

 Resources: corev1.ResourceRequirements{

rtheis · 2024-05-24T16:56:42Z

control-plane-operator/controllers/hostedcontrolplane/oauth/deployment.go

 {Name: "admin-kubeconfig", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "service-network-admin-kubeconfig", DefaultMode: utilpointer.Int32(0640)}}},
 {Name: "konnectivity-proxy-cert", VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: manifests.KonnectivityClientSecret("").Name, DefaultMode: utilpointer.Int32(0640)}}},
 {Name: "konnectivity-proxy-ca", VolumeSource: corev1.VolumeSource{ConfigMap: &corev1.ConfigMapVolumeSource{LocalObjectReference: corev1.LocalObjectReference{Name: manifests.KonnectivityCAConfigMap("").Name}, DefaultMode: utilpointer.Int32(0640)}}},
 },
 }
+
+ if auditConfig.Data[auditPolicyProfileMapKey] != string(configv1.NoneAuditProfileType) {
+ deployment.Spec.Template.Spec.Containers = append(deployment.Spec.Template.Spec.Containers, corev1.Container{


Please update this PR based on #3972

openshift-ci bot added do-not-merge/needs-area do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 7, 2024

openshift-ci bot requested review from csrwng and sjenning May 7, 2024 17:25

openshift-ci bot added the area/control-plane-operator Indicates the PR includes changes for the control plane operator - in an OCP release label May 7, 2024

openshift-ci bot removed the do-not-merge/needs-area label May 7, 2024

heliubj18 force-pushed the enable-oauth-openshift-audit-logs branch from a54adbd to 19f0cd2 Compare May 8, 2024 11:29

heliubj18 changed the title ~~[WIP] OCPBUGS-33060: enable audit log for oauth-openshift~~ OCPBUGS-33060: enable audit log for oauth-openshift May 8, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 8, 2024

stevekuznetsov reviewed May 9, 2024

View reviewed changes

enable audit log for oauth-openshift

5db4b5a

heliubj18 force-pushed the enable-oauth-openshift-audit-logs branch from 19f0cd2 to 5db4b5a Compare May 9, 2024 18:01

openshift-ci-robot added jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. and removed jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. labels May 9, 2024

openshift-ci bot requested a review from xingxingxia May 9, 2024 18:07

openshift-ci bot assigned stevekuznetsov May 13, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 13, 2024

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label May 15, 2024

Joseph-Goergen mentioned this pull request May 15, 2024

OCPBUGS-33181: Fixed audit-logs sigterm failing to terminate gracefully #3972

Merged

4 tasks

rtheis suggested changes May 24, 2024

View reviewed changes

openshift-ci bot assigned rtheis May 24, 2024

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OCPBUGS-33060: enable audit log for oauth-openshift #3994

OCPBUGS-33060: enable audit log for oauth-openshift #3994

heliubj18 commented May 7, 2024 •

edited

openshift-ci-robot commented May 7, 2024

openshift-ci bot commented May 7, 2024

netlify bot commented May 8, 2024

heliubj18 commented May 8, 2024

openshift-ci-robot commented May 8, 2024

heliubj18 commented May 8, 2024

openshift-ci-robot commented May 8, 2024

stevekuznetsov May 9, 2024

heliubj18 May 9, 2024

stevekuznetsov May 9, 2024

heliubj18 May 9, 2024

stevekuznetsov May 9, 2024

heliubj18 May 9, 2024

stevekuznetsov May 9, 2024

heliubj18 commented May 9, 2024

openshift-ci-robot commented May 9, 2024

openshift-ci bot commented May 9, 2024

heliubj18 commented May 13, 2024

stevekuznetsov commented May 13, 2024

stevekuznetsov commented May 13, 2024

xingxingxia commented May 15, 2024

heliubj18 commented May 23, 2024

Joseph-Goergen commented May 24, 2024

rtheis May 24, 2024

OCPBUGS-33060: enable audit log for oauth-openshift #3994

Are you sure you want to change the base?

OCPBUGS-33060: enable audit log for oauth-openshift #3994

Conversation

heliubj18 commented May 7, 2024 • edited

openshift-ci-robot commented May 7, 2024

openshift-ci bot commented May 7, 2024

netlify bot commented May 8, 2024

✅ Deploy Preview for hypershift-docs ready!

heliubj18 commented May 8, 2024

openshift-ci-robot commented May 8, 2024

heliubj18 commented May 8, 2024

openshift-ci-robot commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heliubj18 commented May 9, 2024

openshift-ci-robot commented May 9, 2024

openshift-ci bot commented May 9, 2024

heliubj18 commented May 13, 2024

stevekuznetsov commented May 13, 2024

stevekuznetsov commented May 13, 2024

xingxingxia commented May 15, 2024

heliubj18 commented May 23, 2024

Joseph-Goergen commented May 24, 2024

Choose a reason for hiding this comment

heliubj18 commented May 7, 2024 •

edited