NO-JIRA: Moving auth and oas helper funcs to library-go#2333
Merged
Conversation
|
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 shared dynamic client for encryption test helpers, plus OAuth token and OpenShift Route helpers and assertions that create resources, inspect raw etcd values, and verify encryption state. ChangesAuth & OAS Encryption Test Helpers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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/library/encryption/assertion.go`:
- Around line 232-242: The etcd assertion failure messages in
AssertTokenOfLifeNotEncrypted (and the related token-check helper nearby) are
logging sensitive token material by printing the refresh token marker and raw
payload. Update these tests to keep logs redacted: remove the plain-text token
value and raw etcd content from t.Errorf calls, and replace them with generic
failure text that references the assertion name without exposing token data.
In `@test/library/encryption/helpers.go`:
- Around line 523-530: In the helper that calls clientSet.Etcd.Get and then
indexes resp.Kvs[0], stop execution immediately when the returned key count is
not exactly one by replacing the non-fatal t.Errorf path with a fatal assertion
or equivalent early return before indexing. Also switch the lookup from a
prefix-based Get with routeOfLifeEtcdPrefix to an exact key fetch in the
affected helper(s) so similarly named objects do not collide and resp.Kvs[0] is
only accessed when a single result is guaranteed.
- Around line 536-545: The token pre-cleanup in the helper continues into the
create path even when the existing-token lookup or deletion fails, which can
hide the real setup error. Update the logic around
cs.TokenClient.OAuthAccessTokens().Get and Delete to fail fast instead of only
logging, and return immediately on any unexpected error so the helper never
proceeds to Create with an uncertain state. Keep the fix localized to the
existing cleanup flow in the token helper so the failure is surfaced before
later steps run.
🪄 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: 98c693e0-71d6-458c-9707-533c0617919f
⛔ Files ignored due to path filters (19)
vendor/github.com/openshift/client-go/oauth/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/clusterrolescoperestriction.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthauthorizetoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthclient.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthclientauthorization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/scoperestriction.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/useroauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/scheme/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/scheme/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauth_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthauthorizetoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthclient.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthclientauthorization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/useroauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
test/library/encryption/assertion.gotest/library/encryption/helpers.go
Comment on lines
+232
to
+242
| if strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | ||
| t.Errorf("access token not encrypted, token received from etcd have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) | ||
| } | ||
| } | ||
|
|
||
| func AssertTokenOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, rawTokenOfLife runtime.Object) { | ||
| t.Helper() | ||
| tokenOfLife := rawTokenOfLife.(*oauthapiv1.OAuthAccessToken) | ||
| rawTokenValue := GetRawTokenOfLife(t, clientSet) | ||
| if !strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | ||
| t.Errorf("access token received from etcd doesnt have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid logging token material from etcd assertions.
These failure messages include both the refresh token marker and raw etcd payload. Even in test helpers, OAuth token payloads should stay redacted in logs.
Proposed fix
rawTokenValue := GetRawTokenOfLife(t, clientSet)
if strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) {
- t.Errorf("access token not encrypted, token received from etcd have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue)
+ t.Errorf("access token not encrypted: raw etcd payload contains the plaintext refresh token marker")
}
}
@@
rawTokenValue := GetRawTokenOfLife(t, clientSet)
if !strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) {
- t.Errorf("access token received from etcd doesnt have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue)
+ t.Errorf("access token received from etcd does not contain the expected plaintext refresh token marker")
}
}📝 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.
Suggested change
| if strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | |
| t.Errorf("access token not encrypted, token received from etcd have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) | |
| } | |
| } | |
| func AssertTokenOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, rawTokenOfLife runtime.Object) { | |
| t.Helper() | |
| tokenOfLife := rawTokenOfLife.(*oauthapiv1.OAuthAccessToken) | |
| rawTokenValue := GetRawTokenOfLife(t, clientSet) | |
| if !strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | |
| t.Errorf("access token received from etcd doesnt have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) | |
| if strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | |
| t.Errorf("access token not encrypted: raw etcd payload contains the plaintext refresh token marker") | |
| } | |
| } | |
| func AssertTokenOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, rawTokenOfLife runtime.Object) { | |
| t.Helper() | |
| tokenOfLife := rawTokenOfLife.(*oauthapiv1.OAuthAccessToken) | |
| rawTokenValue := GetRawTokenOfLife(t, clientSet) | |
| if !strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | |
| t.Errorf("access token received from etcd does not contain the expected plaintext refresh token marker") | |
| } | |
| } |
🤖 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/library/encryption/assertion.go` around lines 232 - 242, The etcd
assertion failure messages in AssertTokenOfLifeNotEncrypted (and the related
token-check helper nearby) are logging sensitive token material by printing the
refresh token marker and raw payload. Update these tests to keep logs redacted:
remove the plain-text token value and raw etcd content from t.Errorf calls, and
replace them with generic failure text that references the assertion name
without exposing token data.
There was a problem hiding this comment.
@ardaguclu Thanks for confirming — keeping those assertion messages redacted is the right fix here.
Comment on lines
+523
to
+530
| resp, err := clientSet.Etcd.Get(timeout, routeOfLifeEtcdPrefix, clientv3.WithPrefix()) | ||
| require.NoError(t, err) | ||
|
|
||
| if len(resp.Kvs) != 1 { | ||
| t.Errorf("Expected to get a single key from etcd, got %d", len(resp.Kvs)) | ||
| } | ||
|
|
||
| return string(resp.Kvs[0].Value) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop before indexing when etcd returns the wrong number of keys.
After t.Errorf, execution continues and resp.Kvs[0] panics when no key is returned. Use a fatal assertion before indexing; an exact Get also avoids prefix collisions with similarly named objects.
Proposed fix
- resp, err := clientSet.Etcd.Get(timeout, routeOfLifeEtcdPrefix, clientv3.WithPrefix())
+ resp, err := clientSet.Etcd.Get(timeout, routeOfLifeEtcdPrefix)
require.NoError(t, err)
-
- if len(resp.Kvs) != 1 {
- t.Errorf("Expected to get a single key from etcd, got %d", len(resp.Kvs))
- }
+ require.Len(t, resp.Kvs, 1, "Expected to get a single key from etcd")
return string(resp.Kvs[0].Value)- resp, err := clientSet.Etcd.Get(timeout, tokenOfLifeEtcdPrefix, clientv3.WithPrefix())
+ resp, err := clientSet.Etcd.Get(timeout, tokenOfLifeEtcdPrefix)
require.NoError(t, err)
-
- if len(resp.Kvs) != 1 {
- t.Errorf("Expected to get a single key from etcd, got %d", len(resp.Kvs))
- }
+ require.Len(t, resp.Kvs, 1, "Expected to get a single key from etcd")
return string(resp.Kvs[0].Value)Also applies to: 576-583
🤖 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/library/encryption/helpers.go` around lines 523 - 530, In the helper
that calls clientSet.Etcd.Get and then indexes resp.Kvs[0], stop execution
immediately when the returned key count is not exactly one by replacing the
non-fatal t.Errorf path with a fatal assertion or equivalent early return before
indexing. Also switch the lookup from a prefix-based Get with
routeOfLifeEtcdPrefix to an exact key fetch in the affected helper(s) so
similarly named objects do not collide and resp.Kvs[0] is only accessed when a
single result is guaranteed.
There was a problem hiding this comment.
@ardaguclu Thanks for confirming — agreed. The helper should fail before indexing resp.Kvs[0], and using an exact etcd key lookup here is the safer choice to avoid prefix collisions.
ardaguclu
reviewed
Jun 29, 2026
| t.Helper() | ||
| t.Logf("Creating %q in %q namespace", "route-of-life", ns) | ||
| rawRouteOfLife := RouteOfLife(t, ns) | ||
| routeOfLife, err := cs.RouteClient.Routes(ns).Create(ctx, rawRouteOfLife.(*routev1.Route), metav1.CreateOptions{}) |
Member
There was a problem hiding this comment.
Instead of importing typed client, maybe we can use dynamicclient. So that we would have less dependency;
func CreateAndStoreRouteOfLife(ctx context.Context, t testing.TB, cs ClientSet, ns string) *routev1.Route {
t.Helper()
obj, err := cs.DynamicClient.Resource(routeGVR).Namespace(ns).Create(ctx, RouteOfLife(ns), metav1.CreateOptions{})
require.NoError(t, err)
var route routev1.Route
err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, &route)
require.NoError(t, err)
return &route
}| return routeOfLife | ||
| } | ||
|
|
||
| func RouteOfLife(t testing.TB, ns string) runtime.Object { |
Member
There was a problem hiding this comment.
With dynamicclient, this can be?
func RouteOfLife(ns string) *unstructured.Unstructured {
return &unstructured.Unstructured{
Object: map[string]interface{}{
"apiVersion": "route.openshift.io/v1",
"kind": "Route",
"metadata": map[string]interface{}{
"name": "route-of-life",
"namespace": ns,
},
"spec": map[string]interface{}{
"host": "devcluster.openshift.io",
"port": map[string]interface{}{
"targetPort": 2014,
},
"to": map[string]interface{}{
"name": "dummyroute",
},
},
},
}
}
Comment on lines
+523
to
+530
| resp, err := clientSet.Etcd.Get(timeout, routeOfLifeEtcdPrefix, clientv3.WithPrefix()) | ||
| require.NoError(t, err) | ||
|
|
||
| if len(resp.Kvs) != 1 { | ||
| t.Errorf("Expected to get a single key from etcd, got %d", len(resp.Kvs)) | ||
| } | ||
|
|
||
| return string(resp.Kvs[0].Value) |
| } | ||
| } | ||
|
|
||
| func AssertRoutes(t testing.TB, clientSet ClientSet, expectedMode configv1.EncryptionType, namespace, labelSelector string) { |
Member
There was a problem hiding this comment.
What about moving this to assertion_auth.go. OAS related ones can be moved under assertion_oas.go.
| } | ||
| } | ||
|
|
||
| func CreateAndStoreRouteOfLife(ctx context.Context, t testing.TB, cs ClientSet, ns string) runtime.Object { |
Member
There was a problem hiding this comment.
This can go under helpers_oas.go
| return string(resp.Kvs[0].Value) | ||
| } | ||
|
|
||
| func CreateAndStoreTokenOfLife(ctx context.Context, t testing.TB, cs ClientSet) runtime.Object { |
Member
There was a problem hiding this comment.
This can go under helpers_auth.go
Comment on lines
+232
to
+242
| if strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | ||
| t.Errorf("access token not encrypted, token received from etcd have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) | ||
| } | ||
| } | ||
|
|
||
| func AssertTokenOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, rawTokenOfLife runtime.Object) { | ||
| t.Helper() | ||
| tokenOfLife := rawTokenOfLife.(*oauthapiv1.OAuthAccessToken) | ||
| rawTokenValue := GetRawTokenOfLife(t, clientSet) | ||
| if !strings.Contains(rawTokenValue, tokenOfLife.RefreshToken) { | ||
| t.Errorf("access token received from etcd doesnt have %q (plain text), raw content in etcd is %s", tokenOfLife.RefreshToken, rawTokenValue) |
Member
|
@gangwgr our plan can be like this;
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/library/encryption/assertion_oas.go`:
- Around line 19-24: Update the route encryption assertions so malformed route
objects fail fast instead of treating a missing spec.to.name as an empty string.
In extractRouteToName, detect when the expected name field is absent or invalid
and return an error or otherwise trigger a test failure, then have
AssertRouteOfLifeEncrypted and the related not-encrypted assertion use that
result before calling strings.Contains on the raw etcd value. This should
surface bad payloads explicitly instead of producing misleading pass/fail
behavior.
🪄 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: b28f5f02-de1f-478b-95da-70e0b2245ff5
⛔ Files ignored due to path filters (19)
vendor/github.com/openshift/client-go/oauth/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/clusterrolescoperestriction.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthauthorizetoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthclient.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/oauthclientauthorization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/scoperestriction.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/applyconfigurations/oauth/v1/useroauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/scheme/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/scheme/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauth_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthauthorizetoken.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthclient.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/oauthclientauthorization.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/oauth/clientset/versioned/typed/oauth/v1/useroauthaccesstoken.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (5)
test/library/encryption/assertion_auth.gotest/library/encryption/assertion_oas.gotest/library/encryption/helpers.gotest/library/encryption/helpers_auth.gotest/library/encryption/helpers_oas.go
ardaguclu
reviewed
Jun 29, 2026
| @@ -0,0 +1,407 @@ | |||
| // Code generated by applyconfiguration-gen. DO NOT EDIT. | |||
Member
There was a problem hiding this comment.
Why do we still have these imports?. Since we use dynamiclient, we shouldn't import these.
gangwgr
force-pushed
the
combine-func
branch
2 times, most recently
from
c2fe187 to
5a27cd2
Compare
ardaguclu
reviewed
Jun 29, 2026
| etcdClient := NewEtcdClient(kubeClient) | ||
|
|
||
| return ClientSet{Etcd: etcdClient, ApiServerConfig: apiServerConfigClient, Kube: kubeClient} | ||
| dynClient, err := dynamic.NewForConfig(kubeConfig) |
Member
There was a problem hiding this comment.
Suggested change
| dynClient, err := dynamic.NewForConfig(kubeConfig) | |
| dynamicClient, err := dynamic.NewForConfig(kubeConfig) |
|
|
||
| func CreateAndStoreTokenOfLife(ctx context.Context, t testing.TB, cs ClientSet) *unstructured.Unstructured { | ||
| t.Helper() | ||
| dynTokens := cs.DynamicClient.Resource(oauthAccessTokenGVR) |
Member
There was a problem hiding this comment.
Suggested change
| dynTokens := cs.DynamicClient.Resource(oauthAccessTokenGVR) | |
| tokens := cs.DynamicClient.Resource(oauthAccessTokenGVR) |
|
|
||
| var routeGVR = schema.GroupVersionResource{Group: "route.openshift.io", Version: "v1", Resource: "routes"} | ||
|
|
||
| func CreateAndStoreRouteOfLife(ctx context.Context, t testing.TB, cs ClientSet, ns string) *unstructured.Unstructured { |
Member
There was a problem hiding this comment.
Why don't we return runtime.Object?
| {Group: "route.openshift.io", Resource: "routes"}, | ||
| } | ||
|
|
||
| func AssertRouteOfLifeEncrypted(t testing.TB, clientSet ClientSet, routeOfLife *unstructured.Unstructured) { |
Member
There was a problem hiding this comment.
Why don't we pass runtime.Object?
| t.Helper() | ||
| name, found, err := unstructured.NestedString(route.Object, "spec", "to", "name") | ||
| require.NoError(t, err) | ||
| require.True(t, found && len(name) > 0, "route spec.to.name is missing") |
Member
There was a problem hiding this comment.
We should have 2 separate require.True
ardaguclu
reviewed
Jun 29, 2026
|
|
||
| func AssertRouteOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, routeOfLife *unstructured.Unstructured) { | ||
| t.Helper() | ||
| toName := extractRouteToName(t, routeOfLife) |
Member
There was a problem hiding this comment.
Instead of dealing with unstructured object, we can use Route by converting unstructured to Route.
Member
|
/retitle NO-JIRA: Moving auth and oas helper funcs to library-go |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request explicitly references no jira issue. 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/library/encryption/assertion_oas.go (1)
20-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFail fast on the
runtime.Objectcontract before asserting encryption state.These helpers still assume
resourceis always a*routev1.Routeand thatSpec.To.Nameis populated. A wrong object now panics at the type assertion, and an empty target name makesstrings.Contains(..., "")produce misleading results. Please addrequire.IsType/checked assertion plus arequire.NotEmptyguard before the plaintext check.Suggested fix
func AssertRouteOfLifeEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { t.Helper() - routeOfLife := resource.(*routev1.Route) + routeOfLife, ok := resource.(*routev1.Route) + require.True(t, ok, "expected *routev1.Route, got %T", resource) + require.NotEmpty(t, routeOfLife.Spec.To.Name, "route spec.to.name is missing") rawRouteValue := GetRawRouteOfLife(t, clientSet, routeOfLife.Namespace) if strings.Contains(rawRouteValue, routeOfLife.Spec.To.Name) { t.Errorf("route not encrypted, etcd value contains target name %q in plain text", routeOfLife.Spec.To.Name) } } func AssertRouteOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { t.Helper() - routeOfLife := resource.(*routev1.Route) + routeOfLife, ok := resource.(*routev1.Route) + require.True(t, ok, "expected *routev1.Route, got %T", resource) + require.NotEmpty(t, routeOfLife.Spec.To.Name, "route spec.to.name is missing") rawRouteValue := GetRawRouteOfLife(t, clientSet, routeOfLife.Namespace) if !strings.Contains(rawRouteValue, routeOfLife.Spec.To.Name) { t.Errorf("route not decrypted, etcd value does not contain target name %q in plain text", routeOfLife.Spec.To.Name) } }Also applies to: 29-34
🤖 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/library/encryption/assertion_oas.go` around lines 20 - 25, In AssertRouteOfLifeEncrypted (and the similar helper at the other referenced block), fail fast before checking etcd plaintext by validating the runtime.Object is actually a *routev1.Route and that routeOfLife.Spec.To.Name is not empty. Replace the direct type assertion with a checked assertion/require.IsType-style guard, add a require.NotEmpty guard for the target name, and only then call GetRawRouteOfLife and strings.Contains.
🤖 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.
Duplicate comments:
In `@test/library/encryption/assertion_oas.go`:
- Around line 20-25: In AssertRouteOfLifeEncrypted (and the similar helper at
the other referenced block), fail fast before checking etcd plaintext by
validating the runtime.Object is actually a *routev1.Route and that
routeOfLife.Spec.To.Name is not empty. Replace the direct type assertion with a
checked assertion/require.IsType-style guard, add a require.NotEmpty guard for
the target name, and only then call GetRawRouteOfLife and strings.Contains.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ab32daf6-e579-423f-a134-bfd22b314955
📒 Files selected for processing (5)
test/library/encryption/assertion_auth.gotest/library/encryption/assertion_oas.gotest/library/encryption/helpers.gotest/library/encryption/helpers_auth.gotest/library/encryption/helpers_oas.go
🚧 Files skipped from review as they are similar to previous changes (3)
- test/library/encryption/helpers.go
- test/library/encryption/helpers_auth.go
- test/library/encryption/assertion_auth.go
ardaguclu
reviewed
Jun 29, 2026
|
|
||
| func AssertRouteOfLifeEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { | ||
| t.Helper() | ||
| routeOfLife := resource.(*routev1.Route) |
Member
There was a problem hiding this comment.
Suggested change
| routeOfLife := resource.(*routev1.Route) | |
| routeOfLife, ok := resource.(*routev1.Route) | |
| if !ok { | |
| t.Fatalf... | |
| } |
|
|
||
| func AssertRouteOfLifeNotEncrypted(t testing.TB, clientSet ClientSet, resource runtime.Object) { | ||
| t.Helper() | ||
| routeOfLife := resource.(*routev1.Route) |
Member
There was a problem hiding this comment.
Suggested change
| routeOfLife := resource.(*routev1.Route) | |
| routeOfLife, ok := resource.(*routev1.Route) |
|
|
||
| const tokenOfLifeName = "sha256~token-aaaaaaaa-of-aaaaaaaa-life-aaaaaaaa" | ||
|
|
||
| func CreateAndStoreTokenOfLife(ctx context.Context, t testing.TB, cs ClientSet) *unstructured.Unstructured { |
Member
There was a problem hiding this comment.
Can we return runtime.Object? or Route?
| return obj | ||
| } | ||
|
|
||
| func TokenOfLife() *unstructured.Unstructured { |
Member
There was a problem hiding this comment.
Can we have Route object and convert it to unstructured just before applying it?
| return &route | ||
| } | ||
|
|
||
| func RouteOfLife(ns string) *unstructured.Unstructured { |
Member
There was a problem hiding this comment.
Can we return as runtime.Object or Route?
Member
|
Changes look good to me. We just need fake PRs on cluster-openshift-apiserver-operator, cluster-authentication-operator to prove that changes are working. |
Contributor
Author
ok checking |
Member
|
/approve |
openshift-ci
Bot
added
the
approved
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
ardaguclu
reviewed
Jun 29, 2026
| return &result | ||
| } | ||
|
|
||
| func TokenOfLifeAsResource(_ testing.TB, _ string) runtime.Object { |
Member
There was a problem hiding this comment.
What is the purpose of this function?
Contributor
Author
There was a problem hiding this comment.
This is a wrapper around TokenOfLife() that matches the ResourceFunc field signature func(testing.TB, string) runtime.Object in the encryption test scenarios (OnOffScenario, ProvidersMigrationScenario, KMSToKMSMigrationScenario). It allows consumers like cluster-authentication-operator to directly assign library.TokenOfLifeAsResource instead of writing an inline adapter:
Member
There was a problem hiding this comment.
Why don't we directly update TokenOfLife with that signature?
ardaguclu
reviewed
Jun 29, 2026
| return newTokenOfLife() | ||
| } | ||
|
|
||
| func newTokenOfLife() *oauthapiv1.OAuthAccessToken { |
Member
There was a problem hiding this comment.
Suggested change
| func newTokenOfLife() *oauthapiv1.OAuthAccessToken { | |
| func TokenOfLife(_ testing.TB, _ string) runtime.Object { |
ardaguclu
reviewed
Jun 29, 2026
| return &result | ||
| } | ||
|
|
||
| func RouteOfLife(ns string) *routev1.Route { |
Member
There was a problem hiding this comment.
Suggested change
| func RouteOfLife(ns string) *routev1.Route { | |
| func RouteOfLife(t testing.TB, ns string) runtime.Object { |
?
Member
|
/lgtm |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, 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 |
Contributor
Author
Contributor
Author
|
/unhold |
Member
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Contributor
|
@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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit