CNTRLPLANE-3428: tls: add cluster-authentication-operator and operands to tested compo…#31208
CNTRLPLANE-3428: tls: add cluster-authentication-operator and operands to tested compo…#31208gangwgr wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughParameterize where TLS is read from ObservedConfig with a per-target ChangesTLS ObservedConfig authentication extension
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/tls/tls_observed_config.go (1)
836-836: ⚡ Quick winUse
siLabelinstead of hardcoded "servingInfo" for log consistency.Line 829 correctly uses
siLabelwhen logging minTLSVersion, but this line hardcodes "servingInfo". For the authentication operator (which uses path["oauthServer", "servingInfo"]), the logs would inconsistently show "oauthServer.servingInfo" for minTLSVersion but just "servingInfo" for cipherSuites.Suggested fix
- e2e.Logf("ObservedConfig servingInfo.cipherSuites: %d suites", len(cipherSuites)) + e2e.Logf("ObservedConfig %s.cipherSuites: %d suites", siLabel, len(cipherSuites))🤖 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` at line 836, Replace the hardcoded "servingInfo" string in the e2e.Logf call with the siLabel variable to keep log labels consistent (the line containing e2e.Logf("ObservedConfig servingInfo.cipherSuites: %d suites", len(cipherSuites))). Update the call to use siLabel so it matches the earlier log of minTLSVersion and reflects the full path (e.g., "oauthServer.servingInfo") dynamically.
🤖 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.
Nitpick comments:
In `@test/extended/tls/tls_observed_config.go`:
- Line 836: Replace the hardcoded "servingInfo" string in the e2e.Logf call with
the siLabel variable to keep log labels consistent (the line containing
e2e.Logf("ObservedConfig servingInfo.cipherSuites: %d suites",
len(cipherSuites))). Update the call to use siLabel so it matches the earlier
log of minTLSVersion and reflects the full path (e.g.,
"oauthServer.servingInfo") dynamically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f0d298ae-4ee8-49e1-9cb9-7db47797c99f
📒 Files selected for processing (1)
test/extended/tls/tls_observed_config.go
openshift-ci
Bot
added
the
ready-for-human-review
|
/test tls-observed-config-hypershift |
|
/test tls-observed-config-hypershift |
|
/test tls-observed-config |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
/test e2e-aws-ovn-serial-1of2 |
|
/verified by ci runs |
openshift-ci-robot
added
the
verified
|
@gangwgr: This PR has been marked as verified by 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. |
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request references CNTRLPLANE-3428 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
/lgtm |
| namespace string | ||
| operatorConfigGVR schema.GroupVersionResource | ||
| operatorConfigName string | ||
| servingInfoPath []string // default: ["servingInfo"] |
There was a problem hiding this comment.
Why not set the path explicitly instead of defaulting? Right now the defaulting is set at two different places. Which is not a good coding pattern.
There was a problem hiding this comment.
I doesn't want to impact old namespace, that's why added it
now I have updated pr
|
Looks like this PR got automatically approved without approving it? |
openshift-ci
Bot
added
the
do-not-merge/hold
|
Was the PR approved by coderabbitai? |
|
/coderabbitai review |
CodeRabbit |
…nents Add the authentication operator (openshift-authentication-operator) and its OAuth server operand (openshift-authentication) to all target lists: observedConfigTargets, configMapTargets, serviceTargets, clusterOperatorTargets, and deploymentRolloutTargets. The authentication operator uses a non-standard ObservedConfig path (oauthServer.servingInfo instead of servingInfo). Add a servingInfoPath field to observedConfigTarget so testObservedConfig and verifyObservedConfigForTargets resolve the path dynamically, defaulting to ["servingInfo"] for all existing targets.
openshift-ci-robot
removed
the
verified
|
/lgtm |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gangwgr, ingvagabund, sandeepknd 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 |
I am sorry. Is that yes or no? If coderabbitai can approve a PR I don't think I like it without knowing which rules it follows. |
|
Oh, @gangwgr I see now. Because you are approver under https://github.com/openshift/origin/blob/main/test/extended/tls/OWNERS the PR got automatically self-approved. Perhaps, good to update the config as in https://github.com/openshift/release/pull/78243/files. |
|
/test tls-observed-config-hypershift |
|
Scheduling required tests: |
|
@gangwgr: The following test failed, say
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. |
4 participants
for https://github.com/openshift/cluster-authentication-operator/pull/892]
Add the authentication operator (openshift-authentication-operator) and its OAuth server operand (openshift-authentication) to all target lists: observedConfigTargets, configMapTargets, serviceTargets, clusterOperatorTargets, and deploymentRolloutTargets.
The authentication operator uses a non-standard ObservedConfig path (oauthServer.servingInfo instead of servingInfo). Add a servingInfoPath field to observedConfigTarget so testObservedConfig and verifyObservedConfigForTargets resolve the path dynamically, defaulting to ["servingInfo"] for all existing targets.
Summary by CodeRabbit