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

Jump to bottom

Eigrp #11301 - Configuration failed error type: validation #14765

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Eigrp #11301 - Configuration failed error type: validation #14765

wants to merge 2 commits into from
+290 −81

Conversation

1337kerberos
Copy link
Contributor

@1337kerberos 1337kerberos commented Nov 9, 2023 •
edited
Loading

For more details, please look at: #11301 (comment)
Additional context: https://vyos.dev/T5737

@frrbot frrbot bot added the bugfix label Nov 9, 2023
@1337kerberos 1337kerberos force-pushed the EIGRP_BUG_11301_VALIDATION branch from b15c391 to 7010f1f Compare November 9, 2023 17:54
@1337kerberos
Copy link
Contributor Author

Not sure if I need to fix the CI warnings, it is not dangerous to ignore identifier names for function declarations in the header. Unless maintainers would like me to handle this.

@donaldsharp
Copy link
Member

The rule is that you don't have to fix something you don't touch but if you add new code it must conform to our standard. The tool is telling you about the problem

ton31337
ton31337 requested changes Nov 9, 2023
View reviewed changes
Copy link
Member

@ton31337 ton31337 left a comment

Choose a reason for hiding this comment

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

Documentation and the tests are missing yet.

eigrpd/eigrp_cli.c Outdated Show resolved Hide resolved
eigrpd/eigrp_interface.c
@@ -91,7 +85,7 @@ struct eigrp_interface *eigrp_if_new(struct eigrp *eigrp, struct interface *ifp,
ei->curr_bandwidth = ifp->bandwidth;
ei->curr_mtu = ifp->mtu;

return ei;
return 0;
Copy link
Member

Choose a reason for hiding this comment

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

If this is just returning, we might then use a void() at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a hook definition; the declaration is expected to return int. The same goes for interface deletion.

eigrpd/eigrp_interface.c Outdated Show resolved Hide resolved
eigrpd/eigrp_interface.c Outdated
if (str && !strcmp(str, ifname))
return i;
}
return -1;
Copy link
Member

Choose a reason for hiding this comment

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

I see this function could be converted to boolean, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, it returns the interface index within a vector or -1 if an interface is missing.

Copy link
Member

Choose a reason for hiding this comment

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

there is a definition for an invalid interface ... I'll find it and put it here, but that's what we should use rather than -1

Copy link
Member

@donaldsharp donaldsharp Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

IFINDEX_INTERNAL -> 0 is the only legal value to use for a ifindex if we do not know the ifindex. Everything else can be a legal value and as such we should not be using it

@riw777 riw777 self-requested a review November 14, 2023 16:15
riw777
riw777 approved these changes Nov 21, 2023
View reviewed changes
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

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

looks good ... waiting on @ton31337 's comments to be resolved

@1337kerberos 1337kerberos force-pushed the EIGRP_BUG_11301_VALIDATION branch from 7010f1f to 9c4e157 Compare November 22, 2023 21:11
@github-actions github-actions bot added the rebase PR needs rebase label Nov 22, 2023
@1337kerberos
Copy link
Contributor Author

Regarding missing documentation - it is already present.

eigrpd.rst:
----------------------------------------------------------------------------------------------
.. clicmd:: passive-interface (IFNAME|default)

   This command sets the specified interface to passive mode. On passive mode
   interface, all receiving packets are ignored and eigrpd does not send either
   multicast or unicast EIGRP packets except to EIGRP neighbors specified with
   `neighbor` command. The interface may be specified as `default` to make
   eigrpd default to passive on all interfaces.

   The default is to be passive on all interfaces.

I have fixed the CI warnings and will check if somebody from my team can assist with the test case.

@riw777
Copy link
Member

riw777 commented Nov 28, 2023

not certain we need a topo test here?

@riw777
Copy link
Member

riw777 commented Jan 16, 2024

@volodymyrhuti if we can get the invalid interface bits fixed, we can merge this, I think ...

@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

1 similar comment
@github-actions GitHub Actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

c-po added a commit to vyos/vyos-1x that referenced this pull request Feb 15, 2024
@c-po
eigrp: T2472: remove smoketest
71497e7 
Commit 0eb4168 ("eigrp: T2472: improve code for later tests") added a basic
smoketest for EIGRP, which is also run by the CI hence not having a +x bit at
all.

This just deletes the basic smoketest testing for ASN and EIGRP router-id.
We can revert it once it's fixed in FRR upstream.

FRRouting/frr#14765
Volodymyr Huti added 2 commits March 20, 2024 15:16
eigrp: fix - Configuration failed error type: validation
8968b3f 
Instatiate interface structure once demon is started.
Otherwise it is not possible to set parameters such as bandwidth
untill a `network` configuration was applied.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <[email protected]>
eigrp: reimplement passive-interface [default] command
d92757c 
Maintain a vector of mappings [if_name -> if_passive].
Otherwise, if topo is not already convereged, configuration
does not apply, since iface is missing from the iface list.

Fixes: FRRouting#11301
Signed-off-by: Volodymyr Huti <[email protected]>
@1337kerberos 1337kerberos force-pushed the EIGRP_BUG_11301_VALIDATION branch from 9c4e157 to d92757c Compare March 20, 2024 15:38
@riw777
Copy link
Member

riw777 commented Mar 26, 2024

Is anyone still working on this?

@riw777
Copy link
Member

riw777 commented Jul 23, 2024

@frrbot autoclose in 1 month

@frrbot frrbot bot added the autoclose label Jul 23, 2024
@1337kerberos
Copy link
Contributor Author

@riw777 sorry for not following up on this one. I believe it is still relevant and applicable
I ignored it as I was expecting extra help with testing from the team.
At the moment, I`m not more associated with VyOS, so the testing part is irrelevant now.
If you think it looks good, I will find a time next week to rebase and run it locally.

@frrbot frrbot bot removed the autoclose label Jul 27, 2024
@frrbot
Copy link

frrbot bot commented Jul 27, 2024

This issue will no longer be automatically closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@donaldsharp donaldsharp donaldsharp left review comments

@riw777 riw777 riw777 approved these changes

@ton31337 ton31337 ton31337 requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
bugfix master rebase PR needs rebase size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@1337kerberos @donaldsharp @riw777 @ton31337