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

[OSD-19783] Add backplane subcommand to do health check of VPN and proxy #469

Merged

Conversation

xiaoyu74
Copy link
Contributor

@xiaoyu74 xiaoyu74 commented Jul 1, 2024

What type of PR is this?

(feature)

What this PR does / Why we need it?

Add backplane subcommand to do health check of VPN and proxy connectivity
More discussions in slack

Which Jira/Github issue(s) does this PR fix?

https://issues.redhat.com/browse/OSD-19783

Special notes for your reviewer

1. Latest updates resolved all the review comments - 16 July

  • Refactoring to move the main healthcheck logic to the pkg folder
  • Updated Makefile to use mockgen to generate mock files with the mock interfaces for the test cases
  • Removed the default values defined in the config.go to avoid expose internal URL, allowing the end-user to customize the endpoint URLs in the local backplane configuration
    • VPNCheckEndpoint
    • ProxyCheckEndpoint
  • Added debug logs
  • Split the main connectivity_checks.go function into check_vpn.go and check_proxy.go based on the core functionality.
  • The end-user needs to set the vpn and proxy check-endpoint in the local bp configuration, example below:
cat ~/.config/backplane/config.json 
{
    "proxy-url": ["http://proxy1.example.com:3128", "http://proxy2.example.com:3128"],
    "vpn-check-endpoint": "http://your-vpn-endpoint.example.com",
    "proxy-check-endpoint": "http://your-proxy-endpoint.example.com"
}

2. Command output examples

  • Command test with VPN and proxy working properly
./ocm-backplane health-check
Checking VPN connectivity...
VPN connectivity check passed!

Checking proxy connectivity...
Getting the working proxy URL ['http://proxy1.example.com:3128'] from local backplane configuration.
Testing connectivity to the pre-defined test endpoint ['https://your-proxy-endpoint.example.com'] with the proxy.
Proxy connectivity check passed!

Checking backplane API connectivity...
Successfully connected to the backplane API!
Backplane API connectivity check passed!
  • We can check VPN or Proxy connectivity respectively by specifying the --vpn or --proxy flag
./ocm-backplane healthcheck --vpn
Checking VPN connectivity...
VPN connectivity check passed!

./ocm-backplane healthcheck --proxy
Checking proxy connectivity...
Proxy connectivity check passed!
  • When VPN is disconnected
./ocm-backplane healthcheck --proxy
WARN[0000] No VPN interfaces found: [tun tap ppp wg utun]    
VPN connectivity check failed: No VPN interfaces found: [tun tap ppp wg utun]
Note: Proxy connectivity check requires VPN to be connected. Please ensure VPN is connected and try again.

@openshift-ci openshift-ci bot requested review from feichashao and typeid July 1, 2024 03:03
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 1, 2024
@xiaoyu74
Copy link
Contributor Author

xiaoyu74 commented Jul 1, 2024

/retest

@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2024

Codecov Report

Attention: Patch coverage is 44.05941% with 113 lines in your changes missing coverage. Please review.

Project coverage is 45.66%. Comparing base (272df9a) to head (44e9f3f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #469    +/-   ##
========================================
  Coverage   45.66%   45.66%            
========================================
  Files          72       78     +6     
  Lines        5961     6160   +199     
========================================
+ Hits         2722     2813    +91     
- Misses       2895     2997   +102     
- Partials      344      350     +6     
Files Coverage Δ
cmd/ocm-backplane/root.go 81.81% <100.00%> (+0.86%) ⬆️
pkg/cli/config/config.go 82.94% <100.00%> (+0.26%) ⬆️
pkg/healthcheck/mocks/httpClientMock.go 100.00% <100.00%> (ø)
pkg/healthcheck/mocks/networkMock.go 100.00% <100.00%> (ø)
cmd/ocm-backplane/login/login.go 68.14% <50.00%> (-0.11%) ⬇️
cmd/ocm-backplane/healthcheck/health_check.go 50.00% <50.00%> (ø)
cmd/ocm-backplane/cloud/console.go 21.78% <0.00%> (-1.14%) ⬇️
pkg/healthcheck/check_vpn.go 72.97% <72.97%> (ø)
pkg/healthcheck/check_proxy.go 36.58% <36.58%> (ø)
pkg/healthcheck/connectivity_checks.go 10.66% <10.66%> (ø)

... and 1 file with indirect coverage changes

@xiaoyu74 xiaoyu74 force-pushed the checking_vpn_proxy_availability branch 2 times, most recently from bd00543 to ae50918 Compare July 1, 2024 09:16
Copy link
Contributor

@samanthajayasinghe samanthajayasinghe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @xiaoyu74 .. It's good if you can add at least 50% unit tests to cover new sub command

cmd/ocm-backplane/healthCheck/health_check.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/healthCheck/health_check.go Outdated Show resolved Hide resolved
pkg/cli/config/config.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/healthCheck/connectivity_checks.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/healthCheck/connectivity_checks.go Outdated Show resolved Hide resolved
@xiaoyu74
Copy link
Contributor Author

xiaoyu74 commented Jul 7, 2024

/hold
Will need to discuss and investigate for this subcommand to work in the ocm-container.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 7, 2024
@xiaoyu74 xiaoyu74 force-pushed the checking_vpn_proxy_availability branch from 58f80a5 to ff801f7 Compare July 8, 2024 10:24
cmd/ocm-backplane/healthCheck/connectivity_checks.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/healthCheck/connectivity_checks.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/healthCheck/connectivity_checks.go Outdated Show resolved Hide resolved
cmd/ocm-backplane/login/login.go Outdated Show resolved Hide resolved
pkg/cli/config/config.go Outdated Show resolved Hide resolved
@xiaoyu74
Copy link
Contributor Author

/unhold

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2024
@samanthajayasinghe
Copy link
Contributor

Thanks @xiaoyu74 for moving base logic to /pkg/health and using mocks add unit tests ..
Can you pls update readMe file with the new command ?

| Command | Description |

and pls add a new section and explain how this command works

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
…vity

Fixing golang-lint issue

Refactor the HealthCheckCmd to move the logic in the Run to a separate func

Revert and keep proxyURL field only

Refactored code based on the review feedback

Added test cases

Added some more test cases to improve the coverage

Move the main logic to pkg and generate mock files via mockgen

Removed the default value of backplane configuration and added debug log

Add debug log for cloud console

Updated README.md with the healthcheck usage

Add utun as for checking VPN connectivity in MacOS
@xiaoyu74 xiaoyu74 force-pushed the checking_vpn_proxy_availability branch from dac1c9a to 44e9f3f Compare July 18, 2024 05:38
Copy link
Contributor

openshift-ci bot commented Jul 18, 2024

@xiaoyu74: 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.

@feichashao
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2024
Copy link
Contributor

openshift-ci bot commented Jul 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feichashao, xiaoyu74

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 [feichashao,xiaoyu74]

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

@openshift-merge-bot openshift-merge-bot bot merged commit 03a4452 into openshift:main Jul 18, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants