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

adding option '--insecure-policy' for passthrough and reencrypt routes #12725

Merged
merged 1 commit into from Feb 3, 2017

Conversation

JacobTanenbaum
Copy link
Contributor

PR11953 added support for insecureEdgeTerminationPolicy to reencrypt and passthrough routes,
so it should be supported in the CLI as well.

Bug: 1403155 Link

@JacobTanenbaum
Copy link
Contributor Author

@openshift/networking

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM (with the caveat that we will open a new bug about the validation not happening on the CLI side & --dry-run -o yaml doesn't output an object)

@knobunc
Copy link
Contributor

knobunc commented Jan 31, 2017

[test] @ramr PTAL

@JacobTanenbaum
Copy link
Contributor Author

JacobTanenbaum commented Jan 31, 2017

The BZ that knobunc mentioned above BZ1418023

Copy link
Contributor

@ramr ramr left a comment

Choose a reason for hiding this comment

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

Minor typo otherwise LGTM

@@ -33,6 +33,10 @@ Specify the service (either just its name or using type/name syntax) that the ge
Set a hostname for the new route

.PP
\fB\-\-insecure\-policy\fP=""
Set an insecure for the new route
Copy link
Contributor

Choose a reason for hiding this comment

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

Set an insecure policy (missing that word) for the new route.

@@ -198,6 +198,7 @@ func NewCmdCreatePassthroughRoute(fullName string, f *clientcmd.Factory, out io.
kcmdutil.AddDryRunFlag(cmd)
cmd.Flags().String("hostname", "", "Set a hostname for the new route")
cmd.Flags().String("port", "", "Name of the service port or number of the container port the route will route traffic to")
cmd.Flags().String("insecure-policy", "", "Set an insecure for the new route")
Copy link
Contributor

Choose a reason for hiding this comment

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

Set an insecure policy (missing keyword!) for the new route.

@ramr
Copy link
Contributor

ramr commented Feb 1, 2017

@knobunc as I commented on the other PR - adding some of these checks on client side could be problematic. One is that older cli clients would behave differently vis-a-vis newer cli clients + what happens when the server validation changes say for a list of choices from [foo, bar] to [foo, bar, baz] or even [one, two]? The older cli client checks would be invalid at that point or worse yet even block valid values.

@knobunc
Copy link
Contributor

knobunc commented Feb 2, 2017

[merge]

… for CLI

PR11953 added support for insecureEdgeTerminationPolicy to reencrypt and passthrough routes,
so it should be supported in the CLI as well.

Bug: 1403155
@JacobTanenbaum
Copy link
Contributor Author

[test]

@JacobTanenbaum
Copy link
Contributor Author

@openshift/cli-review PTAL

@knobunc
Copy link
Contributor

knobunc commented Feb 2, 2017

[test]

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to daa215c

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13527/) (Base Commit: 89ed457)

@juanvallejo
Copy link
Contributor

LGTM

@smarterclayton
Copy link
Contributor

We need less client side validation, and more serverside. Clients should not make decisions about valid values.

@knobunc
Copy link
Contributor

knobunc commented Feb 3, 2017

Ok, based on @smarterclayton's feedback, I have closed the above mentioned bug since we are functioning as designed.

@knobunc
Copy link
Contributor

knobunc commented Feb 3, 2017

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to daa215c

@openshift-bot
Copy link
Contributor

openshift-bot commented Feb 3, 2017

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13581/) (Base Commit: c773aad) (Image: devenv-rhel7_5849)

@openshift-bot openshift-bot merged commit 2c1513f into openshift:master Feb 3, 2017
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.

None yet

6 participants