Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework defaulting of secrets #211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Rework defaulting of secrets #211

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 107 additions & 105 deletions pkg/openstackbaremetalset/baremetalhost.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,57 +51,6 @@ func BaremetalHostProvision(
}
bmhStatus.IPAddresses["ctlplane"] = ctlPlaneIP
}
// Instance UserData/NetworkData overrides the default
userDataSecret := instance.Spec.BaremetalHosts[hostName].UserData
networkDataSecret := instance.Spec.BaremetalHosts[hostName].NetworkData

if userDataSecret == nil {
userDataSecret = instance.Spec.UserData
}

if networkDataSecret == nil {
networkDataSecret = instance.Spec.NetworkData
}

sts := []util.Template{}
// User data cloud-init secret
if userDataSecret == nil {
templateParameters := make(map[string]interface{})
templateParameters["AuthorizedKeys"] = strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n")
templateParameters["HostName"] = hostName
//If Hostname is fqdn, use it
if !hostNameIsFQDN(hostName) && instance.Spec.DomainName != "" {
templateParameters["FQDN"] = strings.Join([]string{hostName, instance.Spec.DomainName}, ".")
} else {
templateParameters["FQDN"] = hostName
}
templateParameters["CloudUserName"] = instance.Spec.CloudUserName

// Prepare cloudinit (create secret)
secretLabels := labels.GetLabels(instance, labels.GetGroupLabel(baremetalv1.ServiceName), map[string]string{})
if passwordSecret != nil && len(passwordSecret.Data["NodeRootPassword"]) > 0 {
templateParameters["NodeRootPassword"] = string(passwordSecret.Data["NodeRootPassword"])
}

userDataSecretName := fmt.Sprintf(CloudInitUserDataSecretName, instance.Name, hostName)

userDataSt := util.Template{
Name: userDataSecretName,
Namespace: instance.Namespace,
Type: util.TemplateTypeConfig,
InstanceType: instance.Kind,
AdditionalTemplate: map[string]string{"userData": "/openstackbaremetalset/cloudinit/userdata"},
Labels: secretLabels,
ConfigOptions: templateParameters,
}
sts = append(sts, userDataSt)
userDataSecret = &corev1.SecretReference{
Name: userDataSecretName,
Namespace: instance.Namespace,
}

}

//
// Provision the BaremetalHost
//
Expand All @@ -116,72 +65,125 @@ func BaremetalHostProvision(
preProvNetworkData = instance.Spec.BaremetalHosts[hostName].PreprovisioningNetworkDataName
}

if networkDataSecret == nil && preProvNetworkData == "" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sts := []util.Template{}

// Check IP version and set template variables accordingly
ipAddr, ipNet, err := net.ParseCIDR(ctlPlaneIP)
if err != nil {
// TODO: Remove this conversion once all usage sets ctlPlaneIP in CIDR format.
ipAddr = net.ParseIP(ctlPlaneIP)
if ipAddr == nil {
return err
}
// Instance UserData/NetworkData overrides the default
userDataSecret := instance.Spec.BaremetalHosts[hostName].UserData
networkDataSecret := instance.Spec.BaremetalHosts[hostName].NetworkData

var ipPrefix int
if ipAddr.To4() != nil {
ipPrefix, _ = net.IPMask(net.ParseIP(instance.Spec.CtlplaneNetmask).To4()).Size()
if userDataSecret == nil {
// User data cloud-init secret
if instance.Spec.UserData != nil {
userDataSecret = instance.Spec.UserData
} else {
templateParameters := make(map[string]interface{})
templateParameters["AuthorizedKeys"] = strings.TrimSuffix(string(sshSecret.Data["authorized_keys"]), "\n")
templateParameters["HostName"] = hostName
//If Hostname is fqdn, use it
if !hostNameIsFQDN(hostName) && instance.Spec.DomainName != "" {
templateParameters["FQDN"] = strings.Join([]string{hostName, instance.Spec.DomainName}, ".")
} else {
ipPrefix, _ = net.IPMask(net.ParseIP(instance.Spec.CtlplaneNetmask).To16()).Size()
templateParameters["FQDN"] = hostName
}
_, ipNet, err = net.ParseCIDR(fmt.Sprintf("%s/%d", ipAddr, ipPrefix))
if err != nil {
return err
templateParameters["CloudUserName"] = instance.Spec.CloudUserName

// Prepare cloudinit (create secret)
secretLabels := labels.GetLabels(instance, labels.GetGroupLabel(baremetalv1.ServiceName), map[string]string{})
if passwordSecret != nil && len(passwordSecret.Data["NodeRootPassword"]) > 0 {
templateParameters["NodeRootPassword"] = string(passwordSecret.Data["NodeRootPassword"])
}
}

CtlplaneIPVersion := "ipv6"
if ipAddr.To4() != nil {
CtlplaneIPVersion = "ipv4"
}
userDataSecretName := fmt.Sprintf(CloudInitUserDataSecretName, instance.Name, hostName)

templateParameters := make(map[string]interface{})
templateParameters["CtlplaneIpVersion"] = CtlplaneIPVersion
templateParameters["CtlplaneIp"] = ipAddr
templateParameters["CtlplaneInterface"] = instance.Spec.CtlplaneInterface
templateParameters["CtlplaneGateway"] = instance.Spec.CtlplaneGateway
templateParameters["CtlplaneNetmask"] = net.IP(ipNet.Mask)
if len(instance.Spec.BootstrapDNS) > 0 {
templateParameters["CtlplaneDns"] = instance.Spec.BootstrapDNS
} else {
templateParameters["CtlplaneDns"] = []string{}
userDataSt := util.Template{
Name: userDataSecretName,
Namespace: instance.Namespace,
Type: util.TemplateTypeConfig,
InstanceType: instance.Kind,
AdditionalTemplate: map[string]string{"userData": "/openstackbaremetalset/cloudinit/userdata"},
Labels: secretLabels,
ConfigOptions: templateParameters,
}
sts = append(sts, userDataSt)
userDataSecret = &corev1.SecretReference{
Name: userDataSecretName,
Namespace: instance.Namespace,
}
}
}

if len(instance.Spec.DNSSearchDomains) > 0 {
templateParameters["CtlplaneDnsSearch"] = instance.Spec.DNSSearchDomains
} else {
templateParameters["CtlplaneDnsSearch"] = []string{}
}
if networkDataSecret == nil {
if instance.Spec.NetworkData != nil {
networkDataSecret = instance.Spec.NetworkData
} else if preProvNetworkData == "" {

networkDataSecretName := fmt.Sprintf(CloudInitNetworkDataSecretName, instance.Name, hostName)
// Check IP version and set template variables accordingly
ipAddr, ipNet, err := net.ParseCIDR(ctlPlaneIP)
if err != nil {
// TODO: Remove this conversion once all usage sets ctlPlaneIP in CIDR format.
ipAddr = net.ParseIP(ctlPlaneIP)
if ipAddr == nil {
return err
}

var ipPrefix int
if ipAddr.To4() != nil {
ipPrefix, _ = net.IPMask(net.ParseIP(instance.Spec.CtlplaneNetmask).To4()).Size()
} else {
ipPrefix, _ = net.IPMask(net.ParseIP(instance.Spec.CtlplaneNetmask).To16()).Size()
}
_, ipNet, err = net.ParseCIDR(fmt.Sprintf("%s/%d", ipAddr, ipPrefix))
if err != nil {
return err
}
}

// Flag the network data secret as safe to collect with must-gather
secretLabelsWithMustGather := labels.GetLabels(instance, labels.GetGroupLabel(baremetalv1.ServiceName), map[string]string{
MustGatherSecret: "yes",
})
CtlplaneIPVersion := "ipv6"
if ipAddr.To4() != nil {
CtlplaneIPVersion = "ipv4"
}

networkDataSt := util.Template{
Name: networkDataSecretName,
Namespace: instance.Namespace,
Type: util.TemplateTypeConfig,
InstanceType: instance.Kind,
AdditionalTemplate: map[string]string{"networkData": "/openstackbaremetalset/cloudinit/networkdata"},
Labels: secretLabelsWithMustGather,
ConfigOptions: templateParameters,
}
sts = append(sts, networkDataSt)
networkDataSecret = &corev1.SecretReference{
Name: networkDataSecretName,
Namespace: instance.Namespace,
templateParameters := make(map[string]interface{})
templateParameters["CtlplaneIpVersion"] = CtlplaneIPVersion
templateParameters["CtlplaneIp"] = ipAddr
templateParameters["CtlplaneInterface"] = instance.Spec.CtlplaneInterface
templateParameters["CtlplaneGateway"] = instance.Spec.CtlplaneGateway
templateParameters["CtlplaneNetmask"] = net.IP(ipNet.Mask)
if len(instance.Spec.BootstrapDNS) > 0 {
templateParameters["CtlplaneDns"] = instance.Spec.BootstrapDNS
} else {
templateParameters["CtlplaneDns"] = []string{}
}

if len(instance.Spec.DNSSearchDomains) > 0 {
templateParameters["CtlplaneDnsSearch"] = instance.Spec.DNSSearchDomains
} else {
templateParameters["CtlplaneDnsSearch"] = []string{}
}

networkDataSecretName := fmt.Sprintf(CloudInitNetworkDataSecretName, instance.Name, hostName)

// Flag the network data secret as safe to collect with must-gather
secretLabelsWithMustGather := labels.GetLabels(instance, labels.GetGroupLabel(baremetalv1.ServiceName), map[string]string{
MustGatherSecret: "yes",
})

networkDataSt := util.Template{
Name: networkDataSecretName,
Namespace: instance.Namespace,
Type: util.TemplateTypeConfig,
InstanceType: instance.Kind,
AdditionalTemplate: map[string]string{"networkData": "/openstackbaremetalset/cloudinit/networkdata"},
Labels: secretLabelsWithMustGather,
ConfigOptions: templateParameters,
}
sts = append(sts, networkDataSt)
networkDataSecret = &corev1.SecretReference{
Name: networkDataSecretName,
Namespace: instance.Namespace,
}
} else {
l.Info("can not generate networkData secret when preProvNetworkData is already defined")
}
}

Expand Down