OCPBUGS-99014: Authenticate Konnectivity agents with cluster CA#9031
OCPBUGS-99014: Authenticate Konnectivity agents with cluster CA#9031hlipsig wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@hlipsig: 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. |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
do-not-merge/needs-area
area/control-plane-operator
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (5)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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: 2
🧹 Nitpick comments (1)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go (1)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the required “When ... it should ...” test description format.
Rename or wrap this test so its case description follows the repository convention, for example
When configured with the cluster CA, it should authenticate agents.🤖 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 `@control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go` at line 55, Rename TestKonnectivityServerAuthenticatesAgents to use the required “When ..., it should ...” test description convention, such as describing the cluster CA configuration and expected agent authentication behavior.Source: Coding guidelines
🤖 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
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go`:
- Around line 59-60: Update the test around
LoadDeploymentManifest("kube-apiserver") to stop execution immediately when the
lookup returns an error or nil deployment, before any deployment.Spec
dereference; use the test’s fatal/early-return mechanism and apply the same
guard before dereferencing konnectivityServer.Args.
- Around line 71-74: Update the konnectivityServer.Args assertion in the
relevant test to verify that “--cluster-ca-cert” is immediately followed by
“/etc/konnectivity/ca/ca.crt”, using an ordered-pair or exact-subsequence
assertion instead of ContainElements.
---
Nitpick comments:
In
`@control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.go`:
- Line 55: Rename TestKonnectivityServerAuthenticatesAgents to use the required
“When ..., it should ...” test description convention, such as describing the
cluster CA configuration and expected agent authentication 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: fa825fbc-fa44-44fa-aec7-729246c79fbe
📒 Files selected for processing (2)
control-plane-operator/controllers/hostedcontrolplane/v2/assets/assets_test.gocontrol-plane-operator/controllers/hostedcontrolplane/v2/assets/kube-apiserver/deployment.yaml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9031 +/- ##
==========================================
- Coverage 44.17% 44.17% -0.01%
==========================================
Files 772 772
Lines 96334 96352 +18
==========================================
+ Hits 42559 42565 +6
- Misses 50831 50843 +12
Partials 2944 2944 see 2 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Require Konnectivity agents to present certificates signed by the hosted cluster's existing Konnectivity CA. Add a manifest regression test so agent authentication cannot be removed unnoticed. Signed-off-by: Hilliary Lipsig (RED HAT INC) <b-hlipsig@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Stop immediately when manifest lookup prerequisites fail. Verify the cluster CA path is the value directly following its proxy-server flag. Signed-off-by: Hilliary Lipsig (RED HAT INC) <b-hlipsig@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
7af17d9 to
69928e6
Compare
|
/retitle OCPBUGS-99014: Authenticate Konnectivity agents with cluster CA |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-bug
|
@hlipsig: This pull request references Jira Issue OCPBUGS-99014, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bryan-cox, hlipsig 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 |
openshift-ci
Bot
added
the
approved
|
The UPDATE=true go test ./control-plane-operator/controllers/hostedcontrolplane/...This will update all the |
|
The PR lgtm, pls fix the unit tests and I can tag. |
Regenerate the kube-apiserver deployment fixtures so each platform includes the Konnectivity agent client CA configuration. Signed-off-by: Hilliary Lipsig (RED HAT INC) <b-hlipsig@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
|
Scheduling tests matching the |
Manual Verification ReportTested on a live HostedCluster with the PR's CPO image deployed. Environment:
Results: 9/9 passed
Key observations:
|
|
/verified by @csrwng |
openshift-ci-robot
added
the
verified
|
@csrwng: 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. |
|
Test plan and script here: https://gist.github.com/csrwng/94e9e6d68a61f346823c29d61f4bd9d6 |
csrwng
added
the
acknowledge-critical-fixes-only
Test Resultse2e-aws
Failed TestsTotal failed tests: 8
... and 3 more failed tests e2e-aks
|
|
@hlipsig: 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. |
5 participants
What this PR does / why we need it:
The Konnectivity proxy-server currently accepts agent connections without validating client certificates because its agent listener is not configured with a cluster CA. A reachable client can therefore connect without a certificate signed by the hosted cluster's Konnectivity CA.
Configure
--cluster-ca-certwith the existing mounted Konnectivity CA bundle. Legitimate agents already use certificates signed by this CA. Add a regression test that verifies the embedded kube-apiserver manifest retains the authentication setting.Which issue(s) this PR fixes:
NO-JIRA
Special notes for your reviewer:
Focused tests passed:
GO111MODULE=on GOWORK=off GOFLAGS=-mod=vendor go test -race ./control-plane-operator/controllers/hostedcontrolplane/v2/assets ./control-plane-operator/controllers/hostedcontrolplane/v2/kasChecklist:
Summary by CodeRabbit
--cluster-ca-certargument pointing to the mounted Konnectivity CA file.--cluster-ca-certsetting.