Rework defaulting of secrets

[jpodivin]

Due to the nature of changes made, the diff looks considerably larger than it should be. Please don't be alarmed.

Since we are still discussing how should we deal with asymmetric routing, I've opted to essentially keep the existing behavior. However, the conditionals for setting secrets have been reorganized to flow more logically, simplifying future maintenance.

Also there is log message added, to make a note a special case when secrets aren't generated.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jpodivin
Once this PR has been reviewed and has the lgtm label, please assign viroel for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[abays]

Do we still have this logic somehow represented in the reworked version? If I understand correctly, we don't want to create a networkDataSecret if a preProvNetworkData secret has been set for the host [1]. @rabi can you keep me honest here? :)

[1]

openstack-baremetal-operator/pkg/openstackbaremetalset/baremetalhost.go

Lines 186 to 189 in 615a9be

	preProvNetworkData := foundBaremetalHost.Spec.PreprovisioningNetworkDataName
	if preProvNetworkData == "" {
	preProvNetworkData = instance.Spec.BaremetalHosts[hostName].PreprovisioningNetworkDataName
	}

[jpodivin]

I've dropped that because nobody could really tell me why. If there is a good reason for it, the conditional can get back in. Along with error to be raised in case it fails obviously.

[hjensas]

We discussed this a bit on Slack last friday, Rabi also opened a Jira to discuss.

OCP's capabilities to configure the provisioning network is very limited, it does not support/cannot be configured to use the provisioning network when the nodes are not connected L2 to the provisioning network. OCP's answer to this is to use the "External" network instead. Since we have the RHOSO isolated networks configured on the OCP nodes, we quickly end up with asymmetric routing to consider.

OCP nodes:

• external (10.0.0.0/16)
• RHOSO ctlplane (172.16.0.0/24)
  :: Routes to DCN/Spine-and-Leaf ctlplane's (172.16.1.0/24, 172.16.2.0/24 ...) via RHOSO ctlplane interface.

Baremetals / EDPMs:

• RHOSO ctlplane (172.16.1.0/24) <-- Note - different IP range

EDPM boots and connects to Ironic (Metal3) on external (10.0.0.0/16) - but return traffic is out RHOSO ctlplane i.e a different interface used for return traffic. In this case the rp_filter on OCP nodes drop the traffic ...

One way to get around the asymmetric routing would be to use different network configuration in preProvisioning - i.e the ramdisk uses 192.168.25.0/24, 192.168.25.0/24 - a range that OCP nodes use the default route for, to ensure incoming and return traffic is all on the OCP "External" interface.

I imagined that we would have customers configure the PreprovisioningNetworkDataName for their BMO's and we would still set NetworkData via template. (What @jpodivin is doing in this patch if I get it right?).

However Rabi brought the feedback that the field asked for preprovisioningNetworkData to propagate to networkData - i.e not using template. Because that is what OCP/Metal3 does. I think this argument is valid, it will be confusing for users with experience using OCP Metal3 if we behave differently.

I reconsidered my position once I got Rabi (and the fields) arguments. We should keep behavior aligned with OCP/Metal3.

[jpodivin]

There still seems to be some discussion to be had about the desire behavior. So for now the behavior will remain the same. I've restored conditional on PreprovisioningNetworkDataName. Now the PR is purely housekeeping effort.

When there is a decision on what should take primacy, it's just a question of minor adjustment.

[abays]

I built this and deployed it locally to try a few OpenStackBaremetalSet CRs. The various Secrets seemed to work as expected.

[rabi]

Tbh, I'm not sure what this change is trying to achieve, make the code more readable? I see blocks of code moved around without lot of value add. I'll leave it to the better judgement of the other reviewers though.

[hjensas]

I did some testing, and discussed with Rabi today. It turns out baremetal-operator, docs and how it operates depending on use of image customization being available or not very clear. With OCP it is using image customization and IPA is coreos - hence preprovisioningNetworkDataName must be nmstate not openstack (network_data.json) format. A bit more detail in Jira: OSPRH-10442.

[openshift-merge-robot]

PR needs rebase.

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-sigs/prow repository.