feat(STONEINTG-1661): validate Component for NudgeConfig nudges#1607
feat(STONEINTG-1661): validate Component for NudgeConfig nudges#1607Josh-Everett wants to merge 1 commit into
Conversation
Josh-Everett
requested review from
14rcole,
NigelByrne1,
dirgim,
jencull,
jsztuka,
kasemAlem and
sonam1412
as code owners
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation logic to ensure that components referenced in a NudgeConfig actually exist in the namespace during creation and update operations, along with comprehensive unit tests. Feedback points out that the added webhook 'vnudgeconfig.kb.io' in 'dist/chart/templates/webhook/webhooks.yaml' is a duplicate of an existing definition, which is invalid in Kubernetes and should be consolidated.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - name: vnudgeconfig.kb.io | ||
| clientConfig: | ||
| service: | ||
| name: integration-service-webhook-service | ||
| namespace: {{ .Values.namespace | default .Release.Namespace }} | ||
| path: /validate-appstudio-redhat-com-v1beta2-nudgeconfig | ||
| failurePolicy: Fail | ||
| sideEffects: None | ||
| admissionReviewVersions: | ||
| - v1 | ||
| namespaceSelector: | ||
| matchExpressions: | ||
| - key: kubernetes.io/metadata.name | ||
| operator: NotIn | ||
| values: [kube-system, kube-public, kube-node-lease] | ||
| rules: | ||
| - operations: | ||
| - CREATE | ||
| - UPDATE | ||
| apiGroups: | ||
| - appstudio.redhat.com | ||
| apiVersions: | ||
| - v1beta2 | ||
| resources: | ||
| - nudgeconfigs |
There was a problem hiding this comment.
The webhook vnudgeconfig.kb.io is already defined at lines 50-69 in this file. Defining two webhooks with the same name in a single ValidatingWebhookConfiguration is invalid in Kubernetes and will cause deployment failures or one definition to silently overwrite the other.
Please remove this duplicate entry and instead update the existing webhook definition at lines 50-69 to include the namespaceSelector block.
|
🤖 Finished Review · ✅ Success · Started 2:37 PM UTC · Completed 2:51 PM UTC |
PR Summary by QodoValidate NudgeConfig nudge Component references via admission webhook Description
Diagram
High-Level Assessment
Files changed (5)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1607 +/- ##
===========================================
- Coverage 67.00% 39.67% -27.34%
===========================================
Files 71 70 -1
Lines 9533 9231 -302
===========================================
- Hits 6388 3662 -2726
- Misses 2416 5011 +2595
+ Partials 729 558 -171
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 65 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
10 rules 1. Duplicate chart webhook
|
![qodo-app-for-konflux-ci[bot]](https://avatars.githubusercontent.com/u/155583288?s=60&v=4){kind=small}
| componentList := &applicationapiv1alpha1.ComponentList{} | ||
| if err := v.Client.List(ctx, componentList, client.InNamespace(namespace)); err != nil { | ||
| return fmt.Errorf("failed to list Components in namespace %q: %w", namespace, err) | ||
| } |
There was a problem hiding this comment.
1. Direct v.client.list used 📘 Rule violation ⌂ Architecture
The webhook validator fetches Component resources via the raw controller-runtime client (v.Client.List) instead of going through loader.ObjectLoader. This bypasses the required fetching abstraction and violates the repository’s Kubernetes access policy.
Agent Prompt
## Issue description
New code fetches Kubernetes resources using `v.Client.List(...)` directly. Per compliance, all Kubernetes resource fetching must go through `loader.ObjectLoader` (or an injected abstraction that wraps it).
## Issue Context
This was introduced in the new `validateComponentsExist(...)` logic for `NudgeConfig` validation.
## Fix Focus Areas
- internal/webhook/v1beta2/nudgeconfig_webhook.go[121-134]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - name: vnudgeconfig.kb.io | ||
| clientConfig: | ||
| service: | ||
| name: integration-service-webhook-service | ||
| namespace: {{ .Values.namespace | default .Release.Namespace }} | ||
| path: /validate-appstudio-redhat-com-v1beta2-nudgeconfig | ||
| failurePolicy: Fail | ||
| sideEffects: None | ||
| admissionReviewVersions: | ||
| - v1 | ||
| namespaceSelector: | ||
| matchExpressions: | ||
| - key: kubernetes.io/metadata.name | ||
| operator: NotIn | ||
| values: [kube-system, kube-public, kube-node-lease] | ||
| rules: | ||
| - operations: | ||
| - CREATE | ||
| - UPDATE | ||
| apiGroups: | ||
| - appstudio.redhat.com | ||
| apiVersions: | ||
| - v1beta2 | ||
| resources: | ||
| - nudgeconfigs |
There was a problem hiding this comment.
3. Duplicate chart webhook 🐞 Bug ≡ Correctness
dist/chart/templates/webhook/webhooks.yaml defines vnudgeconfig.kb.io twice in the same ValidatingWebhookConfiguration, causing redundant admission calls and leaving the earlier entry unscoped (so system namespaces are still matched). This can increase admission latency and defeats the intended namespaceSelector restriction.
Agent Prompt
### Issue description
The Helm chart template renders two `webhooks` entries with `name: vnudgeconfig.kb.io`. The earlier one has no `namespaceSelector`, so the webhook still applies to system namespaces and also results in duplicate webhook invocations for normal namespaces.
### Issue Context
There is already an existing `vnudgeconfig.kb.io` entry earlier in the same `webhooks:` list. The PR added a second one with a `namespaceSelector` instead of updating the original.
### Fix Focus Areas
- dist/chart/templates/webhook/webhooks.yaml[49-156]
### What to change
- Remove the newly-added duplicate `vnudgeconfig.kb.io` block.
- Add the `namespaceSelector` stanza to the original `vnudgeconfig.kb.io` webhook entry (the one already present near the top of the validating webhook list).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| patches: | ||
| - path: patches/nudgeconfig_namespace_selector.yaml |
There was a problem hiding this comment.
4. Missing kustomize patch 🐞 Bug ☼ Reliability
config/webhook/kustomization.yaml references patches/nudgeconfig_namespace_selector.yaml, but that file is not present in the PR branch, so kustomize build config/webhook (and config/default) fails. This breaks deployment pipelines that use the kustomize manifests.
Agent Prompt
### Issue description
`config/webhook/kustomization.yaml` includes a patch file path that does not exist in the repository. This causes kustomize to error out when building the webhook manifests.
### Issue Context
The PR adds:
- `patches: - path: patches/nudgeconfig_namespace_selector.yaml`
…but there is no `config/webhook/patches/nudgeconfig_namespace_selector.yaml` in the branch.
### Fix Focus Areas
- config/webhook/kustomization.yaml[1-9]
### What to change
- Create the missing file at `config/webhook/patches/nudgeconfig_namespace_selector.yaml` (and the `config/webhook/patches/` directory).
- The patch should add the intended `namespaceSelector` to the `ValidatingWebhookConfiguration` webhook with `name: vnudgeconfig.kb.io` (metadata.name is `validating-webhook-configuration` in `config/webhook/manifests.yaml`).
Example (strategic-merge) patch content:
```yaml
apiVersion: admissionregistration.k8s.io/v1
kind: ValidatingWebhookConfiguration
metadata:
name: validating-webhook-configuration
webhooks:
- name: vnudgeconfig.kb.io
namespaceSelector:
matchExpressions:
- key: kubernetes.io/metadata.name
operator: NotIn
values:
- kube-system
- kube-public
- kube-node-lease
```
(Alternatively: remove the `patches:` entry if you decide not to apply a namespaceSelector for the kustomize deployment path.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
ReviewFindingsCritical
High
Medium
Low
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
🤖 Review · |
Add a Go ValidatingWebhook that rejects NudgeConfig CREATE/UPDATE when spec.nudges[].from or spec.nudges[].to references a Component that does not exist in the namespace. Pre-existing entries are not re-validated on update (staleness is the stale-reference controller's responsibility). Signed-off-by: Josh Everett <jeverett@redhat.com> Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🤖 Finished Review · ❌ Failure · Started 6:54 PM UTC · Completed 7:00 PM UTC |
| if len(missingFrom) > 0 { | ||
| msg += fmt.Sprintf("; missing 'from' component(s): %s", strings.Join(missingFrom, ", ")) | ||
| } | ||
| if len(missingTo) > 0 { |
There was a problem hiding this comment.
nit: This logs the full rejection message at Info level, but the admission framework also logs returned errors on rejection — so every rejected NudgeConfig produces two log entries with near-identical
content. Consider either removing this explicit log or dropping it to debug level (nudgeconfiglog.V(1).Info(msg)).
| msg += fmt.Sprintf("; missing 'from' component(s): %s", strings.Join(missingFrom, ", ")) | ||
| } | ||
| if len(missingTo) > 0 { | ||
| msg += fmt.Sprintf("; missing 'to' component(s): %s", strings.Join(missingTo, ", ")) |
There was a problem hiding this comment.
nit: No format substitution here —errors.New(msg)is slightly more idiomatic than fmt.Errorf("%s", msg)
|
in general PR is good, beside two minor nits and fixes proposed by AI agents, make sure comments not hallucinations :) Great work @Josh-Everett |
|
@Josh-Everett just FYI there is a flaky test that fails about 40% of the time when run on local. Ticket created to address it, 1694. https://github.com/konflux-ci/integration-service/actions/runs/28600938718/job/84808668695 <- my PR has same issue. |
4 participants
Add a Go ValidatingWebhook that rejects NudgeConfig CREATE/UPDATE when spec.nudges[].from or spec.nudges[].to references a Component that does not exist in the namespace. Pre-existing entries are not re-validated on update (staleness is the stale-reference controller's responsibility).