Skip to content

metrics-server: inherit TLS options from CDI TLSSecurityProfile#4033

Merged
kubevirt-bot merged 1 commit into
kubevirt:mainfrom
Acedus:tls-security-profile-propagation
Mar 8, 2026
Merged

metrics-server: inherit TLS options from CDI TLSSecurityProfile#4033
kubevirt-bot merged 1 commit into
kubevirt:mainfrom
Acedus:tls-security-profile-propagation

Conversation

@Acedus
Copy link
Copy Markdown
Contributor

@Acedus Acedus commented Feb 9, 2026 •
edited
Loading

What this PR does / why we need it:
Previously the metrics-server would be initiated with default TLS configuration. This change makes it so the TLS configuration is updated during runtime for every request according to the CDI TLSSecurityProfile.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Partially addresses https://issues.redhat.com/browse/CNV-78858

Special notes for your reviewer:

Release note:

fix: cdi-operator and cdi-deployment metrics-server now correctly inherit TLS options from the CDI TLSSecrutiyProfile.

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 9, 2026
@Acedus
Copy link
Copy Markdown
Contributor Author

Acedus commented Feb 9, 2026

/cc @akalenyu

@kubevirt-bot kubevirt-bot requested a review from akalenyu February 9, 2026 17:06
@Acedus
Copy link
Copy Markdown
Contributor Author

Acedus commented Feb 9, 2026

fyi @Barakmor1

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 9, 2026 •
edited
Loading

Coverage Status

coverage: 49.33% (-0.03%) from 49.364%
when pulling 865def3 on Acedus:tls-security-profile-propagation
into b17a4d3 on kubevirt:main.

@Acedus
Copy link
Copy Markdown
Contributor Author

Acedus commented Feb 10, 2026

/retest-required

@Acedus Acedus force-pushed the tls-security-profile-propagation branch from 1430322 to 064785f Compare February 10, 2026 09:29
@Barakmor1 Barakmor1 mentioned this pull request Feb 10, 2026
Merged
11 tasks
Barakmor1
Barakmor1 reviewed Feb 10, 2026
View reviewed changes
Comment thread pkg/util/tls-crypto-watch/tls-crypto-watch.go Outdated

// NewCdiConfigTLSWatcher crates a new cdiConfigTLSWatcher
func NewCdiConfigTLSWatcher(ctx context.Context, cdiClient cdiclient.Interface) (CdiConfigTLSWatcher, error) {
func NewCdiConfigTLSWatcher(ctx context.Context, cdiClient cdiclient.Interface) (cache.SharedIndexInformer, CdiConfigTLSWatcher, error) {
Copy link
Copy Markdown
Member

@Barakmor1 Barakmor1 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why is the additional informer output needed?

Copy link
Copy Markdown
Contributor Author

@Acedus Acedus Feb 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certificate secrets creation for the apiserver and upload-proxy are dependent on the existence of the CDI CR, previously during initialization both called NewCdiConfigTLSWatcher before NewCertWatcher which provided the cache sync, having it removed (as neither cdi-operator nor cdi-deployment can make use of NewCdiConfigTLSWatcher with it) makes it so the apiserver would fail initialization once and eventually succeed as the secrets get created and populated. This change just ensures that we keep logic as close as possible to the original.

Barakmor1
Barakmor1 reviewed Feb 10, 2026
View reviewed changes
Comment thread pkg/util/tls-crypto-watch/tls-crypto-watch.go
Comment on lines -97 to -99
klog.V(3).Infoln("Waiting for cache sync")
cache.WaitForCacheSync(ctx.Done(), cdiConfigInformer.HasSynced)
klog.V(3).Infoln("Cache sync complete")
Copy link
Copy Markdown
Member

@Barakmor1 Barakmor1 Feb 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for the logs removal

nunnatsa
nunnatsa reviewed Feb 10, 2026
View reviewed changes
Comment thread cmd/cdi-controller/controller.go Outdated
@Acedus
Copy link
Copy Markdown
Contributor Author

Acedus commented Feb 10, 2026

/retest-required

akalenyu
akalenyu reviewed Feb 11, 2026
View reviewed changes
Comment thread cmd/cdi-operator/operator.go Outdated
@Acedus Acedus force-pushed the tls-security-profile-propagation branch from 064785f to f26f013 Compare February 11, 2026 14:53
akalenyu
akalenyu reviewed Feb 18, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@akalenyu akalenyu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@kubevirt-bot
Copy link
Copy Markdown
Contributor

kubevirt-bot commented Feb 18, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akalenyu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2026
@akalenyu
Copy link
Copy Markdown
Collaborator

akalenyu commented Feb 18, 2026

/retest

@akalenyu
Copy link
Copy Markdown
Collaborator

akalenyu commented Feb 18, 2026

sonar is unhappy about the duplication but I don't think it makes sense to extract the callback

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2026
@awels
Copy link
Copy Markdown
Member

awels commented Feb 25, 2026

sonar is unhappy about the duplication but I don't think it makes sense to extract the callback

I excluded cmd from the duplication checks in sonar. Good thing it is not a voting lane, but next time it won't complain.

@Acedus Acedus force-pushed the tls-security-profile-propagation branch from f26f013 to 0c849e6 Compare March 5, 2026 11:06
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2026
@akalenyu
Copy link
Copy Markdown
Collaborator

akalenyu commented Mar 8, 2026

linter error is real fyi

@Acedus Acedus force-pushed the tls-security-profile-propagation branch from 0c849e6 to 45f6eea Compare March 8, 2026 15:35
@Acedus
metrics-server: inherit TLS options from CDI TLSSecurityProfile
865def3 
Previously the metrics-server would be initiated with default TLS
configuration. This change makes it so the TLS configuration is updated
during runtime for every request according to the CDI
TLSSecurityProfile.

Signed-off-by: Adi Aloni <[email protected]>
@Acedus Acedus force-pushed the tls-security-profile-propagation branch from 45f6eea to 865def3 Compare March 8, 2026 15:37
@akalenyu
Copy link
Copy Markdown
Collaborator

akalenyu commented Mar 8, 2026

/lgtm
/retest

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 8, 2026
@akalenyu
Copy link
Copy Markdown
Collaborator

akalenyu commented Mar 8, 2026

/retest
CI issues for both

@kubevirt-bot kubevirt-bot merged commit 2acfe66 into kubevirt:main Mar 8, 2026
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akalenyu akalenyu akalenyu left review comments

@awels awels awels approved these changes

@arnongilboa arnongilboa Awaiting requested review from arnongilboa

+2 more reviewers

@nunnatsa nunnatsa nunnatsa left review comments

@Barakmor1 Barakmor1 Barakmor1 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@awels awels

@akalenyu akalenyu

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Acedus @coveralls @kubevirt-bot @akalenyu @awels @nunnatsa @Barakmor1