Migrating user_certs_test to ote#1993
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
WalkthroughRefactors test helpers to accept testing.TB, adds a new end-to-end test that verifies API server named TLS certificates, updates TestNamedCertificates to call a shared helper, and removes an import and a wait-for-stabilization call from the service-account-issuer test. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
Comment |
openshift-ci
Bot
added
the
approved
gangwgr
force-pushed
the
user_certs_test
branch
2 times, most recently
from
d5eda9f to
68f850b
Compare
|
/test e2e-aws-ovn-serial-1of2 |
gangwgr
force-pushed
the
user_certs_test
branch
from
20d83fb to
92e3b59
Compare
|
/test e2e-gcp-operator-serial-ote |
gangwgr
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/test e2e-gcp-operator-serial-ote |
7 similar comments
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
|
/test e2e-gcp-operator-serial-ote |
gangwgr
force-pushed
the
user_certs_test
branch
4 times, most recently
from
aef3a35 to
5a1225b
Compare
|
Update a Summary for PR, you can see my PR openshift/service-ca-operator#305, better let me know, such as: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, wangke19 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 |
| err = pollForOperandIssuer(t, kubeClient, []string{"https://kubernetes.default.svc"}) | ||
| require.NoError(t, err, "pollForOperandIssuer failed") | ||
| // Wait for API server to stabilize after configuration change | ||
| testlibraryapi.WaitForAPIServerToStabilizeOnTheSameRevision(t, kubeClient.Pods(operatorclient.TargetNamespace)) |
There was a problem hiding this comment.
| ) | ||
|
|
||
| var _ = g.Describe("[sig-api-machinery] kube-apiserver operator", func() { | ||
| g.It("[Operator][Serial][Timeout:40m] TestNamedCertificates", func() { |
There was a problem hiding this comment.
should TestNamedCertificates be lower case ? xref: https://github.com/gangwgr/cluster-kube-apiserver-operator/blob/5a1225b4a0f5ce82f4efe748ba4414a560e8b3cf/test/e2e/serviceaccountissuer.go#L29
There was a problem hiding this comment.
I keep the same name as gotest
| ) | ||
|
|
||
| var _ = g.Describe("[sig-api-machinery] kube-apiserver operator", func() { | ||
| g.It("[Operator][Serial][Timeout:40m] TestNamedCertificates", func() { |
There was a problem hiding this comment.
I think that this (It) should also describe the purpose
| for _, tc := range testCases { | ||
| tc := tc // capture range variable | ||
|
|
||
| // Use t.Run if available (*testing.T), otherwise run directly (Ginkgo's testing.TB) |
There was a problem hiding this comment.
this is interesting.
normally we would:
setup the test
then run cases as subtest
mhm, should we do that ?
what is the benefit of having subtests vs one single test ?
|
/hold @wangke19 i'm still reviewing the PR :) |
openshift-ci
Bot
added
the
do-not-merge/hold
gangwgr
force-pushed
the
user_certs_test
branch
from
5a1225b to
35b4311
Compare
|
New changes are detected. LGTM label has been removed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/e2e/user_certs.go`:
- Around line 228-245: The poll callback passed to wait.PollImmediate must not
call require.NoError (which calls t.FailNow); in both branches replace the
require.NoError inside the callback with returning (false, err) when
getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) fails, and
compare/return the boolean only otherwise; then after wait.PollImmediate
completes, call require.NoError(t, err, "test case %s failed", tc.name) (or
require.NoError(t, err) in the non-Ginkgo path) to fail the test with the poll
error—update the callback in both the non-Ginkgo and Ginkgo branches referencing
getReturnedCertSerialNumber and wait.PollImmediate accordingly.
🧹 Nitpick comments (2)
test/e2e/user_certs.go (2)
261-265: Consider addingMinVersionto TLS config for defense-in-depth.The static analyzer flagged the missing
MinVersion. While this is test code connecting to a trusted internal API server withInsecureSkipVerify: true(appropriate for certificate inspection), addingMinVersion: tls.VersionTLS12would be a defense-in-depth measure.♻️ Optional improvement
tlsConf := &tls.Config{ VerifyPeerCertificate: verifyPeerCertificate, ServerName: serverName, InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, }
317-317: Usecorev1.TLSCertKeyconstant for consistency.The hardcoded
"tls.crt"string works but is inconsistent withcreateTLSSecretwhich uses thecorev1.TLSCertKeyconstant on line 301.♻️ Optional improvement
- certificates, err := cert.ParseCertsPEM(secret.Data["tls.crt"]) + certificates, err := cert.ParseCertsPEM(secret.Data[corev1.TLSCertKey])
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
test/e2e/serviceaccountissuer.gotest/e2e/user_certs.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
test/e2e/user_certs.gotest/e2e/serviceaccountissuer.go
🪛 ast-grep (0.40.5)
test/e2e/user_certs.go
[warning] 260-264: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
VerifyPeerCertificate: verifyPeerCertificate,
ServerName: serverName,
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (5)
test/e2e/serviceaccountissuer.go (1)
29-29: Verify the timeout removal is intentional.The
[Timeout:30m]tag was removed from the test description. Ensure this is intentional and that the test framework or CI configuration provides adequate timeout handling to prevent indefinitely hanging tests.test/e2e/user_certs.go (4)
34-38: LGTM!The Ginkgo wrapper is well-structured with a descriptive test name and appropriate 40-minute timeout given the expected ~34-minute runtime.
40-113: LGTM!The test setup properly initializes clients, creates test certificates with appropriate cleanup via
defer, and retrieves baseline certificate serial numbers for comparison. Good use ofassert.NoErrorin cleanup defers vsrequire.NoErrorfor critical setup steps.
278-304: LGTM!Secret management helpers are well-implemented with proper error handling and use of standard Kubernetes constants.
353-420: LGTM!API hostname retrieval and test certificate helpers are clean and follow consistent patterns with appropriate error handling and logging.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | ||
| // connect to apiserver using a custom ServerName and examine the returned certificate's | ||
| // serial number to determine if the expected serving certificate was returned. | ||
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | ||
| require.NoError(t, err) | ||
| return tc.expectedSerialNumber == serialNumber, nil | ||
| }) | ||
| require.NoError(t, err) | ||
| }) | ||
| } else { | ||
| // Ginkgo path - no subtests available | ||
| t.Logf("Running test case: %s", tc.name) | ||
| err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | ||
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | ||
| require.NoError(t, err) | ||
| return tc.expectedSerialNumber == serialNumber, nil | ||
| }) | ||
| require.NoError(t, err, "test case %s failed", tc.name) |
There was a problem hiding this comment.
Avoid require.NoError inside poll callbacks.
Using require.NoError inside wait.PollImmediate callbacks is problematic because require calls t.FailNow(), which uses runtime.Goexit(). This can cause unexpected behavior when called from goroutines or callbacks. Instead, return the error from the callback and handle it after the poll completes.
🐛 Proposed fix
err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
// connect to apiserver using a custom ServerName and examine the returned certificate's
// serial number to determine if the expected serving certificate was returned.
serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName)
- require.NoError(t, err)
+ if err != nil {
+ return false, nil // retry on error
+ }
return tc.expectedSerialNumber == serialNumber, nil
})
require.NoError(t, err)Apply the same fix to the Ginkgo path (lines 240-244):
err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) {
serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName)
- require.NoError(t, err)
+ if err != nil {
+ return false, nil // retry on error
+ }
return tc.expectedSerialNumber == serialNumber, nil
})
require.NoError(t, err, "test case %s failed", tc.name)📝 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.
| err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | |
| // connect to apiserver using a custom ServerName and examine the returned certificate's | |
| // serial number to determine if the expected serving certificate was returned. | |
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | |
| require.NoError(t, err) | |
| return tc.expectedSerialNumber == serialNumber, nil | |
| }) | |
| require.NoError(t, err) | |
| }) | |
| } else { | |
| // Ginkgo path - no subtests available | |
| t.Logf("Running test case: %s", tc.name) | |
| err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | |
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | |
| require.NoError(t, err) | |
| return tc.expectedSerialNumber == serialNumber, nil | |
| }) | |
| require.NoError(t, err, "test case %s failed", tc.name) | |
| err := wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | |
| // connect to apiserver using a custom ServerName and examine the returned certificate's | |
| // serial number to determine if the expected serving certificate was returned. | |
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | |
| if err != nil { | |
| return false, nil // retry on error | |
| } | |
| return tc.expectedSerialNumber == serialNumber, nil | |
| }) | |
| require.NoError(t, err) | |
| } else { | |
| // Ginkgo path - no subtests available | |
| t.Logf("Running test case: %s", tc.name) | |
| err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { | |
| serialNumber, err := getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName) | |
| if err != nil { | |
| return false, nil // retry on error | |
| } | |
| return tc.expectedSerialNumber == serialNumber, nil | |
| }) | |
| require.NoError(t, err, "test case %s failed", tc.name) |
🤖 Prompt for AI Agents
In `@test/e2e/user_certs.go` around lines 228 - 245, The poll callback passed to
wait.PollImmediate must not call require.NoError (which calls t.FailNow); in
both branches replace the require.NoError inside the callback with returning
(false, err) when getReturnedCertSerialNumber(kubeConfig.Host, tc.serverName)
fails, and compare/return the boolean only otherwise; then after
wait.PollImmediate completes, call require.NoError(t, err, "test case %s
failed", tc.name) (or require.NoError(t, err) in the non-Ginkgo path) to fail
the test with the poll error—update the callback in both the non-Ginkgo and
Ginkgo branches referencing getReturnedCertSerialNumber and wait.PollImmediate
accordingly.
|
@gangwgr: Closed this PR. 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 kubernetes-sigs/prow repository. |
3 participants
Migrating user_certs_test to ote