cleanup validations for VPN connection creation by DaanHoogland · Pull Request #9195 · apache/cloudstack

DaanHoogland · 2024-06-10T08:29:10Z

Description

This PR adds a validation when creating a VPN connection in a VPC to make sure the VPN gateway exists.

Fixes: #7250

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)
build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2024-06-10T08:38:21Z

Codecov Report

Attention: Patch coverage is 0% with 48 lines in your changes missing coverage. Please review.

Project coverage is 15.08%. Comparing base (67ce326) to head (c9a722b).
Report is 1 commits behind head on 4.19.

Files with missing lines	Patch %	Lines
...com/cloud/network/vpn/Site2SiteVpnManagerImpl.java	0.00%	48 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.19    #9195      +/-   ##
============================================
- Coverage     15.08%   15.08%   -0.01%     
- Complexity    11182    11183       +1     
============================================
  Files          5402     5402              
  Lines        473137   473148      +11     
  Branches      61094    61020      -74     
============================================
  Hits          71364    71364              
- Misses       393835   393846      +11     
  Partials       7938     7938

Flag	Coverage Δ
uitests	`4.30% <ø> (-0.01%)`	⬇️
unittests	`15.80% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

yadvr

LGTM, this may need s2s testing if not already covered by a smoketest

hsato03

CLGTM.

Just one remark, maybe we can use a unique error message in the getAndValidate methods.

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java

DaanHoogland · 2024-06-19T06:33:45Z

CLGTM.

Just one remark, maybe we can use a unique error message in the getAndValidate methods.

The calls have different messages passed (though only slightly). Can you specify what you would like to see there @hsato03 ?

hsato03 · 2024-06-19T16:07:54Z

CLGTM.
Just one remark, maybe we can use a unique error message in the getAndValidate methods.

The calls have different messages passed (though only slightly). Can you specify what you would like to see there @hsato03 ?

What about throw new InvalidParameterValueException(String.format("Unable to find the specified Site-to-Site VPN customer gateway: %s.", customerGatewayId)); in the getAndValidateSite2SiteCustomerGateway

and throw new InvalidParameterValueException(String.format("Unable to find the specified Site-to-Site VPN gateway %s.", vpnGatewayId)); in the getAndValidateSite2SiteVpnGateway?

This should cover all cases and the errMsg parameter can be removed.

DaanHoogland · 2024-06-20T08:20:45Z

@hsato03 applied your suggestion and did some extra cleanup.

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java

sureshanaparti

minor suggestions, clgtm

DaanHoogland · 2024-09-25T17:17:51Z

The issue is not resolved. When I try creating a site to site VPN connection without enabling the VPN gateway of the VPC it doesn't throw any error.

hm, it has been a while and had some updates. I'll retest and see...

@rajujith , this was probably due to some merge issues, It should work now. I.E. you should not be able to create a VPN connection without having a VPN gateway available. Can you try?

@DaanHoogland

I tried again with the packages from 17th Jul and the issue is not resolved.

When I try creating a site to site VPN connection without enabling the VPN gateway of the VPC it doesn't throw any error.

@kiranchavala it was only caught/solved in the API. It is now also the UI.

DaanHoogland · 2024-09-25T17:18:12Z

@blueorangutan package

blueorangutan · 2024-09-25T17:20:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-09-25T18:12:10Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11208

blueorangutan · 2024-09-28T04:56:37Z

[SF] Trillian test result (tid-11568)
Environment: kvm-ol8 (x2), Advanced Networking with Mgmt server ol8
Total time taken: 51996 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr9195-t11568-kvm-ol8.zip
Smoke tests completed. 132 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py

Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

DaanHoogland · 2024-09-30T13:23:18Z

@blueorangutan package

blueorangutan · 2024-09-30T13:24:03Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2024-09-30T14:29:21Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 11242

rajujith

LGTM. It handles the error condition now.

vishesh92

clgtm

boring-cyborg bot added the component:networking label Jun 10, 2024

DaanHoogland linked an issue Jun 10, 2024 that may be closed by this pull request

Enabling site to site vpn connection before enabling VPN Gateway doesn't throw an error message #7250

Closed

DaanHoogland added this to the 4.19.1.0 milestone Jun 10, 2024

DaanHoogland added the status:needs-testing label Jun 14, 2024

yadvr approved these changes Jun 14, 2024

View reviewed changes

yadvr requested review from sureshanaparti and weizhouapache June 18, 2024 07:34

hsato03 approved these changes Jun 18, 2024

View reviewed changes

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java Outdated Show resolved Hide resolved

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java Outdated Show resolved Hide resolved

DaanHoogland force-pushed the ghi7250-vpnGatewayMissingNotification branch from 5aaa6d4 to 215c5f4 Compare June 20, 2024 08:19

sureshanaparti reviewed Jun 20, 2024

View reviewed changes

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java Outdated Show resolved Hide resolved

sureshanaparti reviewed Jun 20, 2024

View reviewed changes

server/src/main/java/com/cloud/network/vpn/Site2SiteVpnManagerImpl.java Outdated Show resolved Hide resolved

sureshanaparti approved these changes Jun 20, 2024

View reviewed changes

apache deleted a comment from blueorangutan Jun 21, 2024

apache deleted a comment from blueorangutan Sep 24, 2024

boring-cyborg bot added the component:UI label Sep 25, 2024

DaanHoogland and others added 10 commits September 30, 2024 15:23

cleanup validations for VPN connection creation

9687fd4

Apply suggestions from code review

fa3a4f8

Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

review sugestion plus cleanup

bfaf450

Apply suggestions from code review

addb093

Co-authored-by: Suresh Kumar Anaparti <sureshkumar.anaparti@gmail.com>

revert some cleanup

088a3db

Apply suggestions from code review

3287781

more cleanup revert

d7cf413

Apply suggestions from code review

7042cfb

pass error from UI to API

c36d973

import reorg

c9a722b

DaanHoogland force-pushed the ghi7250-vpnGatewayMissingNotification branch from 64aa6e2 to c9a722b Compare September 30, 2024 13:23

rajujith approved these changes Oct 1, 2024

View reviewed changes

DaanHoogland removed the status:needs-testing label Oct 1, 2024

vishesh92 approved these changes Oct 1, 2024

View reviewed changes

vishesh92 merged commit 00fe5f1 into apache:4.19 Oct 1, 2024

vishesh92 deleted the ghi7250-vpnGatewayMissingNotification branch October 1, 2024 08:25

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Oct 14, 2024

cleanup validations for VPN connection creation (apache#9195)

cb44b61

DaanHoogland mentioned this pull request Nov 8, 2024

Enabling site to site vpn connection before enabling VPN Gateway doesn't throw an error message #7250

Closed

Conversation

DaanHoogland commented Jun 10, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

codecov bot commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yadvr left a comment

Choose a reason for hiding this comment

Uh oh!

hsato03 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaanHoogland commented Jun 19, 2024

Uh oh!

hsato03 commented Jun 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Jun 20, 2024

Uh oh!

Uh oh!

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Sep 25, 2024

Uh oh!

DaanHoogland commented Sep 25, 2024

Uh oh!

blueorangutan commented Sep 25, 2024

Uh oh!

blueorangutan commented Sep 25, 2024

Uh oh!

blueorangutan commented Sep 28, 2024

Uh oh!

DaanHoogland commented Sep 30, 2024

Uh oh!

blueorangutan commented Sep 30, 2024

Uh oh!

blueorangutan commented Sep 30, 2024

Uh oh!

rajujith left a comment

Choose a reason for hiding this comment

Uh oh!

vishesh92 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

codecov bot commented Jun 10, 2024 •

edited

Loading

hsato03 left a comment •

edited

Loading

hsato03 commented Jun 19, 2024 •

edited

Loading