CNV-33847: KubeVirt: create the etcd encryption key secret, if missing #3148

nunnatsa · 2023-10-30T16:59:50Z

What this PR does / why we need it:
To allow creating of KubeVirt hosted cluster using the hosted cluster API (rather than using the cli).

When creating the hosted cluster using the cli, the cli also creates the secret. But when creating the hosted cluster using the hosted cluster API, the secret is not created.

This PR changes hypershift so it now creates the etcd encryption key secret, if it is not already exist.

Which issue(s) this PR fixes:
Fixes #CNV-33847

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

nunnatsa · 2023-10-31T10:00:17Z

/test e2e-aws
/test e2e-kubevirt-aws-ovn

davidvossel · 2023-10-31T14:19:30Z

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

+	// make sure there is an encryption key secret for etcd
+	if hc.Spec.SecretEncryption == nil {
+		etcdEnc, err := getOrCreateETCDEncKey(ctx, r.Client, hc)
+		if err != nil {
+			return err
+		}
+
+		hc.Spec.SecretEncryption = &hyperv1.SecretEncryptionSpec{
+			Type: hyperv1.AESCBC,
+			AESCBC: &hyperv1.AESCBCSpec{
+				ActiveKey: corev1.LocalObjectReference{
+					Name: etcdEnc.Name,
+				},
+			},
+		}
+	} else if hc.Spec.SecretEncryption.Type == hyperv1.AESCBC {
+		etcdEnc, err := getOrCreateETCDEncKey(ctx, r.Client, hc)
+
+		if err != nil {
+			return err
+		}
+
+		if hc.Spec.SecretEncryption.AESCBC != nil {
+			hc.Spec.SecretEncryption.AESCBC = &hyperv1.AESCBCSpec{}
+		}
+
+		if len(hc.Spec.SecretEncryption.AESCBC.ActiveKey.Name) == 0 {
+			hc.Spec.SecretEncryption.AESCBC.ActiveKey.Name = etcdEnc.Name
+		}
+	} else {
+		log.Error(fmt.Errorf("wrong encryption key type %s", hc.Spec.SecretEncryption.Type), "")
+	}
+


I think much of this logic could be simplified using the createOrUpdate function. Below is a skeleton of how that might look along with some suggestions about things we should look out for when generating the secret

if hc.Spec.SecretEncryption == nil || hc.Spec.SecretEncryption.Type == "" || (hc.Spec.SecretEncryption.Type == hyperv1.AESCBC && hc.Spec.SecretEncryption.AESCBC == nil) { ### Maybe add this function to hypershift-operator/controllers/manifests/manifests.go to be consistent with how other objects are created for hosted cluster? secret := manifests.GeneratedAESCBCSecret(hc.Namespace, hc.Name) ### use createOrUpdate function to handle the case where the secret was already created but we failed to apply the hostedCluster.spec change the first go around> if _, err := createOrUpdate(ctx, r.Client, secret, func() error { ### don't override existing key just in case something weird happened > _, exists := secret.Data[hyperv1.AESCBCKeySecretKey] if exists { return nil } ### If we generated the secret, then the secret needs to be cleaned up automatically when the HostedCluster is deleted> ownerRef := config.OwnerRefFrom(hc) ownerRef.ApplyTo(secret) ### generate key and add to the secret's data return nil }); err != nil { return fmt.Errorf("failed to generate ETCD SecretEncryption key for KubeVirt platform HostedCluster: %w", err) } ### apply new secret reference to hosted cluster }

if hc.Spec.SecretEncryption == nil || hc.Spec.SecretEncryption.Type == "" || (hc.Spec.SecretEncryption.Type == hyperv1.AESCBC && hc.Spec.SecretEncryption.AESCBC == nil) { ### Maybe add this function to hypershift-operator/controllers/manifests/manifests.go to be consistent with how other objects are created for hosted cluster? secret := manifests.GeneratedAESCBCSecret(hc.Namespace, hc.Name) // <<<< this is in a new etcd package, to be also used from api/fixtures. No code in api/fixtures uses manifests ### use createOrUpdate function to handle the case where the secret was already created but we failed to apply the hostedCluster.spec change the first go around> if _, err := createOrUpdate(ctx, r.Client, secret, func() error { ### don't override existing key just in case something weird happened > _, exists := secret.Data[hyperv1.AESCBCKeySecretKey] // not sure about this code. This is just created and we know the key was generated if exists { return nil } ### If we generated the secret, then the secret needs to be cleaned up automatically when the HostedCluster is deleted> ownerRef := config.OwnerRefFrom(hc) ownerRef.ApplyTo(secret) ### generate key and add to the secret's data return nil }); err != nil { return fmt.Errorf("failed to generate ETCD SecretEncryption key for KubeVirt platform HostedCluster: %w", err) } ### apply new secret reference to hosted cluster }

openshift-ci · 2023-10-31T15:15:39Z

@nunnatsa: 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/e2e-ibmcloud-roks	`180c1da`	link	false	`/test e2e-ibmcloud-roks`
ci/prow/e2e-ibmcloud-iks	`180c1da`	link	false	`/test e2e-ibmcloud-iks`

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.

davidvossel

nice! I just had one really minor comment about that log object. other than that, this looks really good

davidvossel · 2023-11-01T20:54:25Z

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -4574,7 +4577,7 @@ func (r *HostedClusterReconciler) serviceAccountSigningKeyBytes(ctx context.Cont
 	return privateKeyPEMBytes, publicKeyPEMBytes, nil
 }

-func (r *HostedClusterReconciler) reconcileKubevirtPlatformDefaultSettings(ctx context.Context, hc *hyperv1.HostedCluster) error {
+func (r *HostedClusterReconciler) reconcileKubevirtPlatformDefaultSettings(ctx context.Context, hc *hyperv1.HostedCluster, createOrUpdate upsert.CreateOrUpdateFN, log logr.Logger) error {


is the log used?

also, you can get the logger from the ctx i believe

log := ctrl.LoggerFrom(ctx)

To allow creating of KubeVirt hosted cluster using the hosted cluster API (rather than using the cli). When creating the hosted cluster using the cli, the cli also creates the secret. But when creating the hosted cluster using the hosted cluster API, the secret is not created. This PR changes hypershift so it now creates the etcd encryption key secret, if it is not already exist. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

davidvossel

/lgtm
/approve

openshift-ci · 2023-11-03T21:21:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel, nunnatsa

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~~ [davidvossel]

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

openshift-ci-robot · 2023-11-04T05:15:27Z

@nunnatsa: This pull request references CNV-33847 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

In response to this:

What this PR does / why we need it:
To allow creating of KubeVirt hosted cluster using the hosted cluster API (rather than using the cli).

When creating the hosted cluster using the cli, the cli also creates the secret. But when creating the hosted cluster using the hosted cluster API, the secret is not created.

This PR changes hypershift so it now creates the etcd encryption key secret, if it is not already exist.

Which issue(s) this PR fixes:
Fixes #CNV-33847

Checklist

Subject and description added to both, commit and PR.

Relevant issues have been referenced.

This change includes docs.

This change includes unit tests.

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.

openshift-ci bot added the do-not-merge/needs-area label Oct 30, 2023

openshift-ci bot requested review from enxebre and isco-rodriguez October 30, 2023 17:02

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Oct 30, 2023

nunnatsa force-pushed the CNV-33847-etcd-enc-key branch from 4cd9993 to 7a8af5d Compare October 31, 2023 06:45

nunnatsa force-pushed the CNV-33847-etcd-enc-key branch from 7a8af5d to 180c1da Compare October 31, 2023 12:50

davidvossel reviewed Oct 31, 2023

View reviewed changes

nunnatsa force-pushed the CNV-33847-etcd-enc-key branch from 180c1da to c8a0c70 Compare November 1, 2023 10:41

davidvossel reviewed Nov 1, 2023

View reviewed changes

nunnatsa force-pushed the CNV-33847-etcd-enc-key branch from c8a0c70 to e05a228 Compare November 2, 2023 07:59

davidvossel reviewed Nov 3, 2023

View reviewed changes

openshift-ci bot assigned davidvossel Nov 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2023

nunnatsa changed the title ~~CNV-33847 - KubeVirt: create the etcd encryption key secret, if missing~~ CNV-33847: KubeVirt: create the etcd encryption key secret, if missing Nov 4, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 4, 2023

openshift-ci bot merged commit ef4dfe9 into openshift:main Nov 4, 2023
12 checks passed

nunnatsa deleted the CNV-33847-etcd-enc-key branch November 4, 2023 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNV-33847: KubeVirt: create the etcd encryption key secret, if missing #3148

CNV-33847: KubeVirt: create the etcd encryption key secret, if missing #3148

nunnatsa commented Oct 30, 2023 •

edited by openshift-ci bot

nunnatsa commented Oct 31, 2023

davidvossel Oct 31, 2023

nunnatsa Oct 31, 2023

openshift-ci bot commented Oct 31, 2023 •

edited

davidvossel left a comment

davidvossel Nov 1, 2023

davidvossel left a comment

openshift-ci bot commented Nov 3, 2023

openshift-ci-robot commented Nov 4, 2023 •

edited by openshift-ci bot

CNV-33847: KubeVirt: create the etcd encryption key secret, if missing #3148

CNV-33847: KubeVirt: create the etcd encryption key secret, if missing #3148

Conversation

nunnatsa commented Oct 30, 2023 • edited by openshift-ci bot

nunnatsa commented Oct 31, 2023

davidvossel Oct 31, 2023

Choose a reason for hiding this comment

nunnatsa Oct 31, 2023

Choose a reason for hiding this comment

openshift-ci bot commented Oct 31, 2023 • edited

davidvossel left a comment

Choose a reason for hiding this comment

davidvossel Nov 1, 2023

Choose a reason for hiding this comment

davidvossel left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 3, 2023

openshift-ci-robot commented Nov 4, 2023 • edited by openshift-ci bot

nunnatsa commented Oct 30, 2023 •

edited by openshift-ci bot

openshift-ci bot commented Oct 31, 2023 •

edited

openshift-ci-robot commented Nov 4, 2023 •

edited by openshift-ci bot