Skip to content

AGENT-246: Temporarily require at least one NMStateConfig to be specified #6239

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

Merged
openshift-merge-robot merged 4 commits into from
Aug 18, 2022
Merged

AGENT-246: Temporarily require at least one NMStateConfig to be specified #6239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc206a7
unit tests: Improve error testing
zaneb Aug 18, 2022
36ce948
Always have an NMStateConfig in ignition tests
zaneb Aug 17, 2022
b0c4f10
Make getting static IP more robust
zaneb Aug 18, 2022
a17dfe3
AGENT-246: Temporarily require at least one NMStateConfig to be speci…
zaneb Aug 17, 2022
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
4 changes: 4 additions & 0 deletions pkg/asset/agent/image/ignition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,10 @@ func (a *Ignition) Generate(dependencies asset.Parents) error {
},
}

if len(agentManifests.NMStateConfigs) == 0 {
Copy link
Contributor

@andfasano andfasano Aug 18, 2022

Choose a reason for hiding this comment

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

I'd recommend to perform this check in the related asset, here:

installer/pkg/asset/agent/manifests/nmstateconfig.go

Line 193 in c70b4fa

Copy link
Member Author

@zaneb zaneb Aug 18, 2022

Choose a reason for hiding this comment

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

It's legal to e.g. use an empty agent-config to create the cluster-manifests and only fill in the NMStateConfigs after. So we don't want to validate too early, only when you go to create the image and you don't have any NMStateConfigs.

Copy link
Contributor

@pawanpinjarkar pawanpinjarkar Aug 18, 2022

Choose a reason for hiding this comment

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

Should the message be more clear that either provide at least one nmstateconfig in cluster-manifests/nmstateconfig.yaml or if providing agent-config.yaml provide at least one nmstateconfig in the networkConfig?

Copy link
Contributor

@andfasano andfasano Aug 18, 2022

Choose a reason for hiding this comment

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

That's right, but it's just the NMStateConfig asset that loads the content of the cluster-manifests/nmstateconfig.yaml file if present on the disk. The AgentManifests it's just a collector, and it's resolved as a dependency in stack by the Ignition one, so not sure if I'm missing something but the validation location doesn't seem the right one?

Copy link
Contributor

@andfasano andfasano Aug 18, 2022

Choose a reason for hiding this comment

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

Screenshot from 2022-08-18 16-54-32

Copy link
Contributor

@andfasano andfasano Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if the create cluster-manifests it's in good shape now, but if you run it with just the install-config.yaml (and without an agent-config.yaml, so that it will get generated), currently the nil check will already fail

Copy link
Contributor

@andfasano andfasano Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

if the validation is placed in nmstateconfig, then "create cluster-manifests" will immediately fail, thus forcing you to provide the nmstateconfig in the agent-config.

So wouldn't be better in general to provide an agent-config template so that the NMStateConfig will not fail, rather than moving the check into another asset?

Copy link
Member Author

@zaneb zaneb Aug 18, 2022

Choose a reason for hiding this comment

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

So wouldn't be better in general

This check is going away again in a couple of weeks, as soon as we have fixed all of the linked bugs. Let's not get too general.

to provide an agent-config template so that the NMStateConfig will not fail, rather than moving the check into another asset?

That wouldn't solve the case where you intentionally create an agent-config with no NMState data, with the intent to customise the manifests to add NMState data after create cluster-manifests.
The only way to catch that case is to make sure the validation doesn't run until you do create image, and the only way to do that is to put it in an asset that is only required by the image target.

Copy link
Contributor

@andfasano andfasano Aug 19, 2022

Choose a reason for hiding this comment

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

Agree, especially considering if that check will go away soon. In general my concerns are about not making an asset self-sufficient, which could indicate a potential problem somewhere else

Copy link
Contributor

@andfasano andfasano Aug 19, 2022

Choose a reason for hiding this comment

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

Btw, the aforementioned scenario was still broken for me, I will re-test it with the latest changes as a double-check

return errors.New("at least one NMState configuration must be provided")
}

nodeZeroIP, err := retrieveRendezvousIP(agentConfigAsset.Config, agentManifests.NMStateConfigs)
if err != nil {
return err
Expand Down
13 changes: 13 additions & 0 deletions pkg/asset/agent/image/ignition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (

hiveext "github.com/openshift/assisted-service/api/hiveextension/v1beta1"
"github.com/openshift/assisted-service/api/v1beta1"
aiv1beta1 "github.com/openshift/assisted-service/api/v1beta1"
"github.com/openshift/assisted-service/models"
hivev1 "github.com/openshift/hive/apis/hive/v1"
"github.com/openshift/installer/pkg/asset"
Expand Down Expand Up @@ -390,6 +391,18 @@ func buildIgnitionAssetDefaultDependencies() []asset.Asset {
},
},
},
NMStateConfigs: []*aiv1beta1.NMStateConfig{
{
Spec: aiv1beta1.NMStateConfigSpec{
Interfaces: []*aiv1beta1.Interface{
{
Name: "eth0",
MacAddress: "00:01:02:03:04:05",
},
},
},
},
},
},
&agentconfig.AgentConfig{
Config: &agent.Config{
Expand Down
48 changes: 29 additions & 19 deletions pkg/asset/agent/manifests/nmstateconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,31 +222,41 @@ func (n *NMStateConfig) validateNMStateLabels() field.ErrorList {
return allErrs
}

func getFirstIP(nmStateConfig *nmStateConfig) string {
for _, intf := range nmStateConfig.Interfaces {
for _, addr4 := range intf.IPV4.Address {
if addr4.IP != "" {
return addr4.IP
}
}
for _, addr6 := range intf.IPV6.Address {
if addr6.IP != "" {
return addr6.IP
}
}
}
return ""
}

// GetNodeZeroIP retrieves the first IP from the user provided NMStateConfigs to set as the node0 IP
func GetNodeZeroIP(nmStateConfigs []*aiv1beta1.NMStateConfig) (string, error) {
var nmStateConfig nmStateConfig
// Use entry for first host
err := yaml.Unmarshal(nmStateConfigs[0].Spec.NetConfig.Raw, &nmStateConfig)
if err != nil {
return "", fmt.Errorf("error unmarshalling nodeZero nmStateConfig: %v", err)
}
for i := range nmStateConfigs {
var nmStateConfig nmStateConfig
err := yaml.Unmarshal(nmStateConfigs[i].Spec.NetConfig.Raw, &nmStateConfig)
if err != nil {
return "", fmt.Errorf("error unmarshalling NMStateConfig: %v", err)
}
if nodeZeroIP := getFirstIP(&nmStateConfig); nodeZeroIP != "" {
if net.ParseIP(nodeZeroIP) == nil {
return "", fmt.Errorf("could not parse static IP: %s", nodeZeroIP)
}

var nodeZeroIP string
if nmStateConfig.Interfaces == nil {
return "", fmt.Errorf("invalid NMStateConfig yaml, no valid interfaces set")
}
return nodeZeroIP, nil
}

if nmStateConfig.Interfaces[0].IPV4.Address != nil {
nodeZeroIP = nmStateConfig.Interfaces[0].IPV4.Address[0].IP
}
if nmStateConfig.Interfaces[0].IPV6.Address != nil {
nodeZeroIP = nmStateConfig.Interfaces[0].IPV6.Address[0].IP
}
if net.ParseIP(nodeZeroIP) == nil {
return "", fmt.Errorf("could not parse nodeZeroIP: %s", nodeZeroIP)
}

return nodeZeroIP, nil
return "", fmt.Errorf("invalid NMStateConfig yaml, no interface IPs set")
}

// GetNMIgnitionFiles returns the list of NetworkManager configuration files
Expand Down
172 changes: 169 additions & 3 deletions pkg/asset/agent/manifests/nmstateconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ spec:
- name: "eth0"
macAddress: "52:54:01:bb:bb:b1"`,
requiresNmstatectl: true,
expectedError: "staticNetwork configuration is not valid.*",
expectedError: "staticNetwork configuration is not valid",
},

{
Expand All @@ -314,7 +314,7 @@ spec:
- name: "eth0"
macAddress: "52:54:01:aa:aa:a1"`,
requiresNmstatectl: true,
expectedError: "staticNetwork configuration is not valid.*",
expectedError: "staticNetwork configuration is not valid",
},

{
Expand Down Expand Up @@ -383,7 +383,7 @@ spec:
found, err := asset.Load(fileFetcher)
assert.Equal(t, tc.expectedFound, found, "unexpected found value returned from Load")
if tc.expectedError != "" {
assert.Regexp(t, tc.expectedError, err.Error())
assert.ErrorContains(t, err, tc.expectedError)
} else {
assert.NoError(t, err)
}
Expand All @@ -409,3 +409,169 @@ spec:
})
}
}

func TestGetNodeZeroIP(t *testing.T) {
cases := []struct {
name string
expectedIP string
expectedError string
configs []string
}{
{
name: "no interfaces",
expectedError: "no interface IPs set",
},
{
name: "first interface",
expectedIP: "192.168.122.21",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
ipv4:
address:
- ip: 192.168.122.21
- name: eth1
type: ethernet
ipv4:
address:
- ip: 192.168.122.22
`,
},
},
{
name: "second interface",
expectedIP: "192.168.122.22",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
- name: eth1
type: ethernet
ipv4:
address:
- ip: 192.168.122.22
`,
},
},
{
name: "second host",
expectedIP: "192.168.122.22",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
- name: eth1
type: ethernet
`,
`
interfaces:
- name: eth0
type: ethernet
- name: eth1
type: ethernet
ipv4:
address:
- ip: 192.168.122.22
`,
},
},
{
name: "ipv4 first",
expectedIP: "192.168.122.22",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
ipv6:
address:
- ip: "2001:0db8::0001"
ipv4:
address:
- ip: 192.168.122.22
`,
},
},
{
name: "ipv6 host first",
expectedIP: "2001:0db8::0001",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
ipv6:
address:
- ip: "2001:0db8::0001"
`,
`
interfaces:
- name: eth0
type: ethernet
ipv4:
address:
- ip: 192.168.122.31
`,
},
},
{
name: "ipv6 first",
expectedIP: "2001:0db8::0001",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
ipv6:
address:
- ip: "2001:0db8::0001"
- name: eth1
type: ethernet
ipv4:
address:
- ip: 192.168.122.22
`,
},
},
{
name: "ipv6",
expectedIP: "2001:0db8::0001",
configs: []string{
`
interfaces:
- name: eth0
type: ethernet
ipv6:
address:
- ip: "2001:0db8::0001"
`,
},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
var configs []*aiv1beta1.NMStateConfig
for _, hostRaw := range tc.configs {
configs = append(configs, &aiv1beta1.NMStateConfig{
Spec: aiv1beta1.NMStateConfigSpec{
NetConfig: aiv1beta1.NetConfig{
Raw: aiv1beta1.RawNetConfig(hostRaw),
},
},
})
}

ip, err := GetNodeZeroIP(configs)
if tc.expectedError == "" {
assert.NoError(t, err)
assert.Equal(t, tc.expectedIP, ip)
} else {
assert.ErrorContains(t, err, tc.expectedError)
}
})
}
}