Fix DataPlaneNodeSet tls verification #1057
Conversation
tests/functional/dataplane/openstackdataplanedeployment_controller_test.go
Outdated
Show resolved
Hide resolved
|
btw shouldn't this be called in a webhook so that the openstackdataplanenodeset can not be created/updated with a miss match to the ctlplane config? in general I think we could probably get rid of tlsEnabled in the nodeset and get it from the ctlplane, or default it to what the ctlplane is configured for. Or is there a use case to have it different? |
The dataplane connects vi the metallb loadbalancer k8s service to the pods exposed to them. If TLS is configured for those is controlled via the tls.podLevel configuraturation of the ctlplane. Right now the implementation checks for both tls.ingress and tls.podLevel configuration. With this it is not possible to deploy edpm nodes with tls.podLevel disabled. This change updates the verification to just check that spec.tlsEnabled of the DataPlaneNodeSet matches tls.podLevel of the ctlplane. Signed-off-by: Martin Schuppert <mschuppert@redhat.com>
stuggi
force-pushed
the
fix_dataplane_tlsmatch
branch
from
b8b0a72
to
6d2fe98
Compare
There was considerable pushback against that. Instead it was decided that the loop is the right place for this. |
There was a problem hiding this comment.
The dataplane connects vi the metallb loadbalancer k8s service to the pods exposed to them. If TLS is configured for those is controlled via the tls.podLevel configuraturation of the ctlplane.
I'm having hard time parsing this. Do you mean that load balancer connection to pods is conditioned on TLS being configured for them on the controlplane? And if so why is that a reason to disable this check?
Or is TLS being enabled on ingress a problem? And if so, why can't we just ensure that TLS on ingress is enabled, instead?
The dataplane do not care about tls on the ingress. they do not connect to the ingress/route. They connect via the loadbalancer service to the pod and tls.PodLevel is what controls if tls is enabled there. With the implementation of the current check you can not deploy with |
ok, its a different discussion I think, so can be done afterwards/somewhere else |
|
I'm going to leave decision on this to @vakwetu, since the check was implemented based on his specification. If he says it's fine to ignore the ingress than that's what we'll do. |
well its not if we can, right now the check introduced a regression where you can not deploy edpm nodes without tlse. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpodivin, stuggi, vakwetu 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit fb1dd00
into
openstack-k8s-operators:main
The dataplane connects vi the metallb loadbalancer k8s service to the pods exposed to them. If TLS is configured for those is controlled via the tls.podLevel configuraturation of the ctlplane.
Right now the implementation checks for both tls.ingress and tls.podLevel configuration. With this it is not possible to deploy edpm nodes with tls.podLevel disabled. This change updates the verification to just check that spec.tlsEnabled of the DataPlaneNodeSet matches tls.podLevel of the ctlplane.