PoC: Fake import to verify KMS integration #1951

ardaguclu · 2025-10-27T09:12:12Z

Fake import to exercise openshift/library-go#2041
/hold

coderabbitai · 2025-10-27T09:12:32Z

Walkthrough

Updates the go.mod replace directive to a newer pseudo-version, adds a hostPath volume audit-dir to the kube-apiserver pod manifest, and implements KMS plugin sidecar injection with wiring through operator startup and TargetConfigController.

Changes

Cohort / File(s)	Change Summary
Dependency Replace Directive `go.mod`	Updates the `replace github.com/openshift/library-go` directive to pseudo-version `v0.0.0-20251105061910-519a2c27c78d`. No other `go.mod` behavioral changes.
Kube API Server Manifest `bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml`	Adds a `hostPath` volume named `audit-dir` with path `/var/log/kube-apiserver`. Existing initContainer/volumeMount references that use `audit-dir` remain unchanged.
Operator startup wiring `pkg/operator/starter.go`	Reads `KMS_PLUGIN_IMAGE` from environment and passes it into `NewTargetConfigController` call site; also passes `configInformers.Config().V1().APIServers()` (APIServers informer) into the TargetConfigController constructor.
KMS plugin implementation `pkg/operator/targetconfigcontroller/kms_plugin.go`	New file adding KMS plugin injection logic: defines `KMSPluginImageEnvVar` and `DefaultKMSPluginImage`; implements `getKMSEncryptionConfig(...)` to obtain APIServer KMS config and `injectKMSPlugin(...)` to resolve image, build a `kms.ContainerConfig`, and call `kms.AddKMSPluginToPodSpec`. Includes logging and error handling.
TargetConfigController wiring & behavior `pkg/operator/targetconfigcontroller/targetconfigcontroller.go`	Adds `kmsPluginImage string` and `apiserverLister configv1listers.APIServerLister` fields; updates `NewTargetConfigController(...)` signature to accept `kmsPluginImage` and `apiserverInformer`, initializes them; passes `apiserverLister` and `kmsPluginImage` into `managePods`; `managePods` now reads observedConfig for proxy env injection and invokes KMS plugin injection when assembling pod templates.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Inspect pkg/operator/targetconfigcontroller/kms_plugin.go handling when APIServer resource is missing, when KMS is disabled, and error cases.
Verify injectKMSPlugin builds correct container and volume specs and preserves hostNetwork behavior.
Confirm constructor/signature changes in targetconfigcontroller.go are consistently applied and callers updated.
Validate starter.go wiring of KMS_PLUGIN_IMAGE and APIServer informer ordering/availability.
Check the audit-dir hostPath path and its intended usage in the manifest.
Confirm the updated go.mod replace pseudo-version is intentional.

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title 'PoC: Fake import to verify KMS integration' directly and clearly describes the main change in the changeset. The title accurately reflects that this is a proof-of-concept that introduces a fake import (the replace directive in go.mod) to verify KMS integration functionality. It is concise, specific, and captures the primary intent of the changes.
Description check	✅ Passed	The pull request description 'Fake import to exercise openshift/library-go#2041' is directly related to the changeset. It clearly indicates that the changes are designed to test a specific external PR, which aligns with the nature of the changes made (the go.mod replace directive and KMS volume mount). The description provides meaningful context about the purpose of these modifications.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 071e794 and 23f5281.

⛔ Files ignored due to path filters (3)

go.sum is excluded by !**/*.sum
vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/plugin.go is excluded by !vendor/**, !**/vendor/**
vendor/modules.txt is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (1)

go.mod (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

go.mod

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2025-10-27T09:12:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ardaguclu
Once this PR has been reviewed and has the lgtm label, please assign tkashem for approval. For more information see the 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

ardaguclu · 2025-10-27T11:23:12Z

/retest

ardaguclu · 2025-10-27T12:08:03Z

/retest

ardaguclu · 2025-10-27T14:30:42Z

/retest

ardaguclu · 2025-11-03T08:37:53Z

/retest

ardaguclu · 2025-11-03T12:05:39Z

/retest

coderabbitai

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between a11ec65 and 071e794.

⛔ Files ignored due to path filters (3)

go.sum is excluded by !**/*.sum
vendor/github.com/openshift/library-go/pkg/operator/encryption/kms/plugin.go is excluded by !vendor/**, !**/vendor/**
vendor/modules.txt is excluded by !vendor/**, !**/vendor/**

📒 Files selected for processing (5)

bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml (1 hunks)
go.mod (1 hunks)
pkg/operator/starter.go (1 hunks)
pkg/operator/targetconfigcontroller/kms_plugin.go (1 hunks)
pkg/operator/targetconfigcontroller/targetconfigcontroller.go (7 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

go.mod

🧰 Additional context used

🪛 YAMLlint (1.37.1)

bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml

[warning] 141-141: too many blank lines (1 > 0)

(empty-lines)

🔇 Additional comments (1)

bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml (1)

138-140: Volume definition correctly complements existing mounts.

The addition of the audit-dir hostPath volume is properly structured and aligns with the existing volumeMount references in both the initContainer and main container. The change is appropriate for the audit logging use case.

coderabbitai · 2025-11-04T09:53:55Z

bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml

  - hostPath:
      path: /var/log/kube-apiserver
    name: audit-dir
+


⚠️ Potential issue | 🟡 Minor

Remove trailing blank line.

The YAML file has an extra blank line at the end of the volumes section that violates linting rules.

Apply this diff to remove the trailing blank line:

- hostPath: path: /var/log/kube-apiserver name: audit-dir -

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

- hostPath:

path: /var/log/kube-apiserver

name: audit-dir

🧰 Tools

🪛 YAMLlint (1.37.1)

[warning] 141-141: too many blank lines (1 > 0)

(empty-lines)

🤖 Prompt for AI Agents

In bindata/bootkube/bootstrap-manifests/kube-apiserver-pod.yaml around line 141, there is an extra trailing blank line at the end of the volumes section; remove that blank line so the file ends immediately after the last volume entry (no extra newline-only line), ensuring the YAML has no blank line violation per linting rules.

coderabbitai · 2025-11-04T09:53:56Z

pkg/operator/targetconfigcontroller/kms_plugin.go

+	klog.Infof("KMS encryption enabled with type=%s, region=%s, keyARN=%s",
+		apiserver.Spec.Encryption.KMS.Type,
+		apiserver.Spec.Encryption.KMS.AWS.Region,
+		apiserver.Spec.Encryption.KMS.AWS.KeyARN)


⚠️ Potential issue | 🔴 Critical

Guard against nil AWS KMS config before logging.

apiserver.Spec.Encryption.KMS.AWS is dereferenced unconditionally, so any cluster using a non-AWS KMS provider (or with an incomplete config during rollout) will panic the operator here. Please gate the logging on KMS.AWS != nil (or branch by provider) before touching those fields.

- klog.Infof("KMS encryption enabled with type=%s, region=%s, keyARN=%s", - apiserver.Spec.Encryption.KMS.Type, - apiserver.Spec.Encryption.KMS.AWS.Region, - apiserver.Spec.Encryption.KMS.AWS.KeyARN) + klog.Infof("KMS encryption enabled with type=%s", apiserver.Spec.Encryption.KMS.Type) + if awsConfig := apiserver.Spec.Encryption.KMS.AWS; awsConfig != nil { + klog.Infof("KMS AWS config: region=%s, keyARN=%s", awsConfig.Region, awsConfig.KeyARN) + }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

klog.Infof("KMS encryption enabled with type=%s, region=%s, keyARN=%s",

apiserver.Spec.Encryption.KMS.Type,

apiserver.Spec.Encryption.KMS.AWS.Region,

apiserver.Spec.Encryption.KMS.AWS.KeyARN)

klog.Infof("KMS encryption enabled with type=%s", apiserver.Spec.Encryption.KMS.Type)

if awsConfig := apiserver.Spec.Encryption.KMS.AWS; awsConfig != nil {

klog.Infof("KMS AWS config: region=%s, keyARN=%s", awsConfig.Region, awsConfig.KeyARN)

}

🤖 Prompt for AI Agents

In pkg/operator/targetconfigcontroller/kms_plugin.go around lines 52 to 55, the code unconditionally dereferences apiserver.Spec.Encryption.KMS.AWS which will panic if AWS KMS config is nil; update the logging to first check that apiserver.Spec.Encryption.KMS != nil and apiserver.Spec.Encryption.KMS.AWS != nil (or branch by provider) and only include region/keyARN in the message when AWS config exists, otherwise log a provider-only message or omit AWS fields to avoid nil dereference.

ardaguclu · 2025-11-04T12:11:49Z

/retest

openshift-ci · 2025-11-05T09:51:20Z

@ardaguclu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-scos-e2e-aws-ovn	`23f5281`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-aws-ovn	`23f5281`	link	true	`/test e2e-aws-ovn`

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.

openshift-merge-robot · 2025-11-05T09:51:24Z

PR needs rebase.

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.

ardaguclu · 2025-11-05T11:31:54Z

/close

openshift-ci · 2025-11-05T11:32:22Z

@ardaguclu: Closed this PR.

In response to this:

/close

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.

Fake import to verify KMS integration

71962d8

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2025

openshift-ci bot requested review from dinhxuanvu and sanchezl October 27, 2025 09:12

ardaguclu mentioned this pull request Oct 27, 2025

NO-JIRA: Introduce KMS encryption mode in encryption library openshift/library-go#2041

Closed

ardaguclu added 3 commits October 27, 2025 19:47

Update based on latest changes

eefd6e0

fix encryption type

df55e94

Temporarily disable KMS plugin access

01ed3d1

Mount hostPath

2afae7d

ardaguclu force-pushed the test-kms branch from 7e3bc78 to 2afae7d Compare November 3, 2025 09:53

ardaguclu added 3 commits November 3, 2025 17:12

Get library-go changes

a11ec65

Add PoC plugin lifecycle

e603a1a

Use custom plugin image

071e794

coderabbitai bot reviewed Nov 4, 2025

View reviewed changes

Revendor library-go

23f5281

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2025

openshift-ci bot closed this Nov 5, 2025

ardaguclu deleted the test-kms branch November 5, 2025 11:34

PoC: Fake import to verify KMS integration #1951

PoC: Fake import to verify KMS integration #1951

Uh oh!

Conversation

ardaguclu commented Oct 27, 2025

Uh oh!

coderabbitai bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

openshift-ci bot commented Oct 27, 2025

Uh oh!

ardaguclu commented Oct 27, 2025

Uh oh!

ardaguclu commented Oct 27, 2025

Uh oh!

ardaguclu commented Oct 27, 2025

Uh oh!

ardaguclu commented Nov 3, 2025

Uh oh!

ardaguclu commented Nov 3, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

ardaguclu commented Nov 4, 2025

Uh oh!

openshift-ci bot commented Nov 5, 2025

Uh oh!

openshift-merge-robot commented Nov 5, 2025

Uh oh!

ardaguclu commented Nov 5, 2025

Uh oh!

openshift-ci bot commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Oct 27, 2025 •

edited

Loading