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

Fix API Server port detection issue by making sure the same local address is used for listening and checking if the port is available #7041

Conversation

rm3l
Copy link
Member

@rm3l rm3l commented Aug 25, 2023

What type of PR is this:
/kind bug
/area dev

What does this PR do / why we need it:
This ensures the same local address is used for listening and checking if a given port is free.
Otherwise, net.listen("tcp", ":$port") would listen on 0.0.0.0:$port (see output below),
and, on some operating systems like Windows 11, 127.0.0.1:$port is surprisingly considered free.

PS C:\Users\asoro> netstat -aon | grep 2000
  TCP    0.0.0.0:20000          0.0.0.0:0              LISTENING       11044
  TCP    127.0.0.1:20001        0.0.0.0:0              LISTENING       11044
  TCP    [::]:20000             [::]:0                 LISTENING       11044

This, as a consequence, made it impossible to run multiple Dev Sessions on Windows.

Using the same local address for listening and checking if the port is free would be safer.
If we decide to support passing a custom address to the API Server, we will use that address instead.

Which issue(s) this PR fixes:
Fixes #6721

PR acceptance criteria:

  • Unit test

  • Integration test

  • Documentation

How to test changes / Special notes to the reviewer:
It should be possible to start several Dev Sessions on Windows. See #6721 (comment)

This ensures the same local address is used for listening and checking if a given port is free.
Otherwise, `net.listen("tcp", ":$port")` would listen on `0.0.0.0:$port`,
and, on some operating systems like Windows 11, `127.0.0.1:$port` is surprisingly considered as free
(see output below). This, as a consequence, made it impossible to run multiple Dev Sessions on Windows.

```
PS C:\Users\asoro> netstat -aon | grep 2000
  TCP    0.0.0.0:20000          0.0.0.0:0              LISTENING       11044
  TCP    127.0.0.1:20001        0.0.0.0:0              LISTENING       11044
  TCP    [::]:20000             [::]:0                 LISTENING       11044
  TCP    [::1]:20000            [::1]:53656            ESTABLISHED     11044
  TCP    [::1]:53656            [::1]:20000            ESTABLISHED     9984
```

Using the same local address for listening and checking if the port is free would be safer.
If we decide to support passing a custom address, we would use that address instead.
@openshift-ci openshift-ci bot added kind/bug Categorizes issue or PR as related to a bug. area/dev Issues or PRs related to `odo dev` labels Aug 25, 2023
@netlify
Copy link

netlify bot commented Aug 25, 2023

Deploy Preview for odo-docusaurus-preview canceled.

Name Link
🔨 Latest commit f437061
🔍 Latest deploy log https://app.netlify.com/sites/odo-docusaurus-preview/deploys/64e8aab25987560008f68e02

@rm3l rm3l changed the title Fix API Server port detection issue by making sure the same local address is used for listening and checking if ports are available Fix API Server port detection issue by making sure the same local address is used for listening and checking if the port is available Aug 25, 2023
@openshift-ci openshift-ci bot requested review from feloy and kadel August 25, 2023 13:21
@rm3l rm3l removed the request for review from kadel August 25, 2023 13:23
@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

OpenShift Unauthenticated Tests on commit 30b1e8b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

NoCluster Tests on commit 30b1e8b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

Unit Tests on commit 30b1e8b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

Validate Tests on commit 30b1e8b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

Windows Tests (OCP) on commit 30b1e8b finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

OpenShift Tests on commit 30b1e8b finished with errors.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

Kubernetes Tests on commit 30b1e8b finished successfully.
View logs: TXT HTML

@odo-robot
Copy link

odo-robot bot commented Aug 25, 2023

Kubernetes Docs Tests on commit 5a3c33d finished successfully.
View logs: TXT HTML

Copy link
Contributor

@feloy feloy 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 this fix

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Aug 28, 2023
@rm3l
Copy link
Member Author

rm3l commented Aug 28, 2023

  [FAIL] odo create/delete/list/set namespace/project tests create namespace [It] should successfully create the namespace
  /go/odo_1/tests/helper/helper_cmd_wrapper.go:101

Test failing on OpenShift for similar reasons as in #7044
Let's run them again to see if the cleanup operation fixes the issue..

@rm3l rm3l closed this Aug 28, 2023
@rm3l rm3l reopened this Aug 28, 2023
@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rm3l
Copy link
Member Author

rm3l commented Aug 28, 2023

Summarizing 1 Failure:
  [FAIL] odo create/delete/list/set namespace/project tests when namespace is created with -w [It] should list the new namespace when listing namespace
  /go/odo_1/tests/integration/cmd_namespace_test.go:38

Ran 404 of 954 Specs in 1333.451 seconds
FAIL! -- 403 Passed | 1 Failed | 0 Pending | 550 Skipped

Failure to be addressed in #7044

/override OpenShift-Integration-tests/OpenShift-Integration-tests

@openshift-ci
Copy link

openshift-ci bot commented Aug 28, 2023

@rm3l: Overrode contexts on behalf of rm3l: OpenShift-Integration-tests/OpenShift-Integration-tests

In response to this:

Summarizing 1 Failure:
 [FAIL] odo create/delete/list/set namespace/project tests when namespace is created with -w [It] should list the new namespace when listing namespace
 /go/odo_1/tests/integration/cmd_namespace_test.go:38

Ran 404 of 954 Specs in 1333.451 seconds
FAIL! -- 403 Passed | 1 Failed | 0 Pending | 550 Skipped

Failure to be addressed in #7044

/override OpenShift-Integration-tests/OpenShift-Integration-tests

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/test-infra repository.

@rm3l
Copy link
Member Author

rm3l commented Aug 28, 2023

  [FAILED] Expected
      <*url.Error | 0xc0008ceea0>: 
      Post "http://127.0.0.1:49154/api/newuser": EOF
      {
          Op: "Post",
          URL: "http://127.0.0.1:49154/api/newuser",
          Err: <*errors.errorString | 0xc000108100>{s: "EOF"},
      }
  to be nil
  In [It] at: C:/Users/Administrator.ANSIBLE-TEST-VS/4509/tests/e2escenarios/e2e_test.go:430 @ 08/28/23 04:09:34.419
[...]
Summarizing 1 Failure:
  [FAIL] E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
  C:/Users/Administrator.ANSIBLE-TEST-VS/4509/tests/e2escenarios/e2e_test.go:430

Ran 4 of 4 Specs in 263.775 seconds
FAIL! -- 3 Passed | 1 Failed | 0 Pending | 0 Skipped

Unrelated flaky E2E test. Reported in #6582

/override windows-integration-test/Windows-test

@openshift-ci
Copy link

openshift-ci bot commented Aug 28, 2023

@rm3l: Overrode contexts on behalf of rm3l: windows-integration-test/Windows-test

In response to this:

 [FAILED] Expected
     <*url.Error | 0xc0008ceea0>: 
     Post "http://127.0.0.1:49154/api/newuser": EOF
     {
         Op: "Post",
         URL: "http://127.0.0.1:49154/api/newuser",
         Err: <*errors.errorString | 0xc000108100>{s: "EOF"},
     }
 to be nil
 In [It] at: C:/Users/Administrator.ANSIBLE-TEST-VS/4509/tests/e2escenarios/e2e_test.go:430 @ 08/28/23 04:09:34.419
[...]
Summarizing 1 Failure:
 [FAIL] E2E Test starting with non-empty Directory add Binding [It] should verify developer workflow of using binding as env in innerloop
 C:/Users/Administrator.ANSIBLE-TEST-VS/4509/tests/e2escenarios/e2e_test.go:430

Ran 4 of 4 Specs in 263.775 seconds
FAIL! -- 3 Passed | 1 Failed | 0 Pending | 0 Skipped

Unrelated flaky E2E test. Reported in #6582

/override windows-integration-test/Windows-test

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/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit a949230 into redhat-developer:main Aug 28, 2023
22 checks passed
@rm3l rm3l deleted the 6721-port-in-use-detection-on-windows branch August 28, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dev Issues or PRs related to `odo dev` kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port in use detection on Windows
3 participants