CNTRLPLANE-3363: Inject health reporter as sidecar container#2317
CNTRLPLANE-3363: Inject health reporter as sidecar container#2317ardaguclu wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@ardaguclu: This pull request references CNTRLPLANE-3363 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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
Bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesKMS Plugin Health Reporter Sidecar
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go (1)
576-585: ⚡ Quick winAdd a wrapper-level negative test for empty
operatorImage.Now that
AddKMSPluginSidecarToPodSpecrequiresoperatorImage, add a table case (and idempotency guard if needed) asserting empty image returns the expected validation error from the builder. This protects the new public API contract from silent regressions.Also applies to: 601-605
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go` around lines 576 - 585, Add a new test case to the tests table in the table-driven test that validates empty operatorImage parameter rejection. Create a test case entry where operatorImage is set to an empty string (instead of "quay.io/test/operator:latest") and set the corresponding tt.wantErr field to match the validation error message that AddKMSPluginSidecarToPodSpec should return when operatorImage is empty. This ensures the function properly validates the required operatorImage parameter and protects the public API contract from regression.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/builder_test.go`:
- Around line 208-218: The Apply method in the KMSPluginBuilder currently
validates healthReporterImage after mutating podSpec, which violates atomicity
on error. In the test case for empty image, add an assertion that verifies
podSpec remains unchanged after Apply returns an error (by comparing the podSpec
state before and after the call). Additionally, move the healthReporterImage
validation logic earlier in the Apply method implementation to occur before any
mutations to podSpec are made, ensuring that validation failures cannot leave
partial sidecar or volume mutations behind.
In `@pkg/operator/encryption/kms/pluginlifecycle/builder.go`:
- Around line 176-179: The health reporter container receives a kubeconfig
argument but the corresponding volume mount is not being added. After the
healthReporterSocketMount logic (around lines 176-179) where
ensureVolumeMountInContainer is called with kmsPluginSocketMount, add similar
logic to check if b.healthReporterKubeconfig is non-empty, and if so, create a
volume mount for the kubeconfig file and add it to the podSpec using
ensureVolumeMountInContainer. Additionally, ensure the actual volume is added to
the pod spec using ensureVolumeInPodSpec to mount the kubeconfig file from the
host filesystem, since the container has ReadOnlyRootFilesystem set to true.
---
Nitpick comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go`:
- Around line 576-585: Add a new test case to the tests table in the
table-driven test that validates empty operatorImage parameter rejection. Create
a test case entry where operatorImage is set to an empty string (instead of
"quay.io/test/operator:latest") and set the corresponding tt.wantErr field to
match the validation error message that AddKMSPluginSidecarToPodSpec should
return when operatorImage is empty. This ensures the function properly validates
the required operatorImage parameter and protects the public API contract from
regression.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 109d0e12-62d3-48a4-aec4-350f6df91146
📒 Files selected for processing (4)
pkg/operator/encryption/kms/pluginlifecycle/builder.gopkg/operator/encryption/kms/pluginlifecycle/builder_test.gopkg/operator/encryption/kms/pluginlifecycle/sidecar.gopkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
| t.Run("empty image: returns error", func(t *testing.T) { | ||
| podSpec := &corev1.PodSpec{ | ||
| Containers: []corev1.Container{{Name: "kube-apiserver"}}, | ||
| Volumes: []corev1.Volume{f.resourceDirVolume}, | ||
| } | ||
| err := NewKMSPluginBuilder(). | ||
| FromEncryptionConfig("encryption-config", cfg). | ||
| WithHealthReporter("kms-health-reporter", "", ""). | ||
| Apply(podSpec, "kube-apiserver") | ||
| require.ErrorContains(t, err, "health reporter image is required") | ||
| }) |
There was a problem hiding this comment.
Preserve atomicity on error for the empty-image path.
This case only checks the error message, but Apply currently validates healthReporterImage after mutating podSpec. Add an unchanged-on-error assertion here and move the image validation earlier in Apply so this path cannot leave partial sidecar/volume mutations behind.
Suggested test hardening in this subtest
t.Run("empty image: returns error", func(t *testing.T) {
podSpec := &corev1.PodSpec{
Containers: []corev1.Container{{Name: "kube-apiserver"}},
Volumes: []corev1.Volume{f.resourceDirVolume},
}
+ original := podSpec.DeepCopy()
err := NewKMSPluginBuilder().
FromEncryptionConfig("encryption-config", cfg).
WithHealthReporter("kms-health-reporter", "", "").
Apply(podSpec, "kube-apiserver")
require.ErrorContains(t, err, "health reporter image is required")
+ require.Equal(t, original, podSpec, "pod spec should be unchanged on error")
})🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/operator/encryption/kms/pluginlifecycle/builder_test.go` around lines 208
- 218, The Apply method in the KMSPluginBuilder currently validates
healthReporterImage after mutating podSpec, which violates atomicity on error.
In the test case for empty image, add an assertion that verifies podSpec remains
unchanged after Apply returns an error (by comparing the podSpec state before
and after the call). Additionally, move the healthReporterImage validation logic
earlier in the Apply method implementation to occur before any mutations to
podSpec are made, ensuring that validation failures cannot leave partial sidecar
or volume mutations behind.
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
0744aff to
555fb53
Compare
|
/uncc @dgrisonnet |
openshift-ci
Bot
requested review from
bertinatto and
ibihim
and removed request for
dgrisonnet
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
555fb53 to
870ba88
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/operator/encryption/kms/pluginlifecycle/builder.go (1)
243-248: 💤 Low valueConsider adding resource limits for the health reporter container.
Only resource requests are set (32Mi memory, 10m CPU) without corresponding limits. While this is sometimes intentional for sidecar containers to avoid OOM kills during spikes, it could also lead to resource contention on nodes if the health reporter misbehaves. Consider whether setting limits (or documenting why they're omitted) would be appropriate for this security-sensitive component.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/kms/pluginlifecycle/builder.go` around lines 243 - 248, The ResourceRequirements object in the health reporter container configuration only defines Requests (32Mi memory and 10m CPU) without corresponding Limits. Add a Limits field to the ResourceRequirements struct with appropriate memory and CPU limits (typically equal to or slightly higher than the requests) to prevent resource contention and ensure proper resource governance for this security-sensitive component. Alternatively, if limits are intentionally omitted, add a code comment explaining the rationale.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/builder.go`:
- Around line 243-248: The ResourceRequirements object in the health reporter
container configuration only defines Requests (32Mi memory and 10m CPU) without
corresponding Limits. Add a Limits field to the ResourceRequirements struct with
appropriate memory and CPU limits (typically equal to or slightly higher than
the requests) to prevent resource contention and ensure proper resource
governance for this security-sensitive component. Alternatively, if limits are
intentionally omitted, add a code comment explaining the rationale.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5951874d-d325-418c-81bb-753aa4cdbf66
📒 Files selected for processing (4)
pkg/operator/encryption/kms/pluginlifecycle/builder.gopkg/operator/encryption/kms/pluginlifecycle/builder_test.gopkg/operator/encryption/kms/pluginlifecycle/sidecar.gopkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/operator/encryption/kms/pluginlifecycle/builder_test.go
- pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
|
|
||
| var kubeconfig string | ||
| if b.staticPod { | ||
| kubeconfig = defaultStaticPodKubeconfig |
There was a problem hiding this comment.
Nit: I know that it is idiomatic in K8s code, but the apply function is very long. We could split it up?
There was a problem hiding this comment.
Good idea. I've moved the logic inside applyHealthReporter.
| // TODO: move to a dedicated least-privilege kubeconfig once available. | ||
| const defaultStaticPodKubeconfig = "/etc/kubernetes/static-pod-resources/configmaps/kube-apiserver-cert-syncer-kubeconfig/kubeconfig" | ||
|
|
||
| type healthReporter struct { |
There was a problem hiding this comment.
Nit: I know that long files are also idiomatic in K8s, but maybe it would deserve its own file?
There was a problem hiding this comment.
Good idea. I've moved this logic inside health_reporter.go.
| // It is a no-op when the KMSEncryption feature gate is not enabled or the encryption-config secret does not exist. | ||
| // The secretClient should be uncached to avoid injecting sidecars based on a stale encryption configuration. | ||
| func AddKMSPluginSidecarToStaticPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess) error { | ||
| func AddKMSPluginSidecarToStaticPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess, operatorImage string) error { |
There was a problem hiding this comment.
Nit: there are more than 4 args, worth considering functional arguments or a config instead. I know it is idiomatic to have plenty of args in Kubernetes.
There was a problem hiding this comment.
It makes sense. But I'd prefer keeping this PR only focusing on health reporter changes without refactoring the other bits.
| corev1.ResourceCPU: resource.MustParse("10m"), | ||
| }, | ||
| }, | ||
| SecurityContext: &corev1.SecurityContext{ |
There was a problem hiding this comment.
I reviewed a PR from Fabio, where this was being set. Is this the same? Does it make sense to have one specific one that can be referenced or called?
There was a problem hiding this comment.
They are identical. But is there a guarantee that kms plugins and health reporter use the same security context?. I think, they should be configured separately?
| encryptionConfigSecretName string | ||
| staticPod bool | ||
|
|
||
| enableHealthReporter bool |
There was a problem hiding this comment.
Is there a scenario, where we don't want to enable the health reporter?
There was a problem hiding this comment.
yes, preflight checker won't use health reporter.
There was a problem hiding this comment.
Now that you say it, it is obvious. And we won't need a comment, because sidecar.go will contain a function for the preflight scenario, right?
There was a problem hiding this comment.
That makes sense. I've updated with documentation.
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
870ba88 to
f5987de
Compare
|
/retitle CNTRLPLANE-3363: Inject health reporter as sidecar container |
openshift-ci
Bot
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
f5987de to
0d22375
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/operator/encryption/kms/pluginlifecycle/health_reporter_test.go (2)
29-34: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider extracting the init-container search to a helper.
The loop that searches
InitContainersfor a container by name is duplicated in lines 29-34 and lines 68-73. Extracting this to a helper function would reduce duplication:func findInitContainer(podSpec *corev1.PodSpec, name string) *corev1.Container { for i := range podSpec.InitContainers { if podSpec.InitContainers[i].Name == name { return &podSpec.InitContainers[i] } } return nil }That said, the duplication is minimal and each test remains self-contained, so this is a minor style preference.
Also applies to: 68-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/kms/pluginlifecycle/health_reporter_test.go` around lines 29 - 34, Extract the duplicated init-container search logic into a helper function. Create a new function called findInitContainer that takes a *corev1.PodSpec and a name string as parameters and returns a *corev1.Container by iterating through InitContainers to find a match. Replace both occurrences of the loop (the one searching for "kms-health-reporter" and any other similar loops at lines 68-73) with calls to this new helper function to eliminate the code duplication.
94-104: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider testing empty
operatorCmdandcontainerNamefor completeness.The error validation in
applyHealthReporterchecks that all three parameters (image, command, container name) are non-empty. The current test only exercises the empty image case. For thorough coverage, you could add subtests for emptyoperatorCmdand emptycontainerNameas well.Since the validation logic is identical for all three parameters, the current test does confirm the error path works correctly, so this is purely a completeness suggestion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/operator/encryption/kms/pluginlifecycle/health_reporter_test.go` around lines 94 - 104, Add two additional subtests to provide complete coverage of the validation logic in applyHealthReporter. Create a subtest for the "empty operatorCmd: returns error" case by calling WithHealthReporter with an empty second parameter (command) while keeping image and containerName populated, and verify it returns an error containing "command" or similar. Similarly, add another subtest for "empty containerName: returns error" by calling WithHealthReporter with an empty third parameter (containerName) while keeping image and operatorCmd populated, and verify it returns the expected error. Both tests should follow the same pattern as the existing "empty image" test, ensuring all three validation checks in applyHealthReporter are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/operator/encryption/kms/pluginlifecycle/health_reporter_test.go`:
- Around line 29-34: Extract the duplicated init-container search logic into a
helper function. Create a new function called findInitContainer that takes a
*corev1.PodSpec and a name string as parameters and returns a *corev1.Container
by iterating through InitContainers to find a match. Replace both occurrences of
the loop (the one searching for "kms-health-reporter" and any other similar
loops at lines 68-73) with calls to this new helper function to eliminate the
code duplication.
- Around line 94-104: Add two additional subtests to provide complete coverage
of the validation logic in applyHealthReporter. Create a subtest for the "empty
operatorCmd: returns error" case by calling WithHealthReporter with an empty
second parameter (command) while keeping image and containerName populated, and
verify it returns an error containing "command" or similar. Similarly, add
another subtest for "empty containerName: returns error" by calling
WithHealthReporter with an empty third parameter (containerName) while keeping
image and operatorCmd populated, and verify it returns the expected error. Both
tests should follow the same pattern as the existing "empty image" test,
ensuring all three validation checks in applyHealthReporter are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 02d9fbd0-8da7-438b-ad24-06f1ea495ec7
📒 Files selected for processing (5)
pkg/operator/encryption/kms/pluginlifecycle/builder.gopkg/operator/encryption/kms/pluginlifecycle/health_reporter.gopkg/operator/encryption/kms/pluginlifecycle/health_reporter_test.gopkg/operator/encryption/kms/pluginlifecycle/sidecar.gopkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- pkg/operator/encryption/kms/pluginlifecycle/sidecar.go
- pkg/operator/encryption/kms/pluginlifecycle/sidecar_test.go
- pkg/operator/encryption/kms/pluginlifecycle/builder.go
- pkg/operator/encryption/kms/pluginlifecycle/health_reporter.go
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
0d22375 to
dc77060
Compare
|
Unrelated |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, ibihim The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| if b.staticPod { | ||
| resourceDirMount := corev1.VolumeMount{Name: resourceDirVolumeName, MountPath: resourcesDir, ReadOnly: true} | ||
| if err := ensureVolumeMountInContainer(podSpec.InitContainers, reporter.Name(), resourceDirMount); err != nil { |
There was a problem hiding this comment.
I find it very elegant that you reused ensureSidecarContaienr, ensureVolumeMountInContainer
| enableHealthReporter bool | ||
| healthReporterContainerName string | ||
| healthReporterOperatorCmd string | ||
| healthReporterImage string |
There was a problem hiding this comment.
would it make sense to create the healthReport here instead? Also, if we make it a pointer, we wouldn't need the boolean
There was a problem hiding this comment.
Yes, it makes sense. Updated.
| }, | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
what do we need this env for? Can we add a comment?
There was a problem hiding this comment.
Necessity is documented.
| return corev1.Container{ | ||
| Name: h.name, | ||
| Image: h.image, | ||
| Command: []string{h.operatorCmd, h.name}, |
There was a problem hiding this comment.
do you need to specify Command because KASO doesn't have an entrypoint?
in your KASO proof PR I see:
if err := kmspluginlifecycle.AddKMSPluginSidecarToStaticPodSpec(ctx, &required.Spec, "kube-apiserver", operatorclient.TargetNamespace, "encryption-config", secretClient, featureGateAccessor, "cluster-kube-apiserver-operator", operatorImagePullSpec); err != nil {
so I think Command here will be cluster-kube-apiserver-operator kms-health-reporter. This makes me wonder if we need to make sure the PATH is correctly set or if we need to use the whole path (/usr/bin/cluster-kube-apiserver-operator).
There was a problem hiding this comment.
I've updated the code to also support empty commands, so that entrpoint will be automatically used (if command is passed as empty). But it is caller's responsibility to verify the presence of the binary IMO.
| } | ||
|
|
||
| func (h *healthReporter) Name() string { | ||
| return h.name |
There was a problem hiding this comment.
it seems like h.name is both the container name (for which it's correctly used in this method), and also the operator cli command. It'd be nice to make that distinction visible somehow
There was a problem hiding this comment.
I agree that it is confusing. I've updated by using subcommand (which refers to the command name in health/cmd.go). Please let me know your thoughts.
| } | ||
| if h.kubeconfig != "" { | ||
| args = append(args, fmt.Sprintf("--kubeconfig=%s", h.kubeconfig)) | ||
| } |
There was a problem hiding this comment.
isn't the kubeconfig required for all health reporters?
There was a problem hiding this comment.
cluster-kube-apiserver-operator requires kubeconfig but others won't need it (they will use in-cluster kubeconfig)
|
|
||
| func (h *healthReporter) BuildSidecarContainer() (corev1.Container, error) { | ||
| args := []string{ | ||
| fmt.Sprintf("--kms-sockets=%s", strings.Join(h.sockets, ",")), |
There was a problem hiding this comment.
do we need to validate for an empty slice?
There was a problem hiding this comment.
Yes, it is better to validate it. Updated.
| // It is a no-op when the KMSEncryption feature gate is not enabled or the encryption-config secret does not exist. | ||
| // The secretClient should be uncached to avoid injecting sidecars based on a stale encryption configuration. | ||
| func AddKMSPluginSidecarToPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess) error { | ||
| func AddKMSPluginSidecarToPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess, operatorCmd string, operatorImage string) error { |
There was a problem hiding this comment.
interesting, I see that the authentication operator has the entreypoint correctly set to /usr/bin/authentication-operator, so we wouldn't need operatorCmd here, right?
There was a problem hiding this comment.
I agree, if we pass operatorCmd empty (to fallback to entrypoint), mechanism should work. So I've updated the code to handle that case.
| AllowPrivilegeEscalation: ptr.To(false), | ||
| Capabilities: &corev1.Capabilities{Drop: []corev1.Capability{"ALL"}}, | ||
| SeccompProfile: &corev1.SeccompProfile{Type: corev1.SeccompProfileTypeRuntimeDefault}, | ||
| }, |
There was a problem hiding this comment.
can we be more strict with the health reporter?
There was a problem hiding this comment.
There is nothing left apart from RunAsRoot: false?. KAS-o needs to run as root (uid: 0)
| // It is a no-op when the KMSEncryption feature gate is not enabled or the encryption-config secret does not exist. | ||
| // The secretClient should be uncached to avoid injecting sidecars based on a stale encryption configuration. | ||
| func AddKMSPluginSidecarToPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess) error { | ||
| func AddKMSPluginSidecarToPodSpec(ctx context.Context, podSpec *corev1.PodSpec, containerName string, encryptionConfigNamespace string, encryptionConfigSecretName string, secretClient corev1client.SecretsGetter, featureGateAccessor featuregates.FeatureGateAccess, operatorCmd string, operatorImage string) error { |
There was a problem hiding this comment.
nit: the params ordering seems a bit off. IMO the feature gate acessor fits better at the end. What about something like this?
func AddKMSPluginSidecarToStaticPodSpec(
ctx context.Context,
podSpec *corev1.PodSpec,
containerName string,
encryptionConfigNamespace string,
encryptionConfigSecretName string,
operatorCmd string,
operatorImage string,
secretClient corev1client.SecretsGetter, // this is not necessarily tied to the encryptionConfig*
featureGateAccessor featuregates.FeatureGateAccess,
) error
ardaguclu
force-pushed
the
inject-health-reporter
branch
from
dc77060 to
a74f1ee
Compare
|
New changes are detected. LGTM label has been removed. |
|
@ardaguclu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
4 participants
This PR injects health reporter container into apiserver pods, if any kms plugin container is added.
Summary by CodeRabbit
New Features
Tests