Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
86 changes: 84 additions & 2 deletions pkg/asset/agent/agentconfig/agenthosts.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (

"github.com/pkg/errors"
"github.com/sirupsen/logrus"
goyaml "gopkg.in/yaml.v2"
"k8s.io/apimachinery/pkg/util/validation/field"
"sigs.k8s.io/yaml"

Expand All @@ -16,6 +17,7 @@ import (
agentAsset "github.com/openshift/installer/pkg/asset/agent"
"github.com/openshift/installer/pkg/asset/agent/joiner"
"github.com/openshift/installer/pkg/asset/agent/workflow"
"github.com/openshift/installer/pkg/types"
"github.com/openshift/installer/pkg/types/agent"
"github.com/openshift/installer/pkg/types/baremetal/validation"
"github.com/openshift/installer/pkg/validate"
Expand All @@ -41,8 +43,9 @@ type nmStateInterface struct {
// AgentHosts generates the hosts information from the AgentConfig and
// OptionalInstallConfig assets.
type AgentHosts struct {
Hosts []agent.Host
rendezvousIP string
Hosts []agent.Host
FencingCredentialsHost []types.Credential
rendezvousIP string
}

// Name returns a human friendly name.
Expand Down Expand Up @@ -93,6 +96,9 @@ func (a *AgentHosts) Generate(_ context.Context, dependencies asset.Parents) err
}
}

// store per host fencing-credentials only when MAC address is used
a.populateFencingCredentialHosts(installConfig)

case workflow.AgentWorkflowTypeAddNodes:
a.Hosts = append(a.Hosts, addNodesConfig.Config.Hosts...)
for i, host := range a.Hosts {
Expand Down Expand Up @@ -367,5 +373,81 @@ func (a *AgentHosts) HostConfigFiles() (HostConfigFileMap, error) {
files[filepath.Join(name, "role")] = []byte(host.Role)
}
}

credsByDir := map[string][]*types.Credential{}
var dirOrder []string

for i := range a.FencingCredentialsHost {
cred := &a.FencingCredentialsHost[i]
dirName := findHostDirForMAC(files, cred.MACAddress)
if dirName == "" {
dirName = nextAvailableHostDir(files, len(a.Hosts))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

non-blocking comment: I think there could be some space for further simplification here - unless I missed something (even though the current code seems to work), by applying the following approach:

  • Initalize a a counter maxNewDirs equals to len(Hosts)
  • if dirName is empty, simply set the new dirName to host-<maxNewDirs+1> and increment maxNewDirs, and use it for adding the mac_addresses
  • Marshal the FC struct and add it to files directly in this loop

That approach may allow to user a simpler logic

files[filepath.Join(dirName, "mac_addresses")] = []byte(strings.ToLower(cred.MACAddress) + "\n")
}
if _, ok := credsByDir[dirName]; !ok {
dirOrder = append(dirOrder, dirName)
}
credsByDir[dirName] = append(credsByDir[dirName], cred)
}

for _, dir := range dirOrder {
cfg := &FencingCredentialsConfig{Credentials: credsByDir[dir]}
data, err := goyaml.Marshal(cfg)
if err != nil {
return nil, err
}
files[filepath.Join(dir, "fencing-credentials.yaml")] = data
}

return files, nil
}

func (a *AgentHosts) populateFencingCredentialHosts(installConfig *agentAsset.OptionalInstallConfig) {
if installConfig.Config == nil || installConfig.Config.ControlPlane == nil ||
installConfig.Config.ControlPlane.Fencing == nil {
return
}

for _, cred := range installConfig.Config.ControlPlane.Fencing.Credentials {
if cred.HostName == "" && cred.MACAddress != "" {
a.FencingCredentialsHost = append(a.FencingCredentialsHost, *cred)
}
}
}

func findHostDirForMAC(files HostConfigFileMap, macAddress string) string {
normalizedMAC := strings.ToLower(macAddress)
for key, content := range files {
if !strings.HasSuffix(key, "/mac_addresses") {
continue
}
dirName := strings.TrimSuffix(key, "/mac_addresses")
for _, mac := range strings.Split(strings.TrimSpace(string(content)), "\n") {
if strings.TrimSpace(mac) == normalizedMAC {
return dirName
}
}
}
return ""
}

func nextAvailableHostDir(files HostConfigFileMap, hostCount int) string {
existing := map[string]bool{}
for key := range files {
parts := strings.SplitN(key, "/", 2)
if len(parts) > 0 {
existing[parts[0]] = true
}
}
// Reserve host-<i> for every host slot, even those that emitted no
// files, so a MAC-only fencing credential never reuses that name.
for i := 0; i < hostCount; i++ {
existing[fmt.Sprintf("host-%d", i)] = true
}
for i := 0; ; i++ {
name := fmt.Sprintf("host-%d", i)
if !existing[name] {
return name
}
}
}
210 changes: 210 additions & 0 deletions pkg/asset/agent/agentconfig/agenthosts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -955,6 +955,216 @@ func (ib *InterfacetBuilder) build() *aiv1beta1.Interface {
return &ib.Interface
}

func TestAgentHosts_FencingCredentialsHost(t *testing.T) {
cases := []struct {
name string
dependencies []asset.Asset
expectedCredentials []types.Credential
}{
{
name: "mac-keyed credentials are collected",
dependencies: []asset.Asset{
&workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall},
&joiner.AddNodesConfig{},
getInstallConfigWithFencing([]*types.Credential{
{MACAddress: "28:d2:44:d2:b2:1a", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
}),
getAgentConfigMultiHost("worker"),
},
expectedCredentials: []types.Credential{
{MACAddress: "28:d2:44:d2:b2:1a", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
},
{
name: "hostname-keyed credentials are excluded",
dependencies: []asset.Asset{
&workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall},
&joiner.AddNodesConfig{},
getInstallConfigWithFencing([]*types.Credential{
{HostName: "master-0", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
}),
getAgentConfigMultiHost("worker"),
},
expectedCredentials: nil,
},
{
name: "mixed credentials only collect mac-keyed",
dependencies: []asset.Asset{
&workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall},
&joiner.AddNodesConfig{},
getInstallConfigWithFencing([]*types.Credential{
{HostName: "master-0", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
{MACAddress: "28:d2:44:d2:b2:1b", Username: "admin2", Password: "pass2", Address: "redfish+https://10.0.0.2/redfish/v1/Systems/1"},
}),
getAgentConfigMultiHost("worker"),
},
expectedCredentials: []types.Credential{
{MACAddress: "28:d2:44:d2:b2:1b", Username: "admin2", Password: "pass2", Address: "redfish+https://10.0.0.2/redfish/v1/Systems/1"},
},
},
{
name: "credential with both hostname and mac is treated as hostname-keyed",
dependencies: []asset.Asset{
&workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall},
&joiner.AddNodesConfig{},
getInstallConfigWithFencing([]*types.Credential{
{HostName: "master-0", MACAddress: "28:d2:44:d2:b2:1a", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
}),
getAgentConfigMultiHost("worker"),
},
expectedCredentials: nil,
},
{
name: "no fencing config produces nil",
dependencies: []asset.Asset{
&workflow.AgentWorkflow{Workflow: workflow.AgentWorkflowTypeInstall},
&joiner.AddNodesConfig{},
getInstallConfigSingleHost(),
getAgentConfigSingleHost(),
},
expectedCredentials: nil,
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
parents := asset.Parents{}
parents.Add(tc.dependencies...)

ah := &AgentHosts{}
err := ah.Generate(context.Background(), parents)
assert.NoError(t, err)
assert.Equal(t, tc.expectedCredentials, ah.FencingCredentialsHost)
})
}
}

func TestHostConfigFiles_FencingCredentials(t *testing.T) {
cases := []struct {
name string
hosts []agent.Host
credentials []types.Credential
expectedFiles map[string]string
}{
{
name: "mac matches existing host directory",
hosts: []agent.Host{
{
Hostname: "master-0",
Interfaces: []*aiv1beta1.Interface{{MacAddress: "aa:bb:cc:dd:ee:01"}},
},
},
credentials: []types.Credential{
{MACAddress: "AA:BB:CC:DD:EE:01", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
expectedFiles: map[string]string{
"master-0/fencing-credentials.yaml": "username: admin",
},
},
{
name: "mac with no matching host creates new directory",
hosts: []agent.Host{
{
Hostname: "master-0",
Interfaces: []*aiv1beta1.Interface{{MacAddress: "aa:bb:cc:dd:ee:01"}},
},
},
credentials: []types.Credential{
{MACAddress: "FF:FF:FF:FF:FF:FF", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
expectedFiles: map[string]string{
"host-1/fencing-credentials.yaml": "username: admin",
"host-1/mac_addresses": "ff:ff:ff:ff:ff:ff",
},
},
{
name: "case-insensitive mac matching",
hosts: []agent.Host{
{
Hostname: "master-0",
Interfaces: []*aiv1beta1.Interface{{MacAddress: "aa:bb:cc:dd:ee:01"}},
},
},
credentials: []types.Credential{
{MACAddress: "AA:BB:CC:DD:EE:01", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
expectedFiles: map[string]string{
"master-0/fencing-credentials.yaml": "username: admin",
},
},
{
name: "new dir avoids collision with existing host-0",
hosts: []agent.Host{
{
Interfaces: []*aiv1beta1.Interface{{MacAddress: "aa:bb:cc:dd:ee:01"}},
},
},
credentials: []types.Credential{
{MACAddress: "FF:FF:FF:FF:FF:FF", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
expectedFiles: map[string]string{
"host-1/fencing-credentials.yaml": "username: admin",
"host-1/mac_addresses": "ff:ff:ff:ff:ff:ff",
},
},
{
name: "new dir avoids collision with empty host that has no files",
hosts: []agent.Host{
{},
},
credentials: []types.Credential{
{MACAddress: "FF:FF:FF:FF:FF:FF", Username: "admin", Password: "pass", Address: "redfish+https://10.0.0.1/redfish/v1/Systems/1"},
},
expectedFiles: map[string]string{
"host-1/fencing-credentials.yaml": "username: admin",
"host-1/mac_addresses": "ff:ff:ff:ff:ff:ff",
},
},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
ah := &AgentHosts{
Hosts: tc.hosts,
FencingCredentialsHost: tc.credentials,
}

files, err := ah.HostConfigFiles()
assert.NoError(t, err)

for path, expectedContent := range tc.expectedFiles {
data, ok := files[path]
assert.True(t, ok, "expected file at %s", path)
assert.Contains(t, string(data), expectedContent)
}
})
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

func getInstallConfigWithFencing(credentials []*types.Credential) *agentAsset.OptionalInstallConfig {
ic := getNoHostsInstallConfig()
ic.Config.ControlPlane.Fencing = &types.Fencing{
Credentials: credentials,
}
return ic
}

func getAgentConfigNoHostname() *AgentConfig {
a := getNoHostsAgentConfig()
a.Config.Hosts = []agent.Host{
{
Role: "master",
Interfaces: []*aiv1beta1.Interface{
{
Name: "eth0",
MacAddress: "28:d2:44:d2:b2:1a",
},
},
},
}
return a
}

func getInstallConfigWithMismatchedNetworkConfig() *agentAsset.OptionalInstallConfig {
a := getInstallConfigSingleHost()
a.Config.Platform.BareMetal.Hosts[0].NetworkConfig = &apiextv1.JSON{
Expand Down
2 changes: 2 additions & 0 deletions pkg/asset/agent/agentconfig/fencingcredentials_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,8 @@ func TestFencingCredentials_YAMLFormat(t *testing.T) {
assert.NotContains(t, yamlContent, "macaddress:")
})

// This subtest verifies YAML format only. Placement into per-host directories
// for MAC-keyed credentials is handled by addFencingCredentials() in ignition.go.
t.Run("with macaddress", func(t *testing.T) {
parents := asset.Parents{}
parents.Add(
Expand Down
Loading