tls: deduplicate ConfigMap helpers into standalone functions"#31136
tls: deduplicate ConfigMap helpers into standalone functions"#31136gangwgr wants to merge 4 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds explicit ConfigMap namespace handling to TLS observed-config tests: ChangesTLS Test Infrastructure Refactoring
Sequence Diagram(s)sequenceDiagram
participant Test as TLS Test
participant K8s as Kubernetes API
participant CVO as ClusterVersionOperator
participant APIServer as API Server
Test->>K8s: validateNamespace(namespace)
Test->>K8s: getConfigMap(namespace/name)
K8s-->>Test: ConfigMap (annotations + data)
alt inject-tls present
CVO->>K8s: inject TLS into ConfigMap (inject-tls)
K8s-->>CVO: update ConfigMap
CVO->>APIServer: apply TLS profile
APIServer-->>Test: TLS profile info (queried)
Test->>K8s: re-fetch ConfigMap (poll)
K8s-->>Test: ConfigMap with servingInfo (minTLSVersion, cipherSuites)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr 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 |
openshift-ci
Bot
added
the
approved
|
/pipeline required |
|
Scheduling required tests: |
| configMapNamespace string | ||
| // configMapKey is the key within the ConfigMap that contains the TLS config | ||
| // (typically "config.yaml"). If empty, defaults to "config.yaml". | ||
| // configMapKey is the data key within the ConfigMap. |
There was a problem hiding this comment.
If you take a look at ingvagabund@d710237 there are two other places where configMapKey is empty. These needs to be set too.
|
|
||
| _, found := cm.Annotations[injectTLSAnnotation] | ||
| // requireAnnotation asserts the given annotation is present on the ConfigMap. | ||
| func requireAnnotation(cm *corev1.ConfigMap, annotationKey string) { |
There was a problem hiding this comment.
This commit adds three new helpers. Unfortunately, the code changes overlap each other so it takes some time to focus the eye to see the boundaries and memorize the parts. To be honest I can look at the code change only so many times before my brain stops separating those three functions from each other.
| o.Expect(err).NotTo(o.HaveOccurred(), fmt.Sprintf("unexpected error checking namespace %s", t.configMapNamespace)) | ||
| validateNamespace(oc, ctx, t.configMapNamespace) | ||
|
|
||
| // Get the expected TLS version from the cluster profile. |
There was a problem hiding this comment.
How is removing the comment related to the refactoring?
Remove the fallback that defaulted configMapKey to "config.yaml" and set the key explicitly on the two targets that left it empty.
Remove the fallback that defaulted configMapNamespace to the target's namespace field and set the namespace explicitly on the two targets (image-registry, samples-operator) that relied on it.
Add a reusable validateNamespace function that checks whether a namespace exists (skipping the test if not) and update the five ConfigMap test functions to use it.
Add a reusable getConfigMap function that fetches a ConfigMap from the API server (failing the test on error) and update the five ConfigMap test functions to use it instead of inlining the Get call.
gangwgr
force-pushed
the
tls-dedup-helpers
branch
2 times, most recently
from
583a7b9 to
e5271ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/extended/tls/tls_observed_config.go`:
- Around line 1189-1192: The code currently treats a missing configMap key as an
empty value and skips the test; change the guard to first check key presence
using the map lookup (e.g., val, ok := cm.Data[t.configMapKey]) and if ok is
false, fail the test (or return an error) to surface a bad key/injection,
otherwise inspect val with strings.Contains for "servingInfo" (and similarly for
the other check around minTLSVersion), only skipping when the key exists but the
expected substring is absent.
- Around line 883-886: The current ConfigMap Get call
(oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get using t.configMapNamespace,
t.configMapName, configChangeCtx) silently continues on any error; change it to
only continue when apierrors.IsNotFound(err) is true and treat all other errors
as test failures (e.g., call t.Fatalf or e2e.Failf with the error details) so
typos or API failures don't drop targets; apply the same change to the other
occurrence around lines 1506-1509.
- Around line 1107-1109: The ConfigMap update in updateConfigMap is vulnerable
to transient resourceVersion conflicts; wrap the call to
oc.AdminKubeClient().CoreV1().ConfigMaps(t.configMapNamespace).Update(...)
inside retry.RetryOnConflict so the update is retried on conflicts, using a
closure that re-reads the ConfigMap (via
oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get(...)) if necessary and then
applies the same mutation before retrying; ensure the retry returns the final
error from Update and preserves the existing error message context.
🪄 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: d4b4e02d-0e6c-429c-ae48-f2044e58a793
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
| cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps(t.configMapNamespace).Get(configChangeCtx, t.configMapName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| e2e.Logf("SKIP: ConfigMap %s/%s not found: %v", cmNamespace, t.configMapName, err) | ||
| e2e.Logf("SKIP: ConfigMap %s/%s not found: %v", t.configMapNamespace, t.configMapName, err) | ||
| continue |
There was a problem hiding this comment.
Fail on unexpected ConfigMap lookup errors here.
Both profile-switch paths continue on any Get error. With the new explicit configMapNamespace/configMapName fields, a typo, missing namespace validation, or transient API failure will silently drop that target from coverage and let the suite pass. Only IsNotFound should skip; other errors should still fail.
Suggested tightening
- cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps(t.configMapNamespace).Get(configChangeCtx, t.configMapName, metav1.GetOptions{})
- if err != nil {
- e2e.Logf("SKIP: ConfigMap %s/%s not found: %v", t.configMapNamespace, t.configMapName, err)
- continue
- }
+ validateNamespace(oc, configChangeCtx, t.configMapNamespace)
+ cm, err := oc.AdminKubeClient().CoreV1().ConfigMaps(t.configMapNamespace).Get(configChangeCtx, t.configMapName, metav1.GetOptions{})
+ if apierrors.IsNotFound(err) {
+ e2e.Logf("SKIP: ConfigMap %s/%s not found", t.configMapNamespace, t.configMapName)
+ continue
+ }
+ o.Expect(err).NotTo(o.HaveOccurred(),
+ fmt.Sprintf("failed to get ConfigMap %s/%s", t.configMapNamespace, t.configMapName))Also applies to: 1506-1509
🤖 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 `@test/extended/tls/tls_observed_config.go` around lines 883 - 886, The current
ConfigMap Get call (oc.AdminKubeClient().CoreV1().ConfigMaps(...).Get using
t.configMapNamespace, t.configMapName, configChangeCtx) silently continues on
any error; change it to only continue when apierrors.IsNotFound(err) is true and
treat all other errors as test failures (e.g., call t.Fatalf or e2e.Failf with
the error details) so typos or API failures don't drop targets; apply the same
change to the other occurrence around lines 1506-1509.
|
Scheduling required tests: |
|
@gangwgr: 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. |
|
Job Failure Risk Analysis for sha: e5271ce
|
2 participants
Summary
Follow-up to : no-jira: tls: extract injectTLSAnnotation constant #31125 (merged). Each commit is independently reviewable.
Summary by CodeRabbit