[WIP]: OCPBUGS-46086: Always set service-account-jwks-uri to LB URL even with custom issuer #1919
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@ShazaAldawamneh: This pull request references Jira Issue OCPBUGS-46086, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In 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. |
WalkthroughUnconditional configuration of service-account-jwks-uri based on APIServerURL was introduced, decoupling it from the issuer check. Error handling for missing APIServerURL now triggers regardless of issuer. Corresponding tests were updated to expect the JWKS URI in more scenarios, with a new test case added. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10–15 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ShazaAldawamneh
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ShazaAldawamneh The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![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
📜 Review details
Configuration used: CodeRabbit 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 (3)
pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go(4 hunks)tls.crt(1 hunks)tls.key(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tls.crt
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go
🧰 Additional context used
🪛 Gitleaks (8.27.2)
tls.key
[high] 1-27: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go (6)
57-63: Reduce duplication with later custom-issuer case.This test is effectively duplicated by the new “custom issuer, no previous issuer” case at Lines 114–121. Either drop one or vary inputs (e.g., add trustedIssuers) to cover a distinct path.
114-121: Good addition; add a negative test for empty APIServerURL.To lock in the error-path contract, add a test where Infrastructure.Status.APIServerURL is empty and verify: an error is surfaced and JWKS URI is not set.
Here’s a self-contained test you can append:
+func TestObservedConfig_EmptyAPIServerURL(t *testing.T) { + testRecorder := events.NewInMemoryRecorder("SAIssuerTest", clock.RealClock{}) + expectedErr := fmt.Errorf("APIServerURL is empty") + newConfig, errs := observedConfig( + unstructuredAPIConfigForIssuer(t, defaultServiceAccountIssuerValue, nil), + func(_ string) (*operatorv1.KubeAPIServer, error) { + return kasStatusForIssuer(defaultServiceAccountIssuerValue), nil + }, + func(_ string) (*configv1.Infrastructure, error) { + return &configv1.Infrastructure{Status: configv1.InfrastructureStatus{APIServerURL: ""}}, expectedErr + }, + testRecorder, + ) + // Unmarshal and assert JWKS not present + unstructuredConfig := unstructured.Unstructured{Object: newConfig} + raw, err := unstructuredConfig.MarshalJSON() + require.NoError(t, err) + cfg := &kubecontrolplanev1.KubeAPIServerConfig{TypeMeta: metav1.TypeMeta{Kind: "KubeAPIServerConfig"}} + require.NoError(t, json.Unmarshal(raw, cfg)) + if _, ok := cfg.APIServerArguments["service-account-jwks-uri"]; ok { + t.Fatalf("expected service-account-jwks-uri to be absent when APIServerURL is empty") + } + require.Contains(t, errs, expectedErr) +}
174-175: Swap expected/actual in assert for clarity.Keep testify’s expected, actual order.
- require.Equal(t, uri, kubecontrolplanev1.Arguments{testLBURI}) + require.Equal(t, kubecontrolplanev1.Arguments{testLBURI}, uri)
178-178: Avoid calling String() on slice type; print the value directly.kubecontrolplanev1.Arguments is a slice; String() may not exist across versions.
- t.Errorf("expected no service-account-jwks-uri to be set, it is %+v", uri.String()) + t.Errorf("expected no service-account-jwks-uri to be set, it is %+v", uri)
215-218: Remove commented-out code in helper; keep intent crisp.These deletes are confusing in tests. Drop them.
- //delete(args, "service-account-issuer") - //delete(args, "api-audiences") args["service-account-jwks-uri"] = kubecontrolplanev1.Arguments{testLBURI}
39-40: Terminology nit: JWKS vs. JWKI.Rename expectInternalJWKI to expectJWKSURI (or similar) for accuracy.
Also applies to: 170-177
📜 Review details
Configuration used: CodeRabbit 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 (1)
pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go(5 hunks)
🔇 Additional comments (5)
pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go (5)
71-71: JWKS URI should always be set to LB URL — good.
75-80: LGTM: same issuer/no change still asserts JWKS URI present.
88-88: LGTM: trusted issuers preserved while JWKS URI asserted.
91-97: LGTM: issuer change triggers event and keeps JWKS URI.
99-105: Verify behavior when auth getter errors.You still assert JWKS URI presence when auth returns an error. Confirm the observer keeps computing JWKS from Infrastructure.Status.APIServerURL even if the auth getter errors (and only fails if APIServerURL is empty).
Signed-off-by: Shaza Aldawamneh <[email protected]>
ShazaAldawamneh
force-pushed
the
OCPBUGS-46086
branch
from
f33ce1f to
47ee8ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go (5)
57-63: Good expansion of table cases to always expect JWKS at the LB URL.Covers key transitions (default→custom, custom unchanged, error pass‑through). Consider adding one more case where Infrastructure.Status.APIServerURL is empty to assert: (a) an error is surfaced and (b) service-account-jwks-uri is not set.
Sample table entry to add:
+ { + name: "missing APIServerURL yields error and no jwks-uri", + existingIssuer: defaultServiceAccountIssuerValue, + issuer: defaultServiceAccountIssuerValue, + // infra getter returns empty APIServerURL + infraError: fmt.Errorf("no APIServerURL"), + expectedIssuer: defaultServiceAccountIssuerValue, + // keep this false if observed code skips jwks-uri on missing APIServerURL + expectInternalJWKI: false, + expectedChange: true, + },Also applies to: 71-71, 75-80, 88-88, 91-97, 99-105, 114-121
174-175: Swap expected/actual in require.Equal for clearer failures.Testify expects require.Equal(t, expected, actual).
- require.Equal(t, uri, kubecontrolplanev1.Arguments{testLBURI}) + require.Equal(t, kubecontrolplanev1.Arguments{testLBURI}, uri)
178-178: Avoid calling String() on kubecontrolplanev1.Arguments.Safer to format the slice directly; avoids coupling to a String() method.
- t.Errorf("expected no service-account-jwks-uri to be set, it is %+v", uri.String()) + t.Errorf("expected no service-account-jwks-uri to be set, it is %+v", uri)
215-217: Remove commented‑out deletes; keep expected args explicit.Dead commented code adds noise in tests.
- //delete(args, "service-account-issuer") - //delete(args, "api-audiences") args["service-account-jwks-uri"] = kubecontrolplanev1.Arguments{testLBURI}
61-61: Nit: rename expectInternalJWKI → expectJWKSURI (or expectJWKS).Current name reads like “JWKI”. Renaming improves readability and avoids confusion with “JWKS”.
If you want, I can generate a quick sed/rg script to safely rename across the file.
📜 Review details
Configuration used: CodeRabbit 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)
pkg/operator/configobservation/auth/auth_serviceaccountissuer.go(1 hunks)pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/configobservation/auth/auth_serviceaccountissuer.go
|
@ShazaAldawamneh: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Previously, when a custom service account issuer was set, the
service-account-jwks-uri argument was not configured in the KubeAPIServer,
causing the JWKS URI to default to the node IP. This led to TLS errors
because the node IP is not included in the certificate SAN.
This commit updates observedConfig to always set
service-account-jwks-uri to the API LB URL regardless of whether the
issuer is default or custom. Unit tests have been updated to validate
this behavior.
Fixes: TLS SAN issues for clients accessing JWKS URI with custom issuers.