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

Use service port name as route targetPort in 'oc expose service' #6274

Merged
merged 1 commit into from
Dec 15, 2015
Merged

Use service port name as route targetPort in 'oc expose service' #6274

merged 1 commit into from
Dec 15, 2015

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Dec 11, 2015

Fixes #6265

There are 6 possible scenarios for services with ports we will hit in oc expose

  1. 1 unnamed service port -> numbered container port
  2. 1 unnamed service port -> named container port
  3. 1 named service port -> numbered container port
  4. 1 named service port -> named container port
  5. 2+ named service ports -> numbered container port
  6. 2+ named service ports -> named container port

Whenever a service port is named (cases 3,4,5,6), we should use the name as the route targetPort

When a service port is unnamed (which means it is the only port), we could try to set a targetPort that will point to the right containers, or we could omit the targetPort and let the route "round-robin" to the single port. In case 1 (numbered container port), we could set a numbered targetPort, but the route would break if the service was edited to set a different targetPort. In case 2 (named container port), we have no choice but to round-robin (container port names aren't available in the endpoints).

This PR updates oc expose service <svc> to generate a route that has a named targetPort when possible, and no targetPort otherwise.

@liggitt
Copy link
Contributor Author

liggitt commented Dec 11, 2015

@Kargakis PTAL

@liggitt
Copy link
Contributor Author

liggitt commented Dec 11, 2015

[test]

cmd.Flags().Set("port", port.Name)
} else {
// Otherwise, omit the targetPort from the route and let it round-robin the one service port
cmd.Flags().Set("omit-target-port", "true")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ugly, I don't see a better way to explicitly tell the route generator we want it to leave out the port

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we decide on using port vs target-port as the name of the parameter (and subsequently the name of the flag for setting the route port)? I think the intention is to use target-port. Then we will not require portString to not be zero in the generator, instead of introducing another field to explicitly pass info.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I agree, updated

@0xmichalis
Copy link
Contributor

You also need to update ParamsNames to include target-port (and drop port and ports)

@0xmichalis
Copy link
Contributor

You also need to update ParamsNames to include target-port (and drop port and ports)

And then update the unit tests for the route generator. I always forget those and being reminded by Jenkins is disturbing:)

@liggitt
Copy link
Contributor Author

liggitt commented Dec 14, 2015

updated tests and cmd tests

@@ -68,7 +68,8 @@ os::cmd::expect_success 'oc create -f test/integration/fixtures/test-service.jso
os::cmd::expect_failure 'oc expose service frontend --create-external-load-balancer'
os::cmd::expect_failure 'oc expose service frontend --port=40 --type=NodePort'
os::cmd::expect_success 'oc expose service frontend'
os::cmd::expect_success_and_text 'oc get route frontend' 'name=frontend'
os::cmd::expect_success_and_text "oc get route frontend --output-version=v1 --template='{{.spec.to.name}}'" "frontend" # routes to correct service
os::cmd::expect_success_and_text "oc get route frontend --output-version=v1 --template='{{.spec.port.targetPort}}'" "<no value>" # no target port for services with unnamed ports
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this <no value> what is really returned?

@0xmichalis
Copy link
Contributor

A question, LGTM in general

@liggitt
Copy link
Contributor Author

liggitt commented Dec 14, 2015

that's what template rendering returns when you reference something that doesn't exist in the struct

@0xmichalis
Copy link
Contributor

The command '/bin/sh -c yum install -y libmnl libnetfilter_conntrack openvswitch     libnfnetlink iptables iproute bridge-utils procps-ng ethtool socat openssl     binutils xz kmod-libs kmod sysvinit-tools device-mapper-libs dbus     && yum clean all' returned a non-zero code: 1
!!! Error in hack/build-images.sh:62
    'docker build -t $1:latest $2' exited with status 1
Call stack:
    1: hack/build-images.sh:62 image(...)
    2: hack/build-images.sh:81 main(...)
Exiting with status 1
make: *** [release] Error 1

[merge]

@liggitt thanks for this

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4334/) (Image: devenv-rhel7_2949)

@0xmichalis
Copy link
Contributor

#6312 flake

[merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 419cca5

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 419cca5

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7817/)

openshift-bot pushed a commit that referenced this pull request Dec 15, 2015
@openshift-bot openshift-bot merged commit 383a3be into openshift:master Dec 15, 2015
@liggitt liggitt deleted the expose_route branch December 15, 2015 14:45
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.

3 participants