-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
The --ports flag does not modify dc env variables #13816
Conversation
@knobunc PTAL |
@rajatchopra PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May want to specify that this will not apply to external appliance based routers such as f5.
@rajatchopra @knobunc PTAL |
How is this related to host-network=true/false? |
@@ -276,7 +276,7 @@ func NewCmdRouter(f *clientcmd.Factory, parentName, name string, out, errout io. | |||
cmd.Flags().StringVar(&cfg.ForceSubdomain, "force-subdomain", "", "A router path format to force on all routes used by this router (will ignore the route host value)") | |||
cmd.Flags().StringVar(&cfg.ImageTemplate.Format, "images", cfg.ImageTemplate.Format, "The image to base this router on - ${component} will be replaced with --type") | |||
cmd.Flags().BoolVar(&cfg.ImageTemplate.Latest, "latest-images", cfg.ImageTemplate.Latest, "If true, attempt to use the latest images for the router instead of the latest release.") | |||
cmd.Flags().StringVar(&cfg.Ports, "ports", cfg.Ports, "A comma delimited list of ports or port pairs to expose on the router pod. The default is set for HAProxy. Port pairs are applied to the service and to host ports (if specified).") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service isn't right is it? It controls what goes into the pod spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears in the router pod containerPort and hostPort:
ports:
- containerPort: 1080
hostPort: 1080
protocol: TCP
It also appears in the service:
exampleport 172.30.138.192 1080/TCP,1935/TCP,1936/TCP 4m
How do you want to phrase this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A comma delimited list of ports or port pairs that set the port in the router pod containerPort and hostPort. These are also used as the service port and targetPort when the router service is created. Note: this does not modify what the router will actually listen on. That can be done by editing the router environment variables. This is meaningful when host-network=false."
Bah. Wrong button. @rajatchopra it only matters when hostNetwork is false because this controls the port mappings in the pod spec that docker sets up. And those only matter when hostNetwork is true. At least, that's what I think is the case. @pecameron can you post the difference between things when --ports is set vs. when it is not? |
@knobunc --ports sets the port in the router pod containerPort and hostPort. It also sets service port and targetPort. |
@knobunc What are we going to do with this? |
--ports is not intended to modify the env variables. That can be done using oc env or by editing the router's dc. Rather it sets the ports that are exposed in the router's service. The help message is confusing and misleading. This change rewords the help text. bug 1420543 https://bugzilla.redhat.com/show_bug.cgi?id=1420543
@knobunc PTAL |
@knobunc Is this ready to go? |
@openshift/networking FYI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Will merge on Monday. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
[merge] |
[merge] |
[merge] last flaked on #14043 (logs: https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1153/) |
Evaluated for origin merge up to 0d4d887 |
continuous-integration/openshift-jenkins/merge FAILURE (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1162/) (Base Commit: 6bbad88) (PR Branch Commit: 0d4d887) |
--ports is not intended to modify the env variables. That can
be done using oc env or by editing the router's dc. Rather it
sets the ports that are exposed in the router's service.
The help message is confusing and misleading. This change
rewords the help text.
bug 1420543
https://bugzilla.redhat.com/show_bug.cgi?id=1420543