MGMT-17618: NMStateConfig interfaces presence should be validated #6305

ori-amizur · 2024-05-09T16:37:51Z

In case there is static networking configuration without mac-interface mapping, it is created but the configuration is not applied.

This change validates that for every ethernet interface there is a corresponding mac-interface mapping when the static network configuration is set during infa-env registration or during infra-env update.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

/cc @gamli75

openshift-ci-robot · 2024-05-09T16:37:55Z

ori-amizur · 2024-05-09T16:38:37Z

/test?

openshift-ci · 2024-05-09T16:38:43Z

@ori-amizur: The following commands are available to trigger required jobs:

/test e2e-agent-compact-ipv4
/test edge-assisted-operator-catalog-publish-verify
/test edge-ci-index
/test edge-e2e-ai-operator-ztp
/test edge-e2e-ai-operator-ztp-sno-day2-workers
/test edge-e2e-ai-operator-ztp-sno-day2-workers-late-binding
/test edge-e2e-metal-assisted
/test edge-e2e-metal-assisted-4-11
/test edge-e2e-metal-assisted-4-12
/test edge-e2e-metal-assisted-cnv
/test edge-e2e-metal-assisted-lvm
/test edge-e2e-metal-assisted-odf
/test edge-images
/test edge-lint
/test edge-subsystem-aws
/test edge-subsystem-kubeapi-aws
/test edge-unit-test
/test edge-verify-generated-code
/test images
/test mce-images

The following commands are available to trigger optional jobs:

/test e2e-agent-ha-dualstack
/test e2e-agent-sno-ipv6
/test edge-e2e-ai-operator-disconnected-capi
/test edge-e2e-ai-operator-ztp-3masters
/test edge-e2e-ai-operator-ztp-capi
/test edge-e2e-ai-operator-ztp-compact-day2-masters
/test edge-e2e-ai-operator-ztp-compact-day2-workers
/test edge-e2e-ai-operator-ztp-disconnected
/test edge-e2e-ai-operator-ztp-hypershift-zero-nodes
/test edge-e2e-ai-operator-ztp-multiarch-3masters-ocp
/test edge-e2e-ai-operator-ztp-multiarch-sno-ocp
/test edge-e2e-ai-operator-ztp-node-labels
/test edge-e2e-ai-operator-ztp-sno-day2-masters
/test edge-e2e-ai-operator-ztp-sno-day2-workers-ignitionoverride
/test edge-e2e-metal-assisted-4-13
/test edge-e2e-metal-assisted-4-14
/test edge-e2e-metal-assisted-4-15
/test edge-e2e-metal-assisted-bond
/test edge-e2e-metal-assisted-bond-4-14
/test edge-e2e-metal-assisted-day2
/test edge-e2e-metal-assisted-day2-arm-workers
/test edge-e2e-metal-assisted-day2-single-node
/test edge-e2e-metal-assisted-external
/test edge-e2e-metal-assisted-external-4-14
/test edge-e2e-metal-assisted-ipv4v6
/test edge-e2e-metal-assisted-ipv6
/test edge-e2e-metal-assisted-kube-api-late-binding-single-node
/test edge-e2e-metal-assisted-kube-api-late-unbinding-ipv4-single-node
/test edge-e2e-metal-assisted-kube-api-net-suite
/test edge-e2e-metal-assisted-mce-4-11
/test edge-e2e-metal-assisted-mce-4-12
/test edge-e2e-metal-assisted-mce-4-13
/test edge-e2e-metal-assisted-mce-4-14
/test edge-e2e-metal-assisted-mce-4-15
/test edge-e2e-metal-assisted-mce-sno
/test edge-e2e-metal-assisted-metallb
/test edge-e2e-metal-assisted-none
/test edge-e2e-metal-assisted-onprem
/test edge-e2e-metal-assisted-single-node
/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-static-ip-suite-4-14
/test edge-e2e-metal-assisted-tang
/test edge-e2e-metal-assisted-tpmv2
/test edge-e2e-metal-assisted-upgrade-agent
/test edge-e2e-nutanix-assisted
/test edge-e2e-nutanix-assisted-2workers
/test edge-e2e-nutanix-assisted-4-14
/test edge-e2e-oci-assisted
/test edge-e2e-oci-assisted-4-14
/test edge-e2e-oci-assisted-iscsi
/test edge-e2e-vsphere-assisted
/test edge-e2e-vsphere-assisted-4-12
/test edge-e2e-vsphere-assisted-4-13
/test edge-e2e-vsphere-assisted-4-14
/test edge-e2e-vsphere-assisted-umn
/test okd-scos-images
/test push-pr-image

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-assisted-service-master-e2e-agent-compact-ipv4
pull-ci-openshift-assisted-service-master-edge-ci-index
pull-ci-openshift-assisted-service-master-edge-e2e-ai-operator-ztp
pull-ci-openshift-assisted-service-master-edge-e2e-metal-assisted
pull-ci-openshift-assisted-service-master-edge-images
pull-ci-openshift-assisted-service-master-edge-lint
pull-ci-openshift-assisted-service-master-edge-subsystem-aws
pull-ci-openshift-assisted-service-master-edge-subsystem-kubeapi-aws
pull-ci-openshift-assisted-service-master-edge-unit-test
pull-ci-openshift-assisted-service-master-edge-verify-generated-code
pull-ci-openshift-assisted-service-master-images
pull-ci-openshift-assisted-service-master-mce-images

In response to this:

/test?

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.

ori-amizur · 2024-05-09T16:39:48Z

/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-bond

codecov · 2024-05-09T16:54:22Z

Codecov Report

Attention: Patch coverage is 53.57143% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 68.04%. Comparing base (904ee71) to head (bc3981e).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6305      +/-   ##
==========================================
- Coverage   68.29%   68.04%   -0.25%     
==========================================
  Files         241      244       +3     
  Lines       35863    37024    +1161     
==========================================
+ Hits        24491    25193     +702     
- Misses       9212     9560     +348     
- Partials     2160     2271     +111

Files	Coverage Δ
pkg/staticnetworkconfig/generator.go	`32.32% <53.57%> (+4.32%)`	⬆️

... and 11 files with indirect coverage changes

ori-amizur · 2024-05-09T17:58:17Z

/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-bond

ori-amizur · 2024-05-09T21:09:41Z

/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-bond

ori-amizur · 2024-05-10T16:09:01Z

/test edge-e2e-metal-assisted-static-ip-suite

gamli75 · 2024-05-12T09:47:15Z

/cc @carbonin

carbonin

I'm a bit worried that parsing out the nmconnection file is a bit fragile.

I was imagining that we just ensure there is something in the HostStaticNetworkConfig.MacInterfaceMap. Is there a use case for that being empty?

ori-amizur · 2024-05-15T10:13:04Z

I'm a bit worried that parsing out the nmconnection file is a bit fragile.

We are already doing it here:

assisted-service/pkg/staticnetworkconfig/generator.go

Lines 139 to 165 in 904ee71

    
           func (s *StaticNetworkConfigGenerator) formatNMConnection(nmConnection string) (string, error) { 
        
           	ini.PrettyFormat = false 
        
           	cfg, err := ini.LoadSources(ini.LoadOptions{IgnoreInlineComment: true}, []byte(nmConnection)) 
        
           	if err != nil { 
        
           		s.log.WithError(err).Errorf("Failed to load the ini format string %s", nmConnection) 
        
           		return "", err 
        
           	} 
        
           	connectionSection := cfg.Section("connection") 
        
           	_, err = connectionSection.NewKey("autoconnect", "true") 
        
           	if err != nil { 
        
           		s.log.WithError(err).Errorf("Failed to add autoconnect key to section connection") 
        
           		return "", err 
        
           	} 
        
           	_, err = connectionSection.NewKey("autoconnect-priority", "1") 
        
           	if err != nil { 
        
           		s.log.WithError(err).Errorf("Failed to add autoconnect-priority key to section connection") 
        
           		return "", err 
        
           	} 
        
           	buf := new(bytes.Buffer) 
        
           	_, err = cfg.WriteTo(buf) 
        
           	if err != nil { 
        
           		s.log.WithError(err).Errorf("Failed to output nmconnection ini file to buffer") 
        
           		return "", err 
        
           	} 
        
           	return buf.String(), nil 
        
           }

- so it isn't very different. So if it was OK there, then it is OK here.

I was imagining that we just ensure there is something in the HostStaticNetworkConfig.MacInterfaceMap. Is there a use case for that being empty?

I don't think there is use case for being empty - but this is a stronger validation. It validates that for every ethernet interface there is a corresponding mac-mapping. So typos and ignored interfaces can be detected.

carbonin · 2024-05-15T13:17:21Z

so it isn't very different. So if it was OK there, then it is OK here

Fair enough.

Are you sure that this isn't going to cause issues for bonds/vlans? Do we have a test anywhere for this?
Are those files formatted differently such that this would fail or not validate them?

ori-amizur · 2024-05-15T13:56:56Z

Are you sure that this isn't going to cause issues for bonds/vlans?

It should be tested - like any other change.

Do we have a test anywhere for this?

We have a test for bond. We don't have a test for vlan yet.

Are those files formatted differently such that this would fail or not validate them?

They all look similar. They all have a connection section with interface-name and type.
The unit-test already contain bond tests. I can add also vlan tests.

ori-amizur · 2024-05-15T15:10:04Z

/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-bond

pkg/staticnetworkconfig/generator_test.go

ori-amizur · 2024-05-23T06:41:37Z

/retest-required

In case there is static networking configuration without mac-interface mapping, it is created but the configuration is not applied. This change validates that for every ethernet interface there is a corresponding mac-interface mapping when the static network configuration is set during infa-env registration or during infra-env update.

ori-amizur · 2024-05-23T10:24:55Z

/test edge-e2e-metal-assisted-static-ip-suite
/test edge-e2e-metal-assisted-bond

openshift-ci · 2024-05-23T11:50:49Z

@ori-amizur: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

paul-maidment

/lgtm

paul-maidment · 2024-05-23T13:36:57Z

/approve

openshift-ci · 2024-05-23T13:37:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ori-amizur, paul-maidment

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:

~~OWNERS~~ [ori-amizur,paul-maidment]

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

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 9, 2024

ori-amizur marked this pull request as draft May 9, 2024 16:37

openshift-ci bot requested a review from gamli75 May 9, 2024 16:38

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 9, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 9, 2024

ori-amizur marked this pull request as ready for review May 10, 2024 12:19

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2024

openshift-ci bot requested a review from carbonin May 12, 2024 09:47

ori-amizur force-pushed the MGMT-17618 branch from e3a8de3 to aa53484 Compare May 12, 2024 13:44

carbonin reviewed May 13, 2024

View reviewed changes

ori-amizur requested a review from carbonin May 15, 2024 12:53

ori-amizur force-pushed the MGMT-17618 branch from aa53484 to ba56b01 Compare May 16, 2024 07:47

paul-maidment reviewed May 16, 2024

View reviewed changes

pkg/staticnetworkconfig/generator_test.go Show resolved Hide resolved

ori-amizur force-pushed the MGMT-17618 branch from ba56b01 to c863b3e Compare May 16, 2024 10:57

ori-amizur requested a review from paul-maidment May 21, 2024 19:25

ori-amizur force-pushed the MGMT-17618 branch from c863b3e to bc3981e Compare May 23, 2024 07:01

paul-maidment approved these changes May 23, 2024

View reviewed changes

openshift-ci bot assigned paul-maidment May 23, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 23, 2024

openshift-merge-bot bot merged commit 484d429 into openshift:master May 23, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-17618: NMStateConfig interfaces presence should be validated #6305

MGMT-17618: NMStateConfig interfaces presence should be validated #6305

ori-amizur commented May 9, 2024

openshift-ci-robot commented May 9, 2024 •

edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

ori-amizur commented May 9, 2024

openshift-ci bot commented May 9, 2024

ori-amizur commented May 9, 2024

codecov bot commented May 9, 2024 •

edited

ori-amizur commented May 9, 2024

ori-amizur commented May 9, 2024

ori-amizur commented May 10, 2024

gamli75 commented May 12, 2024

carbonin left a comment

ori-amizur commented May 15, 2024 •

edited

carbonin commented May 15, 2024

ori-amizur commented May 15, 2024 •

edited

ori-amizur commented May 15, 2024

ori-amizur commented May 23, 2024

ori-amizur commented May 23, 2024

openshift-ci bot commented May 23, 2024

paul-maidment left a comment

paul-maidment commented May 23, 2024

openshift-ci bot commented May 23, 2024

MGMT-17618: NMStateConfig interfaces presence should be validated #6305

MGMT-17618: NMStateConfig interfaces presence should be validated #6305

Conversation

ori-amizur commented May 9, 2024

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented May 9, 2024 • edited by openshift-ci bot

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

ori-amizur commented May 9, 2024

openshift-ci bot commented May 9, 2024

ori-amizur commented May 9, 2024

codecov bot commented May 9, 2024 • edited

Codecov Report

ori-amizur commented May 9, 2024

ori-amizur commented May 9, 2024

ori-amizur commented May 10, 2024

gamli75 commented May 12, 2024

carbonin left a comment

Choose a reason for hiding this comment

ori-amizur commented May 15, 2024 • edited

carbonin commented May 15, 2024

ori-amizur commented May 15, 2024 • edited

ori-amizur commented May 15, 2024

ori-amizur commented May 23, 2024

ori-amizur commented May 23, 2024

openshift-ci bot commented May 23, 2024

paul-maidment left a comment

Choose a reason for hiding this comment

paul-maidment commented May 23, 2024

openshift-ci bot commented May 23, 2024

openshift-ci-robot commented May 9, 2024 •

edited by openshift-ci bot

codecov bot commented May 9, 2024 •

edited

ori-amizur commented May 15, 2024 •

edited

ori-amizur commented May 15, 2024 •

edited