Propagate TLSSecurityProfile to Kubevirt #2089

tiraboschi · 2022-09-20T14:07:05Z

Propagate TLSSecurityProfile to Kubevirt
translating the configuration to its internal
API.
Add unit tests.

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Release note:

Propagate TLSSecurityProfile to Kubevirt

tiraboschi · 2022-09-20T14:08:17Z

this temporanearly relies on untagged Kubevirt from main while we wait for v0.58.0,
avoid merging it.
/hold

coveralls · 2022-09-20T14:18:39Z

Pull Request Test Coverage Report for Build 3180091894

27 of 31 (87.1%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.01%) to 85.333%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/operands/kubevirt.go	27	31	87.1%

Totals
Change from base Build 3180046382:	0.01%
Covered Lines:	4608
Relevant Lines:	5400

💛 - Coveralls

tiraboschi · 2022-09-22T16:16:39Z

/rebase

tiraboschi · 2022-09-22T19:33:06Z

/retest

tiraboschi · 2022-09-23T07:24:41Z

/retest

tiraboschi · 2022-09-26T13:55:45Z

/retest

fossedihelm

Thanks. I know this is temporary, but in the meanwhile, I left two small comments. Thanks

controllers/hyperconverged/hyperconverged_controller_test.go

controllers/operands/kubevirt_test.go

fossedihelm

Thanks @tiraboschi! Looks good overall. I have a doubt about how HCO handles wrong tls configurations. Thanks

fossedihelm · 2022-09-28T07:41:07Z

controllers/operands/kubevirt.go

+	}
+
+	if profile.Custom != nil {
+		profileCiphers = profile.Custom.TLSProfileSpec.Ciphers


Is there a check that prevents to specify ciphers when using 1.3 as minTLSVersion?
1.3 ciphers will be skipped by crypto.OpenSSLToIANACipherSuites(profileCiphers) and this is ok;
but what happens if a user uses a wrong configuration? i.e. if I specify

minTLSVersion: VersionTLS13, ciphers: <one compatible tls1.2 cipher>

this will result in a reject from kv webhook.
More in general, the question is: how hco handle wrong custom configurations?
Thanks

HCO webhook is always going to try validating each change with all the components in dry-run mode, so each configuration change will be accepted only if all the components are going to accept it.
On the other side, if kv webhook will refuse the change, HCO webhook will refuse as a consequence.

Thanks for the clear explanation. In the light of that, it seems to me that everything is great! How about adding a test for this? WDYT? Or is it already tested in another place? Thanks

Not on this specific failure from kw webhook, but in general the mechanism is already tested in validator_test.go,
see: should return error if dry-run update of KV CR returns error

Sounds great! No other objection. Thanks

fossedihelm · 2022-09-28T08:45:18Z

@tiraboschi Feel free to ping me for a lgtm when the PR is ready. Thanks!

Propagate TLSSecurityProfile to Kubevirt translating the configuration to its internal API. Add unit tests. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

sonarcloud · 2022-10-04T07:07:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

tiraboschi · 2022-10-04T07:51:26Z

/unhold
@fossedihelm ping: we should be ready consuming Kubevirt v0.58.0-rc.0

hco-bot · 2022-10-04T08:59:21Z

hco-e2e-image-index-gcp lane succeeded.
/override ci/prow/hco-e2e-image-index-aws

kubevirt-bot · 2022-10-04T08:59:24Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-aws

In response to this:

hco-e2e-image-index-gcp lane succeeded.
/override ci/prow/hco-e2e-image-index-aws

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 kubernetes/test-infra repository.

tiraboschi · 2022-10-04T09:08:06Z

/retest

hco-bot · 2022-10-04T10:44:22Z

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

kubevirt-bot · 2022-10-04T10:44:25Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure

In response to this:

hco-e2e-kv-smoke-gcp lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-azure

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 kubernetes/test-infra repository.

hco-bot · 2022-10-04T11:00:08Z

hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

kubevirt-bot · 2022-10-04T11:00:12Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-sno-aws

In response to this:

hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

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 kubernetes/test-infra repository.

hco-bot · 2022-10-04T11:25:08Z

hco-e2e-upgrade-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-sno-aws

kubevirt-bot · 2022-10-04T11:25:13Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-sno-aws

In response to this:

hco-e2e-upgrade-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-sno-aws

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 kubernetes/test-infra repository.

tiraboschi · 2022-10-04T12:07:08Z

/override-bot

hco-bot · 2022-10-04T12:07:41Z

hco-e2e-upgrade-index-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-aws

kubevirt-bot · 2022-10-04T12:07:44Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-aws

In response to this:

hco-e2e-upgrade-index-azure lane succeeded.
/override ci/prow/hco-e2e-upgrade-index-aws

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 kubernetes/test-infra repository.

tiraboschi · 2022-10-04T12:08:11Z

/override-bot

tiraboschi · 2022-10-04T12:20:29Z

/retest

fossedihelm · 2022-10-04T12:36:57Z

Thanks
/lgtm

openshift-ci · 2022-10-04T14:03:29Z

@tiraboschi: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/okd-hco-e2e-image-index-gcp	2a45128058cb92ff5e4463d74b9a0595267ce0f1	link	true	`/test okd-hco-e2e-image-index-gcp`
ci/prow/okd-hco-e2e-upgrade-index-gcp	2a45128058cb92ff5e4463d74b9a0595267ce0f1	link	true	`/test okd-hco-e2e-upgrade-index-gcp`
ci/prow/okd-hco-e2e-image-index-aws	2a45128058cb92ff5e4463d74b9a0595267ce0f1	link	true	`/test okd-hco-e2e-image-index-aws`
ci/prow/hco-e2e-image-index-aws	`5c8e893`	link	true	`/test hco-e2e-image-index-aws`
ci/prow/hco-e2e-kv-smoke-azure	`5c8e893`	link	true	`/test hco-e2e-kv-smoke-azure`
ci/prow/hco-e2e-image-index-sno-aws	`5c8e893`	link	false	`/test hco-e2e-image-index-sno-aws`
ci/prow/hco-e2e-upgrade-index-sno-aws	`5c8e893`	link	false	`/test hco-e2e-upgrade-index-sno-aws`
ci/prow/hco-e2e-upgrade-index-aws	`5c8e893`	link	true	`/test hco-e2e-upgrade-index-aws`

Full PR test history. Your PR dashboard.

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 kubernetes/test-infra repository. I understand the commands that are listed here.

tiraboschi · 2022-10-04T15:22:36Z

/retest

tiraboschi · 2022-10-05T06:50:01Z

/approve

kubevirt-bot · 2022-10-05T06:50:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tiraboschi

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tiraboschi]

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

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. size/XL labels Sep 20, 2022

kubevirt-bot requested review from machadovilaca and sradco September 20, 2022 14:07

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2022

tiraboschi force-pushed the virt_crypto_policy branch 2 times, most recently from c9d4660 to aa0745e Compare September 21, 2022 15:17

hco-bot force-pushed the virt_crypto_policy branch from aa0745e to a457355 Compare September 22, 2022 16:19

fossedihelm reviewed Sep 27, 2022

View reviewed changes

controllers/hyperconverged/hyperconverged_controller_test.go Show resolved Hide resolved

controllers/operands/kubevirt_test.go Outdated Show resolved Hide resolved

tiraboschi force-pushed the virt_crypto_policy branch from a457355 to 2a45128 Compare September 27, 2022 15:15

tiraboschi requested a review from fossedihelm September 27, 2022 15:44

fossedihelm reviewed Sep 28, 2022

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2022

Propagate TLSSecurityProfile to Kubevirt

5c8e893

Propagate TLSSecurityProfile to Kubevirt translating the configuration to its internal API. Add unit tests. Signed-off-by: Simone Tiraboschi <stirabos@redhat.com>

tiraboschi force-pushed the virt_crypto_policy branch from 2a45128 to 5c8e893 Compare October 4, 2022 07:06

kubevirt-bot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL labels Oct 4, 2022

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2022

kubevirt-bot assigned fossedihelm Oct 4, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 5, 2022

kubevirt-bot merged commit 9046f3f into kubevirt:main Oct 5, 2022

Propagate TLSSecurityProfile to Kubevirt #2089

Propagate TLSSecurityProfile to Kubevirt #2089

Conversation

tiraboschi commented Sep 20, 2022

tiraboschi commented Sep 20, 2022

coveralls commented Sep 20, 2022 • edited Loading

Pull Request Test Coverage Report for Build 3180091894

💛 - Coveralls

tiraboschi commented Sep 22, 2022

tiraboschi commented Sep 22, 2022

tiraboschi commented Sep 23, 2022

tiraboschi commented Sep 26, 2022

fossedihelm left a comment

Choose a reason for hiding this comment

fossedihelm left a comment

Choose a reason for hiding this comment

fossedihelm Sep 28, 2022

Choose a reason for hiding this comment

tiraboschi Sep 28, 2022

Choose a reason for hiding this comment

fossedihelm Sep 28, 2022

Choose a reason for hiding this comment

tiraboschi Sep 28, 2022

Choose a reason for hiding this comment

fossedihelm Sep 28, 2022

Choose a reason for hiding this comment

fossedihelm commented Sep 28, 2022 • edited Loading

sonarcloud bot commented Oct 4, 2022

tiraboschi commented Oct 4, 2022

hco-bot commented Oct 4, 2022

kubevirt-bot commented Oct 4, 2022

tiraboschi commented Oct 4, 2022

hco-bot commented Oct 4, 2022

kubevirt-bot commented Oct 4, 2022

hco-bot commented Oct 4, 2022

kubevirt-bot commented Oct 4, 2022

hco-bot commented Oct 4, 2022

kubevirt-bot commented Oct 4, 2022

tiraboschi commented Oct 4, 2022

hco-bot commented Oct 4, 2022

kubevirt-bot commented Oct 4, 2022

tiraboschi commented Oct 4, 2022

tiraboschi commented Oct 4, 2022

fossedihelm commented Oct 4, 2022

openshift-ci bot commented Oct 4, 2022 • edited Loading

tiraboschi commented Oct 4, 2022

tiraboschi commented Oct 5, 2022

kubevirt-bot commented Oct 5, 2022

coveralls commented Sep 20, 2022 •

edited

Loading

fossedihelm commented Sep 28, 2022 •

edited

Loading

openshift-ci bot commented Oct 4, 2022 •

edited

Loading