Skip to content

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Aug 10, 2025

Add networking validation functions for inbound port restriction and east-west connectivity

Summary

This PR implements two new networking validation functions in pkg/v1/networking:

  1. ValidateInboundPortRestriction - Validates that only SSH port (22) is accessible from external sources by testing common ports (21, 23, 25, 53, 80, 443, 993, 995, 3389, 5432, 3306) using netcat (nc) to ensure they're properly blocked.

  2. ValidateEastWestConnectivity - Creates two temporary test instances and validates inter-instance communication by testing ping connectivity and SSH port accessibility between instances using private IPs.

Both functions are integrated into the existing validation suite in internal/validation/suite.go and follow established patterns for instance lifecycle validation. The implementation includes proper error handling, cleanup mechanisms, and logging.

Key Fix: Resolved SSH authentication failures in CI by properly handling empty attrs.Name fields - providing a default "test-connectivity" name prevents malformed instance names like "-east" and "-west".

Review & Testing Checklist for Human

  • Manual validation testing: Run the full validation suite manually to verify both functions work correctly and don't break existing tests
  • Resource cleanup verification: Test failure scenarios (kill process mid-execution, network timeouts) to ensure test instances are properly cleaned up and don't incur unexpected costs
  • Port scanning accuracy: Verify the inbound port restriction validation actually catches real security violations by temporarily opening a non-SSH port and confirming the test fails
  • Cross-provider compatibility: Test the networking functions work correctly across different cloud providers since network configurations and nc behavior may vary

Recommended Test Plan:

  1. Run make test to verify no regressions in existing validation
  2. Manually execute the LambdaLabs validation tests to confirm both networking functions pass
  3. Monitor cloud provider dashboards during testing to verify proper instance cleanup
  4. Test with intentionally misconfigured instances to verify failure detection

Diagram

%%{ init : { "theme" : "default" }}%%
graph TD
    ValidationSuite["internal/validation/suite.go<br/>RunInstanceLifecycleValidation"]:::minor-edit
    NetworkingPkg["pkg/v1/networking.go<br/>New validation functions"]:::major-edit
    InstancePkg["pkg/v1/instance.go<br/>Existing instance functions"]:::context
    SSHPkg["pkg/ssh/ssh.go<br/>SSH connection utilities"]:::context
    
    ValidationSuite -->|"calls"| NetworkingPkg
    NetworkingPkg -->|"uses"| InstancePkg
    NetworkingPkg -->|"uses"| SSHPkg
    
    NetworkingPkg -->|"creates"| TestInstance1["Test Instance 1<br/>(temporary)"]:::context
    NetworkingPkg -->|"creates"| TestInstance2["Test Instance 2<br/>(temporary)"]:::context
    
    subgraph Legend
        L1["Major Edit"]:::major-edit
        L2["Minor Edit"]:::minor-edit  
        L3["Context/No Edit"]:::context
    end


classDef major-edit fill:#90EE90
classDef minor-edit fill:#87CEEB
classDef context fill:#FFFFFF
Loading

Notes

  • The east-west connectivity function creates real cloud instances during testing, which incurs actual costs and requires careful cleanup
  • The SSH authentication bug fix was critical - empty names were causing malformed instance names that broke SSH key association
  • Port scanning uses nc (netcat) which should be available on most cloud instance images but behavior may vary
  • Both functions follow the existing validation pattern of waiting for instances to be ready before testing
  • Proper error handling and cleanup is implemented but should be verified under failure conditions

Session Details:

- Add ValidateInboundPortRestriction to ensure only SSH port is accessible
- Add ValidateEastWestConnectivity to test inter-instance communication
- Follow existing validation patterns from instance.go and image.go
- Use SSH client for remote port scanning and connectivity testing
- Refactor into helper functions to meet linter requirements

Co-Authored-By: Alec Fong <[email protected]>
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Member

@theFong theFong left a comment

Choose a reason for hiding this comment

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

Lets wire these up in the internal/validation package

devin-ai-integration bot and others added 5 commits August 10, 2025 01:36
- Add ValidateInboundPortRestriction test after ValidateInstanceImage
- Add ValidateEastWestConnectivity test before ValidateTerminateInstance
- Follow existing validation suite patterns and error handling

Co-Authored-By: Alec Fong <[email protected]>
- Use makeDebuggableName() for test instance creation like ValidateCreateInstance
- Follow exact same pattern as working instance creation functions
- Ensures proper SSH key setup and unique naming for test instances

Co-Authored-By: Alec Fong <[email protected]>
- Remove explicit Name field to match ValidateCreateInstance pattern
- Let makeDebuggableName() handle naming internally during instance creation
- Ensures proper SSH key setup for test instances

Co-Authored-By: Alec Fong <[email protected]>
…ions

- Add client.GetInstance calls in waitForInstancesReady to refresh instances from API
- Follow the same pattern used in validation suite at line 108
- Ensures instances have complete SSH connection details before attempting connections
- Update function signature to return refreshed instances

Co-Authored-By: Alec Fong <[email protected]>
- Provide default 'test-connectivity' name when attrs.Name is empty
- Prevents malformed instance names like '-east' and '-west'
- Ensures proper SSH key association for test instances
- Fixes SSH authentication failure in CI tests

Co-Authored-By: Alec Fong <[email protected]>
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.

1 participant