OLS-3632: Add oc-ols CLI scaffolding and kubeconfig integration - #1936
OLS-3632: Add oc-ols CLI scaffolding and kubeconfig integration#1936xiormeesh wants to merge 4 commits into
Conversation
Adds the foundation for the oc-ols kubectl/oc plugin: entry point, root command with global flags and default-mode dispatching, version command, and kubeconfig integration for bearer token extraction and TLS configuration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@xiormeesh: This pull request references OLS-3632 which is a valid 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 11 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe pull request adds the ChangesCLI plugin
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@cli/kubeconfig_test.go`:
- Around line 17-176: Convert the tests in cli/kubeconfig_test.go, including
writeTestKubeconfig and all TestLoadKubeConfig_* cases, from testing.T
assertions to the repository’s established Ginkgo/Gomega BDD structure and
matchers. Preserve each test’s existing coverage and expectations, then run make
test to validate the conversion.
In `@cli/kubeconfig.go`:
- Line 34: Update the error-wrapping paths in the kubeconfig loading flow to
define or reuse appropriate error constants and use each constant as the
fmt.Errorf prefix in the “%s: %w” format. Replace the literal prefixes at the
error sites corresponding to lines 34, 44, 51, and 100 while preserving the
existing wrapped errors and behavior.
- Around line 49-53: Trim whitespace from the contents read in the
BearerTokenFile branch before assigning the result to token, matching client-go
behavior. Add or update the token-file test to write the token followed by a
newline and verify the configured token excludes that newline.
- Around line 64-66: Update the tlsConfig initialization in the kubeconfig TLS
setup to assign ServerName from restConfig.TLSClientConfig.ServerName,
preserving kubeconfig tls-server-name overrides for SNI and certificate
validation. Add a regression test covering propagation of this override.
In `@cli/root.go`:
- Around line 13-26: Update NewRootCmd when constructing the root Cobra command
to bind its input, output, and error streams via SetIn, SetOut, and SetErr using
the injected IOStreams, ensuring help and error output use those streams instead
of process-level defaults.
In `@cli/version_test.go`:
- Around line 8-34: Convert the tests in cli/version_test.go (lines 8-34) and
cli/root_test.go (lines 9-60) from testing.T to the required Ginkgo Describe/It
structure with Gomega assertions, reusing an existing CLI suite or adding one
suite entrypoint as needed. Preserve the current version and root-command
behavior checks, including injected Version validation, and verify the changes
with make test.
In `@cli/version.go`:
- Around line 19-20: Wrap both command output write failures with dedicated
error constants: update cli/version.go lines 19-20 around the version-output
write to return fmt.Errorf with its new constant and the original error, and
update cli/root.go lines 21-22 around the default-mode error-output write
similarly with a separate constant; preserve successful output behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bb0ae2a4-94e2-45d2-83a1-187a08028ddb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!go.sum
📒 Files selected for processing (11)
.ai/spec/how/cli.mdAGENTS.mdcli/kubeconfig.gocli/kubeconfig_test.gocli/root.gocli/root_test.gocli/testutil_test.gocli/version.gocli/version_test.gocmd/oc-ols/main.gogo.mod
| tlsConfig := &tls.Config{ | ||
| InsecureSkipVerify: insecureSkipTLS, //#nosec G402 -- user-controlled via --insecure-skip-tls-verify flag | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
curl -fsSL https://raw.githubusercontent.com/kubernetes/client-go/v0.36.3/rest/config.go |
sed -n '213,240p'Repository: openshift/lightspeed-operator
Length of output: 1295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file list =="
git ls-files | rg '(^|/)cli/kubeconfig\.go$|go\.mod$|go\.sum$' | sed -n '1,120p'
echo "== kubeconfig.go =="
if [ -f cli/kubeconfig.go ]; then
nl -ba cli/kubeconfig.go | sed -n '1,180p'
fi
echo "== relevant config references =="
rg -n "TLSClientConfig|TLSClient|ServerName|InsecureSkipVerify|NewClientSet|config.*Context|restConfig" cli -S || true
echo "== kubernetes client-go version =="
rg -n "k8s.io/client-go" go.mod go.sum || true
echo "== lock file / installed cache probe (read-only) =="
find "$GOPATH" -path '*/pkg/mod/k8s.io/client-go@v0.36.3/rest/config.go' 2>/dev/null | head -n 1 | xargs -r -I{} sh -c 'echo "--- {}"; sed -n "200,260p" "{}"'Repository: openshift/lightspeed-operator
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate kubeconfig.go =="
fd -a kubeconfig\.go . || find . -name kubeconfig.go -print
echo "== cli/kubeconfig.go (sed) =="
sed -n '1,220p' cli/kubeconfig.go
echo "== relevant config references =="
rg -n "TLSClientConfig|TLSClient|ServerName|InsecureSkipVerify|NewClientSet|restConfig|tls-server-name" cli -S || true
echo "== kubernetes client-go version =="
rg -n "k8s.io/client-go" go.mod go.sum || true
echo "== go command availability =="
command -v go || trueRepository: openshift/lightspeed-operator
Length of output: 4455
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== client-go `@v0.36.3` RESTConfig/ClientSet TLS flow =="
tmp=/tmp/client-go-v0.36.3
mkdir -p "$tmp"
curl -fsSL https://raw.githubusercontent.com/kubernetes/client-go/v0.36.3/rest/config.go -o "$tmp/config.go"
curl -fsSL https://raw.githubusercontent.com/kubernetes/client-go/v0.36.3/kubernetes/client.go -o "$tmp/client.go"
printf '\n--- config.go TLS assignment ---\n'
rg -n "TLSClientConfig|ServerName|Insecure|CertFile|KeyFile|CAFile|CAData" "$tmp/config.go" -C 2
printf '\n--- config.go BuildConfigFromFlags/RESTConfig relevant block ---\n'
sed -n '270,380p' "$tmp/config.go"
printf '\n--- client.go TLS dialer construction ---\n'
rg -n "TLSClient|TLSConfig|ServerName|DialTLS|DialContext" "$tmp/client.go" -C 3
printf '\n--- kubeconfig_test tls tests ---\n'
sed -n '1,140p' cli/kubeconfig_test.goRepository: openshift/lightspeed-operator
Length of output: 273
🏁 Script executed:
#!/bin/bash
set -euo pipe Fail
echo "== Go cache availability =="
go env GOPATH GOMODCACHE GOROOT
echo "== Locate installed client-go v0.36.3 files =="
modpath="$GOPATH/pkg/mod/k8s.io/client-go@v0.36.3"
if [ -d "$modpath" ]; then
ls -l "$modpath"
for f in "$modpath/rest/config.go" "$modpath/kubernetes/client.go"; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "TLSClientConfig|ServerName|Insecure|CertFile|KeyFile|CAFile|CAData|TLSConfig|TLSClient|DialTLS|DialContext" "$f" -C 2 || true
fi
done
else
echo "client-go v0.36.3 not in GOMODCACHE=$GOMODCACHE"
fi
echo "== Kubeconfig test context around TLS assertions =="
if [ -f cli/kubeconfig_test.go ]; then
sed -n '1,180p' cli/kubeconfig_test.go
fiRepository: openshift/lightspeed-operator
Length of output: 212
🏁 Script executed:
#!/bin/bash
set -f
set -- 1
echo "== Go cache availability =="
go env GOPATH GOMODCACHE GOROOT
echo "== Locate installed client-go v0.36.3 files =="
modpath="$GOPATH/pkg/mod/k8s.io/client-go@v0.36.3"
if [ -d "$modpath" ]; then
ls -l "$modpath"
for f in "$modpath/rest/config.go" "$modpath/kubernetes/client.go"; do
if [ -f "$f" ]; then
echo "--- $f ---"
rg -n "TLSClientConfig|ServerName|Insecure|CertFile|KeyFile|CAFile|CAData|TLSConfig|TLSClient|DialTLS|DialContext" "$f" -C 2 || true
fi
done
else
echo "client-go v0.36.3 not in GOMODCACHE=$GOMODCACHE"
fi
echo "== Kubeconfig test context around TLS assertions =="
if [ -f cli/kubeconfig_test.go ]; then
sed -n '1,180p' cli/kubeconfig_test.go
fiRepository: openshift/lightspeed-operator
Length of output: 4667
🌐 Web query:
kubernetes client-go v0.36.3 TLSClientConfig.ServerName SNI certificate verification
💡 Result:
In Kubernetes client-go v0.36.3, the ServerName field within TLSClientConfig serves two critical functions regarding TLS connections [1][2][3]: 1. SNI (Server Name Indication): It is passed to the server as the hostname for the SNI extension during the TLS handshake [1][2][3]. 2. Certificate Verification: It is used by the client to verify the server's certificate [1][2][3]. Specifically, the client checks that the provided ServerName matches the hostnames present in the server's certificate [1][2][3]. If ServerName is left empty, the client-go library automatically defaults to using the hostname from the server's connection URL for both SNI and certificate verification purposes [1][4][2][3]. This implementation ensures that both SNI routing and proper certificate identity validation are maintained consistently [5][6].
Citations:
- 1: https://github.com/kubernetes/client-go/blob/master/rest/config.go
- 2: https://github.com/kubernetes/client-go/blob/32801e2d7c449af3373a7728b1b3f9b9c19c1bdb/rest/config.go
- 3: https://github.com/kubernetes/client-go/blob/v0.35.3/rest/config.go
- 4: https://kubernetes.io/docs/reference/config-api/client-authentication.v1/
- 5: https://github.com/kubernetes/client-go/blob/master/transport/config.go
- 6: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/client-go/rest/transport.go
Preserve the kubeconfig TLS server name.
Set tlsConfig.ServerName from restConfig.TLSClientConfig.ServerName. A kubeconfig tls-server-name override is needed so the Kubernetes API client uses that value for SNI and certificate matching. Also add a regression test for this override.
Proposed fix
tlsConfig := &tls.Config{
InsecureSkipVerify: insecureSkipTLS, //#nosec G402 -- user-controlled via --insecure-skip-tls-verify flag
+ ServerName: restConfig.TLSClientConfig.ServerName,
}🤖 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 `@cli/kubeconfig.go` around lines 64 - 66, Update the tlsConfig initialization
in the kubeconfig TLS setup to assign ServerName from
restConfig.TLSClientConfig.ServerName, preserving kubeconfig tls-server-name
overrides for SNI and certificate validation. Add a regression test covering
propagation of this override.
- Define error constants for all error wrapping paths - Trim whitespace from bearer tokens (matches client-go behavior) - Preserve kubeconfig tls-server-name (ServerName) in TLS config - Bind Cobra streams to IOStreams via SetIn/SetOut/SetErr - Wrap fmt.Fprintf errors with ErrWriteOutput constant - Add ServerName propagation test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Follow the repo's BDD test convention per CONTRIBUTING.md. Add suite_test.go entrypoint, convert all test files to Describe/It blocks with Gomega matchers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@xiormeesh: 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. |
2 participants
Description
Adds the foundation for the
oc-olskubectl/oc plugin — a CLI for querying OpenShift Lightspeed from the terminal. This is the first PR in the OLS-1062 epic (9 stories total).Structure follows the oc-agentic CLI pattern: thin entry point in
cmd/oc-ols/, all command logic incli/. Unlike oc-agentic (which is a K8s API client via controller-runtime), oc-ols is a REST client that extracts bearer tokens and TLS config from kubeconfig to make HTTP calls to the OLS service endpoint.New direct dependencies:
spf13/cobra(CLI framework) andk8s.io/cli-runtime(IOStreams). Both are standard kubectl ecosystem libraries.oc-olsrequires token-based authentication (bearer token from kubeconfig). Client-certificate-only contexts (e.g. kubeadmin) are rejected — users mustoc loginwith username/password or SSO first. This is by design: OLS performs user-level authorization via the token, per.ai/spec/how/cli.md(Kubeconfig integration).Build:
go build -o /tmp/oc-ols ./cmd/oc-ols/Type of change
Related Tickets & Documents
Checklist before requesting a review
Testing
make test— full operator suite passes (0 failures, our code adds none)go build -o /tmp/oc-ols ./cmd/oc-ols/compiles successfully/tmp/oc-ols version→ "oc-ols dev"/tmp/oc-ols "hello"→ default mode dispatch stub on stderr/tmp/oc-ols --help→ shows global flags and version subcommandgo build -ldflags "-X github.com/openshift/lightspeed-operator/cli.Version=v0.1.0" -o /tmp/oc-ols ./cmd/oc-ols/ && /tmp/oc-ols version→ "oc-ols v0.1.0"Summary by CodeRabbit
New Features
oc-olscommand-line plugin entry point.Tests
Documentation