[Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support #1061

rashmi43 · 2025-07-02T12:25:59Z

https://github.ibm.com/IBMPrivateCloud/roadmap/issues/67045

https://github.ibm.com/IBMPrivateCloud/roadmap/issues/67190 - CD

Signed-off-by: rashmi_kh <[email protected]>

ibm-ci-bot · 2025-07-02T12:26:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rashmi43

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rashmi43]

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

Signed-off-by: rashmi_kh <[email protected]>

internal/controller/operator/authentication_controller.go

internal/controller/operator/configmap.go

Signed-off-by: rashmi_kh <[email protected]>

rwhundley · 2025-08-04T16:34:22Z

internal/controller/operator/deployment.go

+// cluster, or an empty string when the Secret isn't found or cannot otherwise
+// be retrieved. If an error other than NotFound is received when trying to get
+// the Secret, that is returned as well.
+func (r *AuthenticationReconciler) getAuditSecretNameIfExists(ctx context.Context, namespace string) (string, error) {


You should just check the Authentication CR for whether the audit values are set - that's ultimately the source of truth for the workload and makes this unnecessary.

ok I will change this to check the Authentication CR

rwhundley · 2025-08-04T16:35:28Z

bundle/manifests/operator.ibm.com_authentications.yaml

@@ -228,6 +228,10 @@ spec:
                    type: string
                  providerIssuerURL:
                    type: string
+                  auditURL:


These changes should also be happening in the config directory, and changes here should be generated with make bundle

rwhundley · 2025-08-04T16:38:39Z

api/operator/v1alpha1/authentication_types.go

@@ -139,6 +139,8 @@ type ConfigSpec struct {
 	ICPPort                     int32          `json:"icpPort"`
 	FIPSEnabled                 bool           `json:"fipsEnabled"`
 	ROKSEnabled                 bool           `json:"roksEnabled"`
+	AuditUrl                    string         `json:"auditUrl,omitempty"`


If these aren't required, they should be *string, and nil checks should happen on accesses.

rwhundley · 2025-08-04T16:39:46Z

internal/controller/operator/containers.go

@@ -414,7 +414,7 @@ func buildAuthServiceContainer(instance *operatorv1alpha1.Authentication, authSe

 }

-func buildIdentityProviderContainer(instance *operatorv1alpha1.Authentication, identityProviderImage string, _ string, saasCRNId string) corev1.Container {
+func buildIdentityProviderContainer(instance *operatorv1alpha1.Authentication, identityProviderImage string, _ string, saasCRNId string, auditSecretName string) corev1.Container {


You shouldn't need this extra parameter because the secret name should be set on the Authentication CR, right?

I need this to mount the secret if Len > 0

rwhundley · 2025-08-04T16:40:21Z

internal/controller/operator/containers.go

@@ -1129,16 +1087,16 @@ func buildContainers(instance *operatorv1alpha1.Authentication, authServiceImage
 	return []corev1.Container{authServiceContainer}
 }

-func buildManagerContainers(instance *operatorv1alpha1.Authentication, identityManagerImage string, icpConsoleURL string) []corev1.Container {
+func buildManagerContainers(instance *operatorv1alpha1.Authentication, identityManagerImage string, icpConsoleURL string, auditSecretName string) []corev1.Container {


Same here - Authentication CR should have this name set on it, correct? So no new param is needed.

internal/controller/operator/containers.go

internal/controller/operator/deployment.go

rwhundley · 2025-08-04T16:50:54Z

internal/controller/operator/deployment.go

+				},
+			},
+		}
+		volumes = EnsureVolumePresent(volumes, auditVolume)


I don't think this extra function is needed; it should be possible with just an append. This function is used to build what the Operator thinks the Deployment should contain, so there's no threat of infinitely appending redundant volumes here.

It still appears to be here?

Signed-off-by: rashmi_kh <[email protected]>

Co-authored-by: Robert W. Hundley <[email protected]>

internal/controller/operator/containers.go

rwhundley · 2025-08-04T19:15:40Z

internal/controller/operator/containers.go

@@ -731,7 +731,7 @@ func buildIdentityProviderContainer(instance *operatorv1alpha1.Authentication, i

 }

-func buildIdentityManagerContainer(instance *operatorv1alpha1.Authentication, identityManagerImage string, _ string, auditSecretName string) corev1.Container {
+func buildIdentityManagerContainer(instance *operatorv1alpha1.Authentication, identityManagerImage string, _ string, auditSecretName *string) corev1.Container {


Same as here: https://github.com/IBM/ibm-iam-operator/pull/1061/files/c5b6d918a27eb769b99fbc2c71900d3d1b778920..26233d1af4ec9ead42ea4fac0316656bc7936cd6#r2252377966

rwhundley · 2025-08-04T19:15:48Z

internal/controller/operator/containers.go

@@ -1087,14 +1087,14 @@ func buildContainers(instance *operatorv1alpha1.Authentication, authServiceImage
 	return []corev1.Container{authServiceContainer}
 }

-func buildManagerContainers(instance *operatorv1alpha1.Authentication, identityManagerImage string, icpConsoleURL string, auditSecretName string) []corev1.Container {
+func buildManagerContainers(instance *operatorv1alpha1.Authentication, identityManagerImage string, icpConsoleURL string, auditSecretName *string) []corev1.Container {


Same as here: https://github.com/IBM/ibm-iam-operator/pull/1061/files/c5b6d918a27eb769b99fbc2c71900d3d1b778920..26233d1af4ec9ead42ea4fac0316656bc7936cd6#r2252377966

rwhundley · 2025-08-04T19:15:57Z

internal/controller/operator/containers.go


 	identityManagerContainer := buildIdentityManagerContainer(instance, identityManagerImage, icpConsoleURL, auditSecretName)

 	return []corev1.Container{identityManagerContainer}
 }

-func buildProviderContainers(instance *operatorv1alpha1.Authentication, identityProviderImage string, icpConsoleURL string, saasCrnId string, auditSecretName string) []corev1.Container {
+func buildProviderContainers(instance *operatorv1alpha1.Authentication, identityProviderImage string, icpConsoleURL string, saasCrnId string, auditSecretName *string) []corev1.Container {


Same as here: https://github.com/IBM/ibm-iam-operator/pull/1061/files/c5b6d918a27eb769b99fbc2c71900d3d1b778920..26233d1af4ec9ead42ea4fac0316656bc7936cd6#r2252377966

rwhundley · 2025-08-04T19:19:35Z

internal/controller/operator/containers.go

-	if len(auditSecretName) > 0 {
-		volumeMounts = EnsureVolumeMountPresent(volumeMounts, GetAuditCertsVolumeMount())
-
+	if auditSecretName != nil {


You still want a check on the dereferenced pointer value - an empty name isn't valid, either.

Suggested change

if auditSecretName != nil {

if auditSecretName != nil && *auditSecretName != "" {

internal/controller/operator/containers.go

rwhundley · 2025-08-04T19:40:31Z

internal/controller/operator/deployment.go

+	// Check for the presence of audit-endpoint configmap
+	authIdpConfigMapName := "platform-auth-idp"
+	authIdpConfigMap := &corev1.ConfigMap{}
+	idpCMLogger := log.WithValues("ConfigMap.Name", authIdpConfigMapName, "ConfigMap.Namespace", namespace)
+	if err := r.Get(ctx, types.NamespacedName{Name: authIdpConfigMapName, Namespace: namespace}, authIdpConfigMap); k8sErrors.IsNotFound(err) {
+		idpCMLogger.Info("ConfigMap was not found")
+		return nil, nil
+	} else if err != nil {
+		idpCMLogger.Error(err, "Failed to get ConfigMap")
+		return nil, err
+	}
+	if authIdpConfigMap.Data == nil {
+		idpCMLogger.Info("Invalid ConfigMap")
+		return nil, nil
+	}
+	if authIdpConfigMap.Data["AUDIT_URL"] == "" {
+		idpCMLogger.Info("Audit URL is not specified in ConfigMap; assume no Secret to mount", "key", "AUDIT_URL")
+		return nil, nil
+	}
+	if authIdpConfigMap.Data["AUDIT_SECRET"] == "" {
+		idpCMLogger.Info("Audit Secret is not specified in ConfigMap; assume no Secret", "key", "AUDIT_SECRET")
+		return nil, nil
+	}
+	auditURL = authIdpConfigMap.Data["AUDIT_URL"]
+	auditSecretName = authIdpConfigMap.Data["AUDIT_SECRET"]
+	idpCMLogger.Info("Fetched audit URL and audit Secret from ConfigMap", "AUDIT_SECRET", auditSecretName, "AUDIT_URL", auditURL)


I apologize if I said this before - this section is unnecessary and shouldn't be here because the operating assumption is that this configuration is coming from the Authentication CR, not the ConfigMap.

That said, if the configuration is expected to be set on the ConfigMap before the Authentication CR, e.g. some previous support for this configuration in a different release stream, then the bootstrap controller needs to be updated to perform this identification so that the Authentication CR can be updated to have the correct settings on it before it hits the main Authentication controller.

For reference, you'd add "AUDIT_URL" and "AUDIT_SECRET" and their respective Auth CR property paths to this map, which handles setting the Auth CR properties you specify using values in the platform-auth-idp ConfigMap.

Oh, and all of the fields referenced in that map currently are kept as references, but, assuming you've changed auditUrl and auditSecret to be *string, you should be able to just do something like the following:

// Note no "&" - this property is already a pointer (assuming you've made that change) "AUDIT_URL": authCR.Spec.Config.AuditUrl,

rwhundley · 2025-08-04T19:42:50Z

internal/controller/operator/deployment.go

@@ -1032,4 +1095,42 @@ func buildIdpVolumes(ldapCACert string, routerCertSecret string) []corev1.Volume
 			},
 		},
 	}
+	if auditSecretName != nil {


Suggested change

if auditSecretName != nil {

if auditSecretName != nil && *auditSecretName != "" {

Though it would be better to do as I suggested here - invert this condition and return if that evaluates as true. Reduces the amount of indentation.

rwhundley · 2025-08-04T19:52:56Z

internal/controller/operator/configmap.go

@@ -345,6 +345,10 @@ func updatePlatformAuthIDP(_ common.SecondaryReconciler, _ context.Context, obse
 			"LDAP_CTX_POOL_PREFERREDSIZE"),
 		updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("MASTER_PATH")),
 			"MASTER_PATH"),
+		updatesValuesWhen(not(observedKeySet[*corev1.ConfigMap]("AUDIT_URL")),


In conjunction with this comment, it's my feeling that these should just be added to the updatesAlways call, and, if we're trying to account for an existing setting on the ConfigMap from an upgrade, then we should account for that in the bootstrap controller.

rwhundley · 2025-08-04T20:11:48Z

internal/controller/operator/deployment.go

+				},
+			},
+		}
+		volumes = EnsureVolumePresent(volumes, auditVolume)


It still appears to be here?

Co-authored-by: Robert W. Hundley <[email protected]>

Signed-off-by: rashmi_kh <[email protected]>

rwhundley · 2025-08-06T13:14:13Z

internal/controller/operator/containers.go

+			MountPath: "/pgsql/clientinfo",
+		},
+	}
+	if auditSecretName != nil && *auditSecretName != "" {


Actually, in hindsight, probably better to check if auditSecretName == nil || *auditSecretName == "" and return volumeMounts if true. Then this update to volumeMounts can happen after it without the extra indentation.

rwhundley · 2025-08-06T13:16:39Z

internal/controller/operator/deployment.go

@@ -1032,4 +1095,42 @@ func buildIdpVolumes(ldapCACert string, routerCertSecret string) []corev1.Volume
 			},
 		},
 	}
+	if auditSecretName != nil {


Though it would be better to do as I suggested here - invert this condition and return if that evaluates as true. Reduces the amount of indentation.

rwhundley · 2025-08-07T16:37:16Z

internal/controller/operator/deployment.go

@@ -82,7 +82,7 @@ func (r *AuthenticationReconciler) handleDeployments(ctx context.Context, req ct
 		samlConsoleURL = icpConsoleURL
 	}

-	auditSecretName, err := r.getAuditSecretNameIfExists(ctx, req.Namespace)
+	auditSecretName, err := r.getAuditSecretNameIfExists(context.TODO(), req.Namespace, authCR.Spec.Config.AuditUrl, authCR.Spec.Config.AuditSecret)


This should be ctx again for the context argument. Also you could just pass the pointer to the authentication, at which point you can check both of the values within the function.

You also get the namespace that way, so you go from four params to two.

rwhundley · 2025-08-07T17:40:19Z

internal/controller/operator/deployment.go

-	}
-	if authIdpConfigMap.Data["AUDIT_URL"] == "" {
-		idpCMLogger.Info("Audit URL is not specified in ConfigMap; assume no Secret to mount", "key", "AUDIT_URL")
+func (r *AuthenticationReconciler) getAuditSecretNameIfExists(ctx context.Context, namespace string, auditUrl *string, auditSecretName *string) (*string, error) {


This function should just take two arguments, a context.Context and an *operatorv1alpha1.Authentication. Then values are obtained via dereference from there.

rwhundley · 2025-08-07T17:40:51Z

internal/controller/operator/deployment.go

+	//var auditURL string
+	reqLogger := logf.FromContext(ctx)
+
+	if auditUrl == nil {


Please also check for an empty string.

watch audit-tls secret to update deployments for audit

65f2e31

Signed-off-by: rashmi_kh <[email protected]>

rashmi43 and others added 4 commits August 1, 2025 15:38

pass autdit url from authn CR

a4aa178

Signed-off-by: rashmi_kh <[email protected]>

change the logic to use secret length

27ae7e0

Signed-off-by: rashmi_kh <[email protected]>

Merge branch 'master' into audit-tls-cp4s

4995af4

print audit variables

08ebc79

Signed-off-by: rashmi_kh <[email protected]>

rashmi43 requested a review from rwhundley August 4, 2025 16:20

rashmi43 changed the title ~~[Audit log forwarding] - watch audit-tls secret to update deployments for audit~~ [Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support Aug 4, 2025

rwhundley requested changes Aug 4, 2025

View reviewed changes

internal/controller/operator/authentication_controller.go Show resolved Hide resolved

internal/controller/operator/configmap.go Outdated Show resolved Hide resolved

internal/controller/operator/configmap.go Outdated Show resolved Hide resolved

rashmi43 added 3 commits August 4, 2025 22:02

update variable name

c5b6d91

Signed-off-by: rashmi_kh <[email protected]>

remove watch

26a0204

Signed-off-by: rashmi_kh <[email protected]>

remove watch

1d61bf7

Signed-off-by: rashmi_kh <[email protected]>

rwhundley requested changes Aug 4, 2025

View reviewed changes

rashmi43 and others added 4 commits August 4, 2025 22:31

add to auth yaml samples

6f0ab93

Signed-off-by: rashmi_kh <[email protected]>

add to auth yaml samples

1557b97

Signed-off-by: rashmi_kh <[email protected]>

change to stringpointer

6a99b9c

Signed-off-by: rashmi_kh <[email protected]>

Update internal/controller/operator/deployment.go

26233d1

Co-authored-by: Robert W. Hundley <[email protected]>

rwhundley requested changes Aug 4, 2025

View reviewed changes

rashmi43 requested review from rwhundley and Tirumalavasa August 5, 2025 08:52

rashmi43 and others added 3 commits August 5, 2025 14:24

Update internal/controller/operator/containers.go

e6eb35b

Co-authored-by: Robert W. Hundley <[email protected]>

change to string pointer

91dafe2

Signed-off-by: rashmi_kh <[email protected]>

change to string pointer

a76590a

Signed-off-by: rashmi_kh <[email protected]>

rwhundley requested changes Aug 8, 2025

View reviewed changes

	if auditSecretName != nil {
	if auditSecretName != nil && *auditSecretName != "" {

[Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support #1061

Are you sure you want to change the base?

[Audit log forwarding] - update audit-tls secret to IM deployments for audit forwarding support #1061

Uh oh!

Conversation

rashmi43 commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ibm-ci-bot commented Jul 2, 2025

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

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!

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!

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!

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!

Uh oh!

rashmi43 commented Jul 2, 2025 •

edited

Loading