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.

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

M #-: Add IP Spoofing support for NIC_ALIAS #4764

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

Conversation

rdiaz-on
Copy link
Contributor

Add IP addresses of a NIC_ALIAS to the corresponding ipset if the parent
NIC belongs to the "Bridged & Security Groups" network mode (fw) when a
VM is instantiated or a NIC_ALIAS attached.

Signed-off-by: Ricardo Diaz [email protected]

Add IP addresses of a NIC_ALIAS to the corresponding ipset if the parent
NIC belongs to the "Bridged & Security Groups" network mode (fw) when a
VM is instantiated or a NIC_ALIAS attached.

Signed-off-by: Ricardo Diaz <[email protected]>
@rdiaz-on rdiaz-on requested a review from rsmontero May 19, 2020 15:30
# Execute post-boot networking setup
{
:driver => :vnm,
:action => :post,
Copy link
Member

Choose a reason for hiding this comment

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

Why not clean action?

Copy link
Contributor Author

@rdiaz-on rdiaz-on Jun 12, 2020

Choose a reason for hiding this comment

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

Clean action deletes the whole iptables chain included the rules for the parent NIC

nic_build_hash(nic_element, nic)

if !VNMMAD.pre_action?
nic.get_info(self)
Copy link
Member

Choose a reason for hiding this comment

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

Do these functions work/make sense for NIC_ALIAS? (when calling it as nics_build('TEMPLATE/NIC_ALIAS')

# @param element [String] the NIC_ID
# @return [Hash] the NIC_ALIAS
def nic_alias(id)
if @nic_aliases
Copy link
Member

@rsmontero rsmontero Jun 1, 2020

Choose a reason for hiding this comment

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

better use exit condition.

return nil if @nic_aliases.nil?

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 will change it

@@ -334,6 +334,14 @@ def self.vars(vm, nic, sg_id = nil)
vars[:set_sg_out] = "#{vars[:chain]}-#{sg_id}-o"
end

vars[:nic_aliases] = []

unless nic[:alias_ids].nil?
Copy link
Member

Choose a reason for hiding this comment

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

prefer if !nic[:alias_ids].nil? for single statements

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 will change it

@atodorov-storpool
Copy link
Contributor

Hello.
I am sorry to step in but I am a bit disappointed with this PR. IMHO this is not M #- as there are two bugs related to this: #3079 and #3354 and there is already developed pull request for #3354 .
Looked at another side, besides implementing a solution that does not follow own guidelines (I mean #3354 (comment)) sound like downplaying contributor's work and is not nice. At least from my point of view.

Such behaviour and delaying PRs with years is highly discouraging to do project contributions.

Regards,
Anton

@atodorov-storpool
Copy link
Contributor

^ @rsmontero
#4764 (comment)

@rsmontero
Copy link
Member

Thanks @atodorov-storpool for this an other contributions to the project. As you see this PR is still in review and will include reference to the affected bugs.

As per the implementation this is indeed inspired by your PR, but re-written to improve its design and better fit in the driver. Also note that design decisions evolve with the code and need to consider its relation with other new components. Finally, we need to prioritize our available development resources, that unfortunately force us to delay some contributions. Please note that these decisions are not easy for us, and by not means imply that we underestimate your contributions.

Thanks again for your valuable contributions and hope you'll keep making them in the future.

@mzealey
Copy link

mzealey commented Oct 27, 2020

What is the status of this? It would be nice to have this included in opennebula...

@mzealey
Copy link

mzealey commented Oct 27, 2020

I have tested that this commit applies fine onto 5.12.0.3 and fixes the issue by adding/removing alias ips to the ipset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants