Vsock: Add alternate http/https port in case 80/443 in use #3332

praveenkumar · 2022-09-05T08:29:13Z

This patch try to solve user issue where port 80 or 443 already in
use and crc start fails by having alternate port 8080 and 8443.

Note: Now user have to add those ports to application routes to access
it. Like https://myapp.apps-crc.testing:8443 or http://myapp.apps-crc.testing:8080

$ ./crc start
[...]
WARN Port 80 is already in use. Going to use 8080

$ curl --unix-socket ~/.crc/crc-http.sock http:/unix/network/services/forwarder/all | jq .
[
  {
    "local": "127.0.0.1:2222",
    "remote": "192.168.127.2:22",
    "protocol": "tcp"
  },
  {
    "local": "127.0.0.1:6443",
    "remote": "192.168.127.2:6443",
    "protocol": "tcp"
  },
  {
    "local": ":443",
    "remote": "192.168.127.2:443",
    "protocol": "tcp"
  },
  {
    "local": ":8080",
    "remote": "192.168.127.2:80",
    "protocol": "tcp"
  }
]

Fixes: Issue #3331

cfergeau · 2022-09-05T14:55:33Z

pkg/crc/machine/vsock.go

+func HTTPAndHTTPS() []types.ExposeRequest {
+	hPort, hsPort := httpPort, httpsPort
+	if isPortInUse(hPort) {
+		logging.Warnf("Port %s is already in use. Going to use %s", httpPort, alternateHTTPPort)


Using port %s instead might sound nicer.

Is this the very last warning which is printed? Or is this going to be lost in the middle of the startup logs of crc?

Going to use does not sound conclusive. Assigned %s instead, as use followed by using sounds repetitive.

Is this the very last warning which is printed? Or is this going to be lost in the middle of the startup logs of crc?

@cfergeau It is not the last warning, this is something part of info messages (Since it is warning so should've the WARN and different color)

cfergeau

Note: Now user have to add those ports to application routes to access
it. Like https://myapp.apps-crc.testing:8443 or http://myapp.apps-crc.testing:8080

This should be more than a note in the commit log, but part of the warning which is displayed to the user. This also needs to be mentioned in documentation.

cfergeau · 2022-09-05T14:56:17Z

pkg/crc/machine/vsock.go

+}
+
+func isPortInUse(port string) bool {
+	ln, err := net.Listen("tcp", ":"+port)


This could use fmt.Sprintf for consistency with the code before it

Fwiw, listening on port 80/443 is a privileged operation on some OS, I don't know if isPortInUse could fail because of this.

Fwiw, listening on port 80/443 is a privileged operation on some OS, I don't know if isPortInUse could fail because of this.

@cfergeau I checked with linux/mac running a python simple http server on port 80 and this is identified by this function.

cfergeau · 2022-09-05T14:56:40Z

pkg/crc/machine/vsock.go

+func HTTPAndHTTPS() []types.ExposeRequest {
+	hPort, hsPort := httpPort, httpsPort
+	if isPortInUse(hPort) {
+		logging.Warnf("Port %s is already in use. Going to use %s", httpPort, alternateHTTPPort)


Is this the very last warning which is printed? Or is this going to be lost in the middle of the startup logs of crc?

cfergeau · 2022-09-05T14:58:05Z

pkg/crc/machine/vsock.go

+			Protocol: "tcp",
+			Local:    fmt.Sprintf(":%s", hPort),
+			Remote:   net.JoinHostPort(virtualMachineIP, httpPort),
+		},


I'd have a preference for not putting http/https together to avoid code duplication.

exposeRequest = append(exposeRequest, requestWithAlternatePort(httpPort, alternateHTTPPort) exposeRequest = append(exposeRequest, requestWithAlternatePort(httpsPort, alternateHTTPSPort)

Alternate is not the right wording as that means that either can be assigned consecutively.

fallback would work.

gbraad · 2022-09-06T07:52:14Z

WARN Port 80 is already in use. Going to use 8080

WARN Port 80 is already in use. Assigned 8080 as alternative.

gbraad · 2022-09-06T07:54:25Z

pkg/crc/machine/vsock.go

+	internalSSHPort    = "22"
+	localIP            = "127.0.0.1"
+	httpPort           = "80"
+	alternateHTTPPort  = "8080"


most common use of alternate is 'every other'. better use alternative

gbraad

Wording

praveenkumar · 2022-09-08T05:31:48Z

Note: Now user have to add those ports to application routes to access
it. Like https://myapp.apps-crc.testing:8443 or http://myapp.apps-crc.testing:8080
This should be more than a note in the commit log, but part of the warning which is displayed to the user. This also needs to be mentioned in documentation.

@cfergeau Now this warning propagated to start. Let me know if we want this change in separate commit (like now) or you want to squash the last 2 commits.

anjannath · 2022-09-08T05:44:14Z

when a fallback port is chosen for HTTPS then the crc console command won't be giving the correct URL for the console, it'd be nice if crc console knew which port to use

something we can document or need to solve in a follow up pr

gbraad · 2022-09-08T07:00:38Z

knew which port to use

would need to be stored as part of the machine config as state :-/

anjannath · 2022-09-08T07:41:11Z

knew which port to use

would need to be stored as part of the machine config as state :-/

we can also get this information from the daemon, listing the exposed ports

praveenkumar · 2022-09-08T08:42:24Z

knew which port to use

would need to be stored as part of the machine config as state :-/

we can also get this information from the daemon, listing the exposed ports

yes I was going to use this solution but #3331 (comment) need discussion so putting that to hold.

/hold

praveenkumar · 2022-09-09T09:38:04Z

/unhold

pkg/crc/config/types.go

pkg/crc/config/validations.go

cfergeau · 2022-09-09T10:22:56Z

pkg/crc/machine/types/types.go

@@ -31,6 +31,10 @@ type StartConfig struct {

 	// Shared dirs
 	EnableSharedDirs bool
+
+	// Port to access openshift route


Ports to access OpenShift routes?

cfergeau · 2022-09-09T10:23:38Z

pkg/crc/machine/vsock.go

@@ -102,12 +102,12 @@ func vsockPorts(preset crcPreset.Preset) []types.ExposeRequest {
 			},
 			types.ExposeRequest{
 				Protocol: "tcp",
-				Local:    fmt.Sprintf(":%s", httpsPort),
+				Local:    fmt.Sprintf(":%s", strconv.FormatUint(uint64(ingressHTTPSPort), 10)),


:%d should work?

pkg/crc/machine/vsock.go

cfergeau · 2022-09-09T10:28:32Z

pkg/crc/machine/vsock.go

+	if userAssignedHTTPPort != httpPort {
+		fallbackPortWarning += fmt.Sprintf(fallbackPortWarningTmpl, userAssignedHTTPPort, "http", userAssignedHTTPPort)
+	}
+	return fallbackPortWarning


I don't really see the point of carrying this warning string all the way from pkg/crc/machine/vsock.go so that it can be displayed in cmd/crc/cmd/start.go. We could just check if ingress-http-port/ingress-https-port are set to their default values, and print the warning if not?

We could just check if ingress-http-port/ingress-https-port are set to their default values, and print the warning if not?

@cfergeau where you want to check it, in cmd/start.go ? If I do that I can't have it at the end as what we discussed previously so current implementation seemed clean to me.

The current implementation prints the message in cmd/start.go and manages to print it at the end, so I don't understand your objection?

The current implementation prints the message in cmd/start.go and manages to print it at the end, so I don't understand your objection?

@cfergeau current implementation works because we carry this warning from pkg/crc/machine/vsock.go or you are saying we just should should check that in pkg/crc/machine/start.go and return from there?

If done in pkg/crc/machine/start.go, I'd prefer to see expose it as fallbackHTTPPortInUse bool; fallbackHTTPSPortInUse bool;, and convert this to a warning in cmd/start.go. It's probably fine to not even do that, and check the config in cmd/start.go? Or does it have downsides?

cfergeau · 2022-09-09T10:30:27Z

pkg/crc/config/callbacks.go

@@ -32,3 +32,13 @@ func RequiresCRCSetup(key string, _ interface{}) string {
 	return fmt.Sprintf("Changes to configuration property '%s' are only applied during 'crc setup'.\n"+
 		"Please run 'crc setup' for this configuration to take effect.", key)
 }
+
+func RequiresHTTPPortChangeWarning(key string, value interface{}) string {
+	return fmt.Sprintf("Changes to configuration property '%s' will break the openshift route.\n"+


... will break OpenShift HTTP routes. In order to access OpenShift applications through HTTP URLs, the %d port must be manually specified, such as http://myapp.apps-crc.testing:%d

cfergeau · 2022-09-09T10:32:46Z

pkg/crc/config/callbacks.go

+}
+
+func RequiresHTTPSPortChangeWarning(key string, value interface{}) string {
+	return fmt.Sprintf("Changes to configuration property '%s' will break the openshift webconsole.\n"+


We need the same warning as in the HTTP case, plus something for the web console. After this change, the OpenShift console will be non-functional because of OpenShift limitations (dunno if it's possible to insert the URL of the web console in this message?)

We need the same warning as in the HTTP case, plus something for the web console. After this change, the OpenShift console will be non-functional because of OpenShift limitations

Sure, that we can do.

dunno if it's possible to insert the URL of the web console in this message?

Not at this moment because web console url is created from bundle metadata side.

cfergeau · 2022-09-09T10:33:25Z

pkg/crc/config/settings.go

@@ -107,6 +109,10 @@ func RegisterSettings(cfg *Config) {

 	cfg.AddSetting(KubeAdminPassword, "", ValidateString, SuccessfullyApplied,
 		"User defined kubeadmin password")
+	cfg.AddSetting(IngressHTTPPort, 80, ValidatePort, RequiresHTTPPortChangeWarning,
+		"Consent to use alternative http port in case 80 is in use (1024-65535, default: 80)")


Consent to use? "Alternative HTTP port to use for OpenShift ingress/routes in case port 80 is already in use on the host" ? But this could be too long.

Yeah I can put it, too long shouldn't be issue if it make more clear message to user.

cfergeau · 2022-09-09T10:38:46Z

pkg/crc/machine/vsock.go

+	fallbackPortWarningTmpl := `
+Note: Port %s is Assigned.
+You have to add this port to the application route to access
+it. like %s://myapp.apps-crc.testing:%s


to acces it. like %s .. -> to access it, such as %s://myapp.apps-crc.testing:%s

cmd/crc/cmd/start.go

cfergeau · 2022-09-15T08:58:28Z

cmd/crc/cmd/start.go

+	fallbackPortWarningTmpl := `
+Note: Port %d is Assigned.
+You have to add this port to the application route to access
+it, such as %s://myapp.apps-crc.testing:%d


The config warning uses slightly different terminology:

Changes to configuration property '%s' will break OpenShift HTTPS routes.\n"+ "In order to access OpenShift applications through HTTPS URLs "+ "the %d port must be manually specified, such as https://myapp.apps-crc.testing:%d\n"

" OpenShift HTTPS routes" and "OpenShift applications through HTTPS URLs" is used rather than "the application route"

Maybe Port %d is used for OpenShift HTTP routes instead of the default. You have to add this port to HTTP URLs when accessing OpenShift application, such as ... ?

cfergeau · 2022-09-15T08:59:36Z

pkg/crc/config/settings.go

+	cfg.AddSetting(IngressHTTPPort, constants.OpenShiftIngressHTTPPort, ValidatePort, RequiresHTTPPortChangeWarning,
+		fmt.Sprintf("Alternative HTTP port to use for OpenShift ingress/routes in case port 80 is already in use on the host (1024-65535, default: %d)", constants.OpenShiftIngressHTTPPort))
+	cfg.AddSetting(IngressHTTPSPort, constants.OpenShiftIngressHTTPSPort, ValidatePort, RequiresHTTPSPortChangeWarning,
+		fmt.Sprintf("Alternative HTTPS port to use for OpenShift ingress/routes in case port 443 is already in use on the host (1024-65535, default: %d)", constants.OpenShiftIngressHTTPSPort))


The in case port xx is already in use on the host is no longer accurate, the defined port will always be used.

cfergeau · 2022-09-15T09:01:02Z

cmd/crc/cmd/start.go

@@ -268,6 +276,9 @@ func writeOpenShiftTemplatedMessage(writer io.Writer, s *startResult) error {
 	if s.ClusterConfig.ClusterType == preset.OKD {
 		tmpl = fmt.Sprintf("%s\n\n%s", startTemplateForOpenshift, startTemplateForOKD)
 	}
+	if s.FallbackPortWarning != "" {
+		tmpl = fmt.Sprintf("%s\n%s", tmpl, fallbackPortWarning)
+	}


Do we get any benefits from having FallbackPortWarning in startResult rather than calling portFallbackWarning() here?

If it is not part of result then you will not get it as part of crc start -ojson, in case some other app want to make use of this warning to show then it can be used.

$ crc start -o json [...] "developerCredentials": { "username": "developer", "password": "developer" } }, "fallbackPortWarning": "\nNote: Port 8080 is Assigned.\nYou have to add this port to the application route to access\nit, such as http://myapp.apps-crc.testing:8080\n" }

If we want to expose this in json, this would look more useful than an arbitrary string:

[...] "developerCredentials": { "username": "developer", "password": "developer" } }, "alternativeIngressPorts": { "http": "8080", } }

What we want to expose is debatable, warning can be useful if the app just need to popup or even show in the terminal. Also this is the same warning what crc start cmd shows. If we just put alternativeIngressPorts then user of it need to put own interpretation/warning around it.

In my opinion, a preformatted string is almost never the right thing to have in an API, be it a json struct, a function return value, ... As soon as you need something different from the hardcoded string, you have to add awkward parsing code to get back the original data, and rebuild what you need from it.
One way forward is to not add anything to the json output for now, and to add 'something' when we have clear requirements for it.

@cfergeau Ack, I remove the warn string from json output now.

cfergeau · 2022-09-15T09:01:28Z

cmd/crc/cmd/start.go

+func portFallbackWarning() string {
+	var fallbackPortWarning string
+	fallbackPortWarningTmpl := `
+Note: Port %d is Assigned.


Why is the method called 'Warning' but the string is Note:? :)

@cfergeau I put the Warning instead Note.

This patch try to solve user issue where port 80 or 443 already in use and `crc start` fails by providing user an option as part of `crc config set` to have different http/https port. This patch also add callback function and startup warning in case port setting is choose by user. ``` $ crc config set ingress-http-port 8080 Changes to configuration property 'ingress-http-port' will break the openshift route. User need to add 8080 port to application route to access it. like http://myapp.apps-crc.testing:8080 $ crc start [...] Use the 'oc' command line interface: $ eval $(crc oc-env) $ oc login -u developer https://api.crc.testing:6443 Warning: Port 80 is already in use. Assigned port 8080 instead. You have to add thise port to the application route to access it. like http://myapp.apps-crc.testing:8080 $ curl --unix-socket ~/.crc/crc-http.sock http:/unix/network/services/forwarder/all | jq . [ { "local": "127.0.0.1:2222", "remote": "192.168.127.2:22", "protocol": "tcp" }, { "local": "127.0.0.1:6443", "remote": "192.168.127.2:6443", "protocol": "tcp" }, { "local": ":443", "remote": "192.168.127.2:443", "protocol": "tcp" }, { "local": ":8080", "remote": "192.168.127.2:80", "protocol": "tcp" } ] ```

openshift-ci · 2022-09-16T12:50:20Z

@praveenkumar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-crc	`04ce01c`	link	true	`/test e2e-crc`

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/test-infra repository. I understand the commands that are listed here.

cfergeau · 2022-09-19T08:04:04Z

cmd/crc/cmd/start.go

+	fallbackPortWarningTmpl := `
+Warning: Port %d is used for OpenShift %s routes instead of the default
+You have to add this port to %s URLs when accessing OpenShift application,
+such as %s://myapp.apps-crc.testing:%d


Warning that the console will be non-functional would be useful here as well. Especially as one message just before the warning is

The server is accessible via web console at: {{ .ClusterConfig.WebConsoleURL }}

A follow-up would be to remove this message when the https is overridden.

Address as a follow-up: #3350

openshift-ci · 2022-09-19T08:04:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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~~ [cfergeau]

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

openshift-ci bot requested review from cfergeau and gbraad September 5, 2022 08:29

cfergeau reviewed Sep 5, 2022

View reviewed changes

gbraad reviewed Sep 6, 2022

View reviewed changes

gbraad requested changes Sep 6, 2022

View reviewed changes

openshift-ci bot assigned gbraad Sep 6, 2022

praveenkumar force-pushed the issue_3331 branch from 2737a5f to ad04058 Compare September 8, 2022 05:28

praveenkumar force-pushed the issue_3331 branch from ad04058 to 695f2bd Compare September 8, 2022 07:16

openshift-ci bot added the do-not-merge/hold label Sep 8, 2022

praveenkumar force-pushed the issue_3331 branch from 695f2bd to 3c2d39c Compare September 8, 2022 13:56

openshift-ci bot removed the do-not-merge/hold label Sep 9, 2022

cfergeau reviewed Sep 9, 2022

View reviewed changes

praveenkumar force-pushed the issue_3331 branch from 3c2d39c to ad9b951 Compare September 12, 2022 10:46

anjannath reviewed Sep 13, 2022

View reviewed changes

cmd/crc/cmd/start.go Outdated Show resolved Hide resolved

praveenkumar force-pushed the issue_3331 branch from ad9b951 to 32ad43c Compare September 13, 2022 11:00

cfergeau reviewed Sep 15, 2022

View reviewed changes

praveenkumar force-pushed the issue_3331 branch from 32ad43c to 8cff2e0 Compare September 15, 2022 14:55

praveenkumar added the has-to-be-in-release This PR need to go in coming release. label Sep 16, 2022

praveenkumar force-pushed the issue_3331 branch from 8cff2e0 to 04ce01c Compare September 16, 2022 10:18

cfergeau approved these changes Sep 19, 2022

View reviewed changes

openshift-ci bot assigned cfergeau Sep 19, 2022

openshift-ci bot added the lgtm label Sep 19, 2022

openshift-ci bot added the approved label Sep 19, 2022

praveenkumar mentioned this pull request Sep 19, 2022

Remove console url when custom https port is set #3350

Open

praveenkumar merged commit 35ed30a into crc-org:main Sep 19, 2022

anjannath mentioned this pull request May 22, 2023

Allow for port override/reassignment when port 80 (or 22) is used by a local service on Windows #2760

Closed

Vsock: Add alternate http/https port in case 80/443 in use #3332

Vsock: Add alternate http/https port in case 80/443 in use #3332

Conversation

praveenkumar commented Sep 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Sep 6, 2022

Choose a reason for hiding this comment

gbraad left a comment

Choose a reason for hiding this comment

praveenkumar commented Sep 8, 2022

anjannath commented Sep 8, 2022

gbraad commented Sep 8, 2022

anjannath commented Sep 8, 2022

praveenkumar commented Sep 8, 2022

praveenkumar commented Sep 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfergeau Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 16, 2022

Choose a reason for hiding this comment

gbraad Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

openshift-ci bot commented Sep 19, 2022

cfergeau Sep 16, 2022 •

edited

Loading

gbraad Sep 19, 2022 •

edited

Loading