Skip to content

CNF-25115: Include supported values in validation error messages #440

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sebrandon1 wants to merge 1 commit into
base: master
Choose a base branch
from
+270 −10
Open

CNF-25115: Include supported values in validation error messages #440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions pkg/controller/certmanager/deployment_overrides_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package certmanager
import (
"fmt"
"strconv"
"strings"
"unsafe"

appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -94,7 +95,7 @@ func withContainerArgsValidateHook(certmanagerinformer certmanagerinformer.CertM
validateArgs := func(argMap map[string]string, supportedArgs []string) error {
for k, v := range argMap {
if !slices.Contains(supportedArgs, k) {
return fmt.Errorf("validation failed due to unsupported arg %q=%q", k, v)
return fmt.Errorf("validation failed due to unsupported arg %q=%q; supported args are: %s", k, v, strings.Join(supportedArgs, ", "))
}
}
return nil
Expand Down Expand Up @@ -189,7 +190,7 @@ func withContainerEnvValidateHook(certmanagerinformer certmanagerinformer.CertMa
validateEnv := func(argMap map[string]corev1.EnvVar, supportedEnv []string) error {
for k, v := range argMap {
if !slices.Contains(supportedEnv, k) {
return fmt.Errorf("validation failed due to unsupported arg %q=%q", k, v)
return fmt.Errorf("validation failed due to unsupported env var %q=%q; supported env vars are: %s", k, v, strings.Join(supportedEnv, ", "))
}
}
return nil
Expand Down Expand Up @@ -238,7 +239,7 @@ func withPodLabelsValidateHook(certmanagerinformer certmanagerinformer.CertManag
validateLabels := func(labels map[string]string, supportedLabelKeys []string) error {
for k, v := range labels {
if !slices.Contains(supportedLabelKeys, k) {
return fmt.Errorf("validation failed due to unsupported label %q=%q", k, v)
return fmt.Errorf("validation failed due to unsupported label %q=%q; supported labels are: %s", k, v, strings.Join(supportedLabelKeys, ", "))
}
}
return nil
Expand Down Expand Up @@ -316,12 +317,12 @@ func validateResources(resources v1alpha1.CertManagerResourceRequirements, suppo
errs := []error{}
for k, v := range resources.Limits {
if !slices.Contains(supportedResourceNames, string(k)) {
errs = append(errs, fmt.Errorf("validation failed due to unsupported resource limits %q=%s", k, v.String()))
errs = append(errs, fmt.Errorf("validation failed due to unsupported resource limits %q=%s; supported resources are: %s", k, v.String(), strings.Join(supportedResourceNames, ", ")))
}
}
for k, v := range resources.Requests {
if !slices.Contains(supportedResourceNames, string(k)) {
errs = append(errs, fmt.Errorf("validation failed due to unsupported resource requests %q=%s", k, v.String()))
errs = append(errs, fmt.Errorf("validation failed due to unsupported resource requests %q=%s; supported resources are: %s", k, v.String(), strings.Join(supportedResourceNames, ", ")))
}
}
return utilerrors.NewAggregate(errs)
Expand Down
269 changes: 264 additions & 5 deletions pkg/controller/certmanager/deployment_overrides_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,267 @@ import (
"github.com/openshift/cert-manager-operator/api/operator/v1alpha1"
)

func TestWithContainerEnvValidateHook(t *testing.T) {
tests := []struct {
name string
certManagerObj v1alpha1.CertManager
deploymentName string
wantErrMsg string
omitCertManager bool
}{
{
name: "lister returns error when certmanager cluster is absent",
deploymentName: certmanagerControllerDeployment,
omitCertManager: true,
},
{
name: "controller accepts supported env var HTTP_PROXY",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
ControllerConfig: &v1alpha1.DeploymentConfig{
OverrideEnv: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy:8080"},
},
},
},
},
deploymentName: certmanagerControllerDeployment,
},
{
name: "controller accepts all supported env vars",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
ControllerConfig: &v1alpha1.DeploymentConfig{
OverrideEnv: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy:8080"},
{Name: "HTTPS_PROXY", Value: "https://proxy:8443"},
{Name: "NO_PROXY", Value: "localhost"},
},
},
},
},
deploymentName: certmanagerControllerDeployment,
},
{
name: "controller rejects unsupported env var with supported list",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
ControllerConfig: &v1alpha1.DeploymentConfig{
OverrideEnv: []corev1.EnvVar{
{Name: "UNKNOWN_VAR", Value: "foo"},
},
},
},
},
deploymentName: certmanagerControllerDeployment,
wantErrMsg: "unsupported env var",

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Assert the supported environment-variable lists.

These cases only check "unsupported env var", so removing supported env vars are: ... would not fail the tests. Assert the full supported-list suffix for controller and the empty-list behavior for webhook/cainjector.

Also applies to: 90-90, 105-105

🤖 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/controller/certmanager/deployment_overrides_validation_test.go` at line
75, Update the relevant test cases in the deployment override validation tests
to assert the complete error message: include the supported-environment-variable
list suffix for the controller and assert the expected empty-list formatting for
webhook and cainjector. Replace the partial “unsupported env var” expectations
in each affected wantErrMsg entry while preserving the existing validation
scenarios.

},
{
name: "webhook rejects any env var (empty supported list)",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
WebhookConfig: &v1alpha1.DeploymentConfig{
OverrideEnv: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy:8080"},
},
},
},
},
deploymentName: certmanagerWebhookDeployment,
wantErrMsg: "unsupported env var",
},
{
name: "cainjector rejects any env var (empty supported list)",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
CAInjectorConfig: &v1alpha1.DeploymentConfig{
OverrideEnv: []corev1.EnvVar{
{Name: "HTTP_PROXY", Value: "http://proxy:8080"},
},
},
},
},
deploymentName: certmanagerCAinjectorDeployment,
wantErrMsg: "unsupported env var",
},
{
name: "nil controller config skips validation",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{},
},
deploymentName: certmanagerControllerDeployment,
},
{
name: "unsupported deployment name returns error",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{},
},
deploymentName: "unknown-deployment",
wantErrMsg: `unsupported deployment name "unknown-deployment" provided`,
},
}

ctx := t.Context()
fakeClient, certManagerInformers, certManagerChan := setupSyncedFakeCertManagerInformer(t, ctx)

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if tc.omitCertManager {
hook := withContainerEnvValidateHook(certManagerInformers, tc.deploymentName)
hookErr := hook(nil, &appsv1.Deployment{})
require.Error(t, hookErr)
assert.Contains(t, hookErr.Error(), `failed to get certmanager "cluster"`)
return
}

withFakeCertManagerForTest(t, ctx, fakeClient, certManagerChan, &tc.certManagerObj)

hook := withContainerEnvValidateHook(certManagerInformers, tc.deploymentName)
hookErr := hook(nil, &appsv1.Deployment{})

if tc.wantErrMsg != "" {
require.Error(t, hookErr)
assert.Contains(t, hookErr.Error(), tc.wantErrMsg)
} else {
require.NoError(t, hookErr)
}
})
}
}

func TestWithPodLabelsValidateHook(t *testing.T) {
tests := []struct {
name string
certManagerObj v1alpha1.CertManager
deploymentName string
wantErrMsg string
omitCertManager bool
}{
{
name: "lister returns error when certmanager cluster is absent",
deploymentName: certmanagerControllerDeployment,
omitCertManager: true,
},
{
name: "controller accepts supported label",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
ControllerConfig: &v1alpha1.DeploymentConfig{
OverrideLabels: map[string]string{
"azure.workload.identity/use": "true",
},
},
},
},
deploymentName: certmanagerControllerDeployment,
},
{
name: "controller rejects unsupported label with supported list",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
ControllerConfig: &v1alpha1.DeploymentConfig{
OverrideLabels: map[string]string{
"unknown-label": "value",
},
},
},
},
deploymentName: certmanagerControllerDeployment,
wantErrMsg: "unsupported label",
Comment thread
coderabbitai[bot] marked this conversation as resolved.
},
{
name: "webhook rejects any label (empty supported list)",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
WebhookConfig: &v1alpha1.DeploymentConfig{
OverrideLabels: map[string]string{
"some-label": "value",
},
},
},
},
deploymentName: certmanagerWebhookDeployment,
wantErrMsg: "unsupported label",
},
{
name: "cainjector rejects any label (empty supported list)",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{
CAInjectorConfig: &v1alpha1.DeploymentConfig{
OverrideLabels: map[string]string{
"some-label": "value",
},
},
},
},
deploymentName: certmanagerCAinjectorDeployment,
wantErrMsg: "unsupported label",
},
{
name: "nil controller config skips validation",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{},
},
deploymentName: certmanagerControllerDeployment,
},
{
name: "unsupported deployment name returns error",
certManagerObj: v1alpha1.CertManager{
ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
Spec: v1alpha1.CertManagerSpec{},
},
deploymentName: "unknown-deployment",
wantErrMsg: `unsupported deployment name "unknown-deployment" provided`,
},
}

ctx := t.Context()
fakeClient, certManagerInformers, certManagerChan := setupSyncedFakeCertManagerInformer(t, ctx)

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
if tc.omitCertManager {
hook := withPodLabelsValidateHook(certManagerInformers, tc.deploymentName)
hookErr := hook(nil, &appsv1.Deployment{})
require.Error(t, hookErr)
assert.Contains(t, hookErr.Error(), `failed to get certmanager "cluster"`)
return
}

withFakeCertManagerForTest(t, ctx, fakeClient, certManagerChan, &tc.certManagerObj)

hook := withPodLabelsValidateHook(certManagerInformers, tc.deploymentName)
hookErr := hook(nil, &appsv1.Deployment{})

if tc.wantErrMsg != "" {
require.Error(t, hookErr)
assert.Contains(t, hookErr.Error(), tc.wantErrMsg)
} else {
require.NoError(t, hookErr)
}
})
}
}

func TestValidateResources(t *testing.T) {
tests := []struct {
name string
resources v1alpha1.CertManagerResourceRequirements
resourceNamesSupported []string
errorExpected bool
wantErrContains string
}{
{
name: "validate cpu resource name in resources limits",
Expand Down Expand Up @@ -101,7 +356,7 @@ func TestValidateResources(t *testing.T) {
errorExpected: false,
},
{
name: "unsupported resource name in resources limits and requests should return error",
name: "unsupported resource name in resources limits and requests should return error with supported list",
resources: v1alpha1.CertManagerResourceRequirements{
Limits: corev1.ResourceList{
corev1.ResourceEphemeralStorage: k8sresource.MustParse("10Gi"),
Expand All @@ -112,6 +367,7 @@ func TestValidateResources(t *testing.T) {
},
resourceNamesSupported: []string{string(corev1.ResourceCPU), string(corev1.ResourceMemory)},
errorExpected: true,
wantErrContains: "supported resources are: cpu, memory",
},
}

Expand All @@ -120,6 +376,9 @@ func TestValidateResources(t *testing.T) {
err := validateResources(tc.resources, tc.resourceNamesSupported)
if tc.errorExpected {
assert.Error(t, err)
if tc.wantErrContains != "" {
assert.Contains(t, err.Error(), tc.wantErrContains)
}
} else {
assert.NoError(t, err)
}
Expand Down Expand Up @@ -309,7 +568,7 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
},
},
deploymentName: certmanagerControllerDeployment,
wantErrMsg: `validation failed due to unsupported arg "--totally-unknown-flag"="value"`,
wantErrMsg: `validation failed due to unsupported arg "--totally-unknown-flag"="value"; supported args are: --acme-http01-solver-nameservers, --acme-http01-solver-resource-limits-cpu, --acme-http01-solver-resource-limits-memory, --acme-http01-solver-resource-request-cpu, --acme-http01-solver-resource-request-memory, --dns01-recursive-nameservers, --dns01-recursive-nameservers-only, --v, -V, --metrics-listen-address, --issuer-ambient-credentials, --enable-certificate-owner-ref, --certificate-request-minimum-backoff-duration, --concurrent-workers, --kube-api-qps, --kube-api-burst, --max-concurrent-challenges`,
},
{
name: "controller accepts performance tuning flags",
Expand Down Expand Up @@ -546,7 +805,7 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
},
},
deploymentName: certmanagerWebhookDeployment,
wantErrMsg: `validation failed due to unsupported arg "--metrics-listen-address"="0.0.0.0:9402"`,
wantErrMsg: `validation failed due to unsupported arg "--metrics-listen-address"="0.0.0.0:9402"; supported args are: --v, -V`,
},
{
name: "webhook rejects certificate-request-minimum-backoff-duration",
Expand All @@ -559,7 +818,7 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
},
},
deploymentName: certmanagerWebhookDeployment,
wantErrMsg: `validation failed due to unsupported arg "--certificate-request-minimum-backoff-duration"="1m"`,
wantErrMsg: `validation failed due to unsupported arg "--certificate-request-minimum-backoff-duration"="1m"; supported args are: --v, -V`,
},
{
name: "nil webhook config skips validation",
Expand Down Expand Up @@ -607,7 +866,7 @@ func TestWithContainerArgsValidateHook(t *testing.T) {
},
},
deploymentName: certmanagerCAinjectorDeployment,
wantErrMsg: `validation failed due to unsupported arg "--dns01-recursive-nameservers"="8.8.8.8:53"`,
wantErrMsg: `validation failed due to unsupported arg "--dns01-recursive-nameservers"="8.8.8.8:53"; supported args are: --v, -V`,
},
{
name: "nil cainjector config skips validation",
Expand Down