oc: add create route subcommands #6819

0xmichalis · 2016-01-25T16:59:55Z

Add three subcommands under oc create route that create secure
routes.

oc expose will continue creating unsecured routes as it already
does but we will not enhance it further.

@deads2k @liggitt @pweil- @ramr @smarterclayton @jwforres @fabianofranz

Fixes #6094

deads2k · 2016-01-25T18:27:31Z

WIP because I need the rebase to land first in order to use StructuredGenerators.

Do we really want structured generators? I didn't see a lot of value to having them.

0xmichalis · 2016-01-25T18:33:41Z

Do we really want structured generators? I didn't see a lot of value to having them.

We don't necessarily need them here but we had a long discussion about generators in general and I suppose we want to switch to using structured.

deads2k · 2016-01-25T18:40:40Z

We don't necessarily need them here but we had a long discussion about generators in general and I suppose we want to switch to using structured.

I'd like to leave them out until we need them or until someone here can present a strong use-case for them.

0xmichalis · 2016-01-25T18:53:52Z

ok, I will switch to the already existing generator interface

deads2k · 2016-01-25T19:02:01Z

pkg/cmd/cli/cmd/create_route.go

+}
+
+// CreateEdgeRoute implements the behavior to run the create edge route command.
+func CreateEdgeRoute(f *clientcmd.Factory, cmdOut io.Writer, cmd *cobra.Command, args []string) error {


I would have expected a helper to get the certs in the correct format and a straight assignments, so something more like:

route := routeapi.Route{} route.Name = args[0] route.TLS.Termination = edge route.TLS.CertFile = readCert(cert flag) routeClient.Namespace(ns).Create(route)

0xmichalis · 2016-01-26T17:25:53Z

@deads2k not sure why but my subcommands won't inherit the flags from create route. I had a look at Derek's create secret code and he seemed to have the same issue

0xmichalis · 2016-01-26T17:26:00Z

[test]

deads2k · 2016-01-26T18:00:30Z

@Kargakis there's an "add global flag" or some such, right?

fabianofranz · 2016-01-26T18:17:34Z

pkg/util/file/fileutil.go

+}
+
+// LoadCert reads the specified certificate file and returns it as a bytes array.
+func LoadCert(file string) ([]byte, error) {


Do you plan to add any logic specific to loading cert and key files here, like structure validation or something similar? Otherwise I don't see the valid in this being specific, should just use a generic loader like LoadData or LoadFileData.

s/valid/value

Do you plan to add any logic specific to loading cert and key files here, like structure validation or something similar? Otherwise I don't see the valid in this being specific, should just use a generic loader like LoadData or LoadFileData.

Nope, this is just a refactor of the oadm ~~registry~~ router code so that I won't duplicate logic. I will rename to LoadData

0xmichalis · 2016-01-27T13:47:03Z

all comments addressed, ptal

0xmichalis · 2016-01-27T13:51:23Z

pkg/cmd/cli/cmd/create_route.go

+	cmd.Flags().String("cert-file", "", "Provide certificate contents.")
+	cmd.Flags().String("key-file", "", "Provide key contents.")
+	cmd.Flags().String("ca-cert-file", "", "Provide cert authority certificate contents.")
+	cmd.Flags().String("dest-ca-cert", "", "Provide the contents of the ca certificate of the final destination.")


Note that this is --dest-ca-cert and not --dest-ca-cert-file. I think we can use --cert, --key, and ca-cert for the rest of the flags. @fabianofranz @deads2k @liggitt

Note that this is --dest-ca-cert and not --dest-ca-cert-file. I think we can use --cert, --key, and ca-cert for the rest of the flags. @fabianofranz @deads2k @liggitt

Is the help correct? Looks like its pointing at a file? I don't mind dropping the -file suffix, but I would like to have the help be a little more specific about exactly what kind of a file we need and a followup issue to actually parse the certs and keys.

Is the help correct? Looks like its pointing at a file?

Yes, its' correct. How about --dest-ca-cert=CERTIFICATE? Or CERT, CERTIFICATE_LOCATION?

I'm also ok with dropping the -file suffix, but the flag description must be clear about what it expects, like "Path to a certificate file for TLS..." or something similar. You could also add them to the usage, where we use FILENAME when flags are expected to get the path to a file. Example: ... [--dest-ca-cert=FILENAME].

@fabianofranz square brackets mean the flag is optional, right? --dest-ca-cert is required for reencrypt routes so I didn't include them in the usage message.

Why is it required?

Why is it required?

We require destination certificates in reencrypt routes

origin/pkg/route/api/validation/validation.go

Line 118 in a1c70de

result = append(result, fielderrors.NewFieldInvalid("destinationCACertificate", tls.DestinationCACertificate, "edge termination does not support destination certificates"))

Wrong link

this is it

origin/pkg/route/api/validation/validation.go

Line 95 in a1c70de

result = append(result, fielderrors.NewFieldRequired("destinationCACertificate"))

0xmichalis · 2016-01-28T13:48:31Z

adding tests

deads2k · 2016-01-28T18:54:32Z

pkg/cmd/cli/cmd/create_route.go

+	if err != nil {
+		return err
+	}
+	mapper, typer := f.Object()


From here down this method is icky. We should make this easier to use, but I don't see it as a need for this pull.

deads2k · 2016-01-28T18:59:19Z

pkg/cmd/cli/cmd/create_route.go

+// unsecuredRoute will return a route with enough info so that it can direct traffic to
+// the service provided by --expose. Callers of this helper are responsible for providing
+// tls configuration, path, and the hostname of the route.
+func unsecuredRoute(kc *kclient.Client, namespace, routeName, serviceName, portString string) (*api.Route, error) {


much more readable, thanks.

deads2k · 2016-01-28T19:00:56Z

Minor comments. I'd like to get someone on the UI team to declare --expose or --service. Otherwise, lgtm.

fabianofranz · 2016-01-29T19:31:59Z

+1 for --service then LGTM

0xmichalis · 2016-01-30T14:29:35Z

changed to --service and made --port mandatory in case of exposing a non-existent service

Any other comments?

Add three subcommands under `oc create route` that create secure routes. `oc expose` will continue creating unsecured routes as it already does but we will not enhance it further.

0xmichalis · 2016-01-31T14:32:46Z

#6818
#6031
flakes

re[test]

0xmichalis · 2016-01-31T16:10:03Z

one clean run, one new-app flake

[test]

smarterclayton · 2016-02-01T02:37:33Z

--service, not --expose. The create commands are about creating the
object, the object references a service.

On Sun, Jan 31, 2016 at 9:32 AM, Michail Kargakis notifications@github.com
wrote:

#6818 #6818
#6031 #6031
flakes

re[test]

—
Reply to this email directly or view it on GitHub
#6819 (comment).

openshift-bot · 2016-02-01T02:40:12Z

Evaluated for origin test up to a555a35

openshift-bot · 2016-02-01T03:55:13Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/630/)

fabianofranz · 2016-02-02T14:30:21Z

LGTM

0xmichalis · 2016-02-02T16:00:58Z

[merge]

openshift-bot · 2016-02-02T16:03:27Z

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

openshift-bot · 2016-02-02T16:03:27Z

Evaluated for origin merge up to a555a35

Merged by openshift-bot

This was referenced Jan 25, 2016

Add oc create route subcommands docs openshift/openshift-docs#1274

Merged

oc: generate path-based routes in expose #6056

Merged

deads2k reviewed Jan 25, 2016
View reviewed changes

0xmichalis changed the title ~~[WIP] oc: Add create route subcommands~~ oc: add create route subcommands Jan 26, 2016

fabianofranz reviewed Jan 26, 2016
View reviewed changes

0xmichalis reviewed Jan 27, 2016
View reviewed changes

deads2k reviewed Jan 28, 2016
View reviewed changes

oc: add create route subcommands

a555a35

Add three subcommands under `oc create route` that create secure routes. `oc expose` will continue creating unsecured routes as it already does but we will not enhance it further.

openshift-bot pushed a commit that referenced this pull request Feb 2, 2016

Merge pull request #6819 from kargakis/create-route-subcommand

1647ee4

Merged by openshift-bot

openshift-bot merged commit 1647ee4 into openshift:master Feb 2, 2016

0xmichalis deleted the create-route-subcommand branch February 2, 2016 18:12

oc: add create route subcommands #6819

oc: add create route subcommands #6819

Conversation

0xmichalis commented Jan 25, 2016

deads2k commented Jan 25, 2016

0xmichalis commented Jan 25, 2016

deads2k commented Jan 25, 2016

0xmichalis commented Jan 25, 2016

Choose a reason for hiding this comment

0xmichalis commented Jan 26, 2016

0xmichalis commented Jan 26, 2016

deads2k commented Jan 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jan 27, 2016

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

smarterclayton Jan 27, 2016 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xmichalis commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jan 28, 2016

fabianofranz commented Jan 29, 2016

0xmichalis commented Jan 30, 2016

0xmichalis commented Jan 31, 2016

0xmichalis commented Jan 31, 2016

smarterclayton commented Feb 1, 2016

openshift-bot commented Feb 1, 2016

openshift-bot commented Feb 1, 2016

fabianofranz commented Feb 2, 2016

0xmichalis commented Feb 2, 2016

openshift-bot commented Feb 2, 2016

openshift-bot commented Feb 2, 2016