[tls] nova novncproxy to support vencrypt #748

stuggi · 2024-04-26T09:53:14Z

Restructures the tls section of the novncproxy API to support passing a secret name which holds the cert/key to be used for vencrypt. The CA to validate the vnc server is part of the global bundle.

Jira: OSPRH-6552

Restructures the tls section of the novncproxy API to support passing a secret name which holds the cert/key to be used for vencrypt. The CA to validate the vnc server is part of the global bundle. Jira: OSPRH-6552

Issues a certificate for nova novncproxy if PodLevel tls is enabled and configures it as vencrypt secret on the novnc proxy. Depends-On: openstack-k8s-operators/nova-operator#748 Depends-On: openstack-k8s-operators/dataplane-operator#862 JIRA: OSPRH-6552

Issues a certificate for nova novncproxy if PodLevel tls is enabled and configures it as vencrypt secret on the novnc proxy. Depends-On: openstack-k8s-operators#779 Depends-On: openstack-k8s-operators/nova-operator#748 Depends-On: openstack-k8s-operators/dataplane-operator#862 JIRA: OSPRH-6552

gibizer

Looks good to me. Thanks for adding the env test coverage too.

gibizer · 2024-04-26T10:06:13Z

templates/nova.conf

@@ -139,6 +139,12 @@ notify_on_state_change = vm_and_task_state
 enabled = True
 novncproxy_host = "::0"
 novncproxy_port = 6080
+{{if (index . "VencryptClientKey") }}
+auth_schemes=vencrypt,none


so "none" here allows the proxy to fall back to not encrypted connection if the qemu side of the connection does not support encryption. I'm wondering if we need to allow this.

First I thought that in greendfield we don't need this as either TLS is enabled in the whole deployment or not. But I guess we need to support deploying without TLS, booting instances, and then enabling TLS in the deployment. At that point the existing qemu instances will not support TLS. So we need the fallback.

yes correct thats the use case we'd need this for. and since the computes config is controlling if the vencrypt is used I think its not a big issue.

yepp fine by me.

for what its worth i believe that we are not currently planning to support enabling tls after the fact as a documented procedure but I'm fine with making the code capable of that so that we can actually do it when a customer or PM asks.

i was wondering the same as gibi but I'm fine with that answer.

We already got that request. E.g. for adoption if someone has not tlse on the current env, it should stay the same, but possible to be enabled as a day 2 thing after adoption. the procedure is on our list to be tested/document.

softwarefactory-project-zuul · 2024-04-26T10:08:29Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/598eea0b7aaf4b49868ef7309df5bd27

❌ openstack-k8s-operators-content-provider FAILURE in 13m 15s
⚠️ nova-operator-kuttl SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ nova-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ nova-operator-tempest-multinode-ceph SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

Issues a certificate for nova novncproxy if PodLevel tls is enabled and configures it as vencrypt secret on the novnc proxy. Depends-On: openstack-k8s-operators#779 Depends-On: openstack-k8s-operators/nova-operator#748 Depends-On: openstack-k8s-operators/dataplane-operator#862 JIRA: OSPRH-6552

gibizer · 2024-04-26T10:11:59Z

build failure seems to be due to non backward compatible change in the CRD

    go vet ./...
    # github.com/openstack-k8s-operators/openstack-operator/pkg/openstack
    pkg/openstack/nova.go:243:64: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    pkg/openstack/nova.go:257:48: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    make: *** [Makefile:113: vet] Error 1
  stdout_lines: <omitted>

stuggi · 2024-04-26T10:24:08Z

build failure seems to be due to non backward compatible change in the CRD

    go vet ./...
    # github.com/openstack-k8s-operators/openstack-operator/pkg/openstack
    pkg/openstack/nova.go:243:64: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    pkg/openstack/nova.go:257:48: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    make: *** [Makefile:113: vet] Error 1
  stdout_lines: <omitted>

yes it is a breaking change. what I could do is

keep the old param,
land this PR
get the openstack-operator patch landed to use the new params
update nova to remove the old
update openstack-operator

gibizer · 2024-04-26T10:33:02Z

build failure seems to be due to non backward compatible change in the CRD
    go vet ./...
    # github.com/openstack-k8s-operators/openstack-operator/pkg/openstack
    pkg/openstack/nova.go:243:64: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    pkg/openstack/nova.go:257:48: cellTemplate.NoVNCProxyServiceTemplate.TLS.SecretName undefined (type "github.com/openstack-k8s-operators/nova-operator/api/v1beta1".TLSSection has no field or method SecretName)
    make: *** [Makefile:113: vet] Error 1
  stdout_lines: <omitted>
yes it is a breaking change. what I could do is

keep the old param,

land this PR

get the openstack-operator patch landed to use the new params

update nova to remove the old

update openstack-operator

Yeah that is the way forward I guess. Unfortunately the tempest coverage on openstack-operator does not include the vnc tests so we cannot use the test result there to force merge this PR.

softwarefactory-project-zuul · 2024-04-26T10:46:47Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/926df67f1d9b4ab991932521bb69d253

❌ openstack-k8s-operators-content-provider FAILURE in 13m 30s
⚠️ nova-operator-kuttl SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ nova-operator-tempest-multinode SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider
⚠️ nova-operator-tempest-multinode-ceph SKIPPED Skipped due to failed job openstack-k8s-operators-content-provider

will be removed/reverted in a follow up.

SeanMooney

this looks good to me too
just one thing to note is this might conflict with the work kamil is doing on observed generations but there is no way to really avoid that.

SeanMooney · 2024-04-26T12:03:01Z

templates/nova.conf

@@ -139,6 +139,12 @@ notify_on_state_change = vm_and_task_state
 enabled = True
 novncproxy_host = "::0"
 novncproxy_port = 6080
+{{if (index . "VencryptClientKey") }}
+auth_schemes=vencrypt,none


for what its worth i believe that we are not currently planning to support enabling tls after the fact as a documented procedure but I'm fine with making the code capable of that so that we can actually do it when a customer or PM asks.

i was wondering the same as gibi but I'm fine with that answer.

gibizer

thanks for doing the extra dance with the deprecation

openshift-ci · 2024-04-26T12:21:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, SeanMooney, stuggi

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~~ [SeanMooney,gibizer,stuggi]

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

stuggi · 2024-04-26T13:56:58Z

Note, tempest vnc tests will fail. that is expected with the current openstack-operator version. we should override it. basically with moving the params, but still have the deprecated one, the openstack-operator expects that from the route -> vnc proxy it is possible to use tls while it isn't right now.

stuggi · 2024-04-26T13:59:11Z

Note, tempest vnc tests will fail. that is expected with the current openstack-operator version. we should override it. basically with moving the params, but still have the deprecated one, the openstack-operator expects that from the route -> vnc proxy it is possible to use tls while it isn't right now.

or we temporary disable tempest.api.compute.servers.test_novnc.NoVNCConsoleTestJSON if you prefer

gibizer · 2024-04-26T14:13:31Z

Note, tempest vnc tests will fail. that is expected with the current openstack-operator version. we should override it. basically with moving the params, but still have the deprecated one, the openstack-operator expects that from the route -> vnc proxy it is possible to use tls while it isn't right now.

or we temporary disable tempest.api.compute.servers.test_novnc.NoVNCConsoleTestJSON if you prefer

I will override the job if this test fail. I will expect a clean result in the follow up where the deprecated field is dropped.

softwarefactory-project-zuul · 2024-04-26T14:21:41Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/5dcf4df7ffe4465a8ac634d3ba09f106

✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 02m 47s
✔️ nova-operator-kuttl SUCCESS in 40m 53s
❌ nova-operator-tempest-multinode FAILURE in 2h 00m 02s
❌ nova-operator-tempest-multinode-ceph FAILURE in 2h 30m 21s

stuggi · 2024-04-26T14:23:19Z

Note, tempest vnc tests will fail. that is expected with the current openstack-operator version. we should override it. basically with moving the params, but still have the deprecated one, the openstack-operator expects that from the route -> vnc proxy it is possible to use tls while it isn't right now.

or we temporary disable tempest.api.compute.servers.test_novnc.NoVNCConsoleTestJSON if you prefer

I will override the job if this test fail. I will expect a clean result in the follow up where the deprecated field is dropped.

yes those failed https://logserver.rdoproject.org/48/748/b06b9bcdc8c76faa85bb8cc643a699babde9f41f/github-check/nova-operator-tempest-multinode-ceph/1e623d3/controller/ci-framework-data/tests/test_operator/tempest-tests/stestr_results.html

gibizer · 2024-04-26T14:25:52Z

The rest of the tempest is green so I'm merging this.

Issues a certificate for nova novncproxy if PodLevel tls is enabled and configures it as vencrypt secret on the novnc proxy. Depends-On: openstack-k8s-operators#779 Depends-On: openstack-k8s-operators/nova-operator#748 Depends-On: openstack-k8s-operators/dataplane-operator#862 JIRA: OSPRH-6552

[tls] nova novncproxy to support vencrypt

f4cb777

Restructures the tls section of the novncproxy API to support passing a secret name which holds the cert/key to be used for vencrypt. The CA to validate the vnc server is part of the global bundle. Jira: OSPRH-6552

openshift-ci bot requested review from dprince and jamepark4 April 26, 2024 09:53

openshift-ci bot added the approved label Apr 26, 2024

stuggi requested a review from olliewalsh April 26, 2024 09:55

gibizer reviewed Apr 26, 2024

View reviewed changes

stuggi mentioned this pull request Apr 26, 2024

[tls] nova novncproxy vencrypt support openstack-k8s-operators/openstack-operator#780

Merged

[tls] deprecate tls.secretName for novncproxy

b06b9bc

will be removed/reverted in a follow up.

stuggi force-pushed the vncproxy_client_cert branch from 3a957b9 to b06b9bc Compare April 26, 2024 10:56

SeanMooney approved these changes Apr 26, 2024

View reviewed changes

openshift-ci bot assigned SeanMooney Apr 26, 2024

openshift-ci bot added the lgtm label Apr 26, 2024

gibizer approved these changes Apr 26, 2024

View reviewed changes

openshift-ci bot assigned gibizer Apr 26, 2024

gibizer merged commit 58dd2ab into openstack-k8s-operators:main Apr 26, 2024
5 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tls] nova novncproxy to support vencrypt #748

[tls] nova novncproxy to support vencrypt #748

stuggi commented Apr 26, 2024 •

edited by openshift-ci bot

Loading

gibizer left a comment

gibizer Apr 26, 2024

stuggi Apr 26, 2024

gibizer Apr 26, 2024

SeanMooney Apr 26, 2024

stuggi Apr 26, 2024

softwarefactory-project-zuul bot commented Apr 26, 2024

gibizer commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

softwarefactory-project-zuul bot commented Apr 26, 2024

SeanMooney left a comment

SeanMooney Apr 26, 2024

gibizer left a comment

openshift-ci bot commented Apr 26, 2024

stuggi commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

softwarefactory-project-zuul bot commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

[tls] nova novncproxy to support vencrypt #748

[tls] nova novncproxy to support vencrypt #748

Conversation

stuggi commented Apr 26, 2024 • edited by openshift-ci bot Loading

gibizer left a comment

Choose a reason for hiding this comment

gibizer Apr 26, 2024

Choose a reason for hiding this comment

stuggi Apr 26, 2024

Choose a reason for hiding this comment

gibizer Apr 26, 2024

Choose a reason for hiding this comment

SeanMooney Apr 26, 2024

Choose a reason for hiding this comment

stuggi Apr 26, 2024

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Apr 26, 2024

gibizer commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

softwarefactory-project-zuul bot commented Apr 26, 2024

SeanMooney left a comment

Choose a reason for hiding this comment

SeanMooney Apr 26, 2024

Choose a reason for hiding this comment

gibizer left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 26, 2024

stuggi commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

softwarefactory-project-zuul bot commented Apr 26, 2024

stuggi commented Apr 26, 2024

gibizer commented Apr 26, 2024

stuggi commented Apr 26, 2024 •

edited by openshift-ci bot

Loading