test(tls): add unit tests for pkg/tls#1515
Conversation
Cover parseTLSSecurityProfile, GetTLSProfileSpec, FetchAPIServerTLSConfig, NewTLSConfigFromProfile (including the TLS-1.3 CipherSuites sentinel assertion), and exact pinning of the canned TLSProfiles[Old|Intermediate| Modern] cipher lists and MinTLSVersion. Also cover the cipher helpers (tlsVersion, tlsVersionOrDie, cipherSuite, openSSLToIANACipherSuites, cipherCode, cipherCodes) and SecurityProfileWatcher.Reconcile across the APIServer-absent, profile-only, adherence-only, both-change, steady-state, nil-callback, and parse-error paths. Tests use Ginkgo v2 + Gomega and a controller-runtime fake client; no production code changes. Signed-off-by: Mat Kowalski <mko@redhat.com>
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. 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. |
kubevirt-bot
added
the
dco-signoff: yes
kubevirt-bot
added
the
do-not-merge/release-note-label-needed
|
[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 |
|
/hold |
kubevirt-bot
added
the
do-not-merge/hold
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of tests for the TLS package, covering cipher suite mapping, version resolution, and the security profile controller. Feedback identifies a critical signature mismatch in the SecurityProfileWatcher callback recorder that will lead to compilation errors. Additionally, a suggestion was provided to correct misleading comments regarding the contents of the cipher translation table to accurately reflect the inclusion of TLS 1.3 IANA names.
| newAdherence TLSAdherencePolicy | ||
| } | ||
|
|
||
| func (r *callbackRecorder) fn() func(ctx context.Context, oldProfile, newProfile TLSProfileSpec, oldAdherence, newAdherence TLSAdherencePolicy) { |
There was a problem hiding this comment.
The callback signature in callbackRecorder.fn includes oldAdherence and newAdherence parameters, but the OnProfileChange field in the SecurityProfileWatcher struct (as defined in pkg/tls/controller.go) currently only accepts three arguments: ctx, oldTLSProfileSpec, and newTLSProfileSpec. This mismatch will cause a compilation error. Ensure the production code is updated to match this signature before merging.
| It("does not pass through an IANA name directly (only OpenSSL names are in the table)", func() { | ||
| // Sanity check: the table is OpenSSL-keyed, so feeding an IANA-style | ||
| // name yields no output. cipherCode() compensates by trying IANA first. |
There was a problem hiding this comment.
The comment and test description are slightly misleading. The openSSLToIANA table in pkg/tls/ciphers.go actually contains IANA names as keys for TLS 1.3 cipher suites (e.g., "TLS_AES_128_GCM_SHA256"). The test correctly identifies that TLS 1.2 IANA names are missing from the keys, but the generalization that "only OpenSSL names are in the table" is incorrect.
| It("does not pass through an IANA name directly (only OpenSSL names are in the table)", func() { | |
| // Sanity check: the table is OpenSSL-keyed, so feeding an IANA-style | |
| // name yields no output. cipherCode() compensates by trying IANA first. | |
| It("does not pass through a TLS 1.2 IANA name directly (only OpenSSL and TLS 1.3 IANA names are in the table)", func() { | |
| // Sanity check: the table is primarily OpenSSL-keyed (except for TLS 1.3), | |
| // so feeding a TLS 1.2 IANA-style name yields no output. cipherCode() compensates by trying IANA first. |
|
@mkowalski: The following test failed, say
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
Summary
pkg/tlspackage:parseTLSSecurityProfile,GetTLSProfileSpec,FetchAPIServerTLSConfig,NewTLSConfigFromProfile(including the documented TLS-1.3CipherSuitessentinel-untouched contract), and exact pinning ofTLSProfiles[Old|Intermediate|Modern]cipher lists +MinTLSVersion.tlsVersion,tlsVersionOrDie,cipherSuite,openSSLToIANACipherSuites,cipherCode,cipherCodes) andSecurityProfileWatcher.Reconcileacross APIServer-absent, profile-only-change, adherence-only-change, both-change, steady-state, nil-callback, and parse-error paths.controller-runtimefake client.Note
This PR depends on the
tlsAdherenceproduction-code work (introducingTLSAdherencePolicy,parseTLSAdherence,FetchAPIServerTLSConfig, andSecurityProfileWatcher.InitialTLSAdherencePolicy). It will compile and pass once that change lands; opened now to keep the test coverage ready for review alongside it.76 specs, all passing locally against the adherence branch.