add ingress admission controller to restrict hostname updates #12653

JacobTanenbaum · 2017-01-24T20:32:36Z

This ensures that openshift's rules regarding who can serve a hostname does not
get violated by ingress objects. If upstream behavior is desired this controller can be
disabled in the master config file.

I wrote this assuming that we might have other needs for an admission controller with ingress
objects and it should not be difficult to extend this one.

Trello Card

JacobTanenbaum · 2017-01-24T20:32:54Z

@knobunc I believe this is ready for review

knobunc · 2017-01-25T16:04:15Z

Please fix the title:

Typo "ingress_s_"
Shorten it so it conforms to the preferred git style and doesn't get truncated. Perhaps "add ingress admission controller to restrict hostname updates"

knobunc

Nice start

knobunc · 2017-01-25T16:05:56Z

pkg/cmd/server/origin/master_config.go

@@ -69,6 +69,7 @@ import (
 	imageadmission "github.com/openshift/origin/pkg/image/admission"
 	imagepolicy "github.com/openshift/origin/pkg/image/admission/imagepolicy/api"
 	imageapi "github.com/openshift/origin/pkg/image/api"
+	ingressadmit "github.com/openshift/origin/pkg/ingress/admission"


Why is this ingressadmit when the rest seem to be ...admission?

knobunc · 2017-01-25T16:07:01Z

pkg/ingress/admission/api/types.go

+
+// IngressAdmissionConfig is the configuration for the the ingress
+// controller limiter plugin. It changes the behavior of ingress
+//objects to behave better with openshift routes and routers.


We seem to be missing a space here after the //

knobunc · 2017-01-25T16:07:06Z

pkg/ingress/admission/api/types.go

+type IngressAdmissionConfig struct {
+	unversioned.TypeMeta
+
+	//UpstreamHostnameUpdate when true causes updates that attempt


We seem to be missing a space here after the //

knobunc · 2017-01-25T16:07:48Z

pkg/ingress/admission/api/types.go

+// controller limiter plugin. It changes the behavior of ingress
+//objects to behave better with openshift routes and routers.
+//*NOTE* Disabling this plugin causes ingress objects to behave
+//the same as in upstream kubernetes


I think we need to say that disabling this has potential security implications in the router when handling ingress objects.

knobunc · 2017-01-25T16:09:10Z

pkg/ingress/admission/api/types.go

+
+	//UpstreamHostnameUpdate when true causes updates that attempt
+	//to add or modify hostnames to succeed. Otherwise those updates
+	//fail in order to ensure hostname behavior


This needs to be clarified to explain that we don't allow addition or renaming of hostnames in the ingress object when set to false. If set to true then we allow the changes, but that has security implications.

What is the default?

booleans declared but not initialized are given false in golang. The code is structured so that if other options are added to IngressAdmissionConfig without enabling hostname updates they fail

knobunc · 2017-01-25T16:11:16Z

pkg/ingress/admission/api/v1/types.go

+	"k8s.io/kubernetes/pkg/api/unversioned"
+)
+
+// IngressAdmissionConfig is the configuration for the the ingress


Same comments here as earlier file

knobunc · 2017-01-25T16:12:35Z

pkg/ingress/admission/ingress_admission.go

+	return nil
+}
+
+func checkHostnames(oldIngress, newIngress *kextensions.Ingress) bool {


Can we call this haveHostsChanged? Or something that makes the actual test more clear.

knobunc · 2017-01-25T16:15:25Z

pkg/ingress/admission/ingress_admission.go

+}
+
+func checkHostnames(oldIngress, newIngress *kextensions.Ingress) bool {
+	m := make(map[string]int)


Any reason to use an int rather than a bool?

Actually, an empty struct appears even better since then it can optimize away to 0 bytes for the value.

But... Kubernetes has sets.NewString() that makes a set of strings... perhaps we should use that?

does the sets.NewString() apply here? wouldn't I have to loop through the Ingress.Spec.Rules[] to extract the host strings?

knobunc · 2017-01-25T16:18:36Z

pkg/ingress/admission/ingress_admission.go

+
+	for _, element := range newIngress.Spec.Rules {
+		_, present := m[element.Host]
+		if !present {


Is it better style to do:
if _, present := m[element.Host]; present {

knobunc · 2017-01-25T16:19:14Z

pkg/ingress/admission/ingress_admission_test.go

+				},
+			}
+		} else {
+			//Used to test deleteing a hostname


JacobTanenbaum · 2017-01-30T14:39:10Z

@openshift/networking

JacobTanenbaum · 2017-01-30T14:40:43Z

@liggitt @derekwaynecarr Please review

liggitt · 2017-01-30T18:08:49Z

pkg/ingress/admission/ingress_admission.go

+
+func haveHostnamesChanged(oldIngress, newIngress *kextensions.Ingress) bool {
+	m := make(map[string]int)
+	for _, element := range oldIngress.Spec.Rules {


what about TLS.Hosts?

Ingress rule hosts need to be immutable for the same reason as route hosts (prevent theft). TLS hosts are only used to match TLS configuration to a rule which doesn't need to be restricted (it's like changing tls conf for a route).

DirectXMan12

a couple of minor nits

DirectXMan12 · 2017-01-31T19:56:29Z

pkg/ingress/admission/ingress_admission.go

+}
+
+func haveHostnamesChanged(oldIngress, newIngress *kextensions.Ingress) bool {
+	m := make(map[string]int)


the standard convention for sets is to use map[TYPE]struct{}, but even better is k8s.io/apimachinery/pkg/utils/sets.String, which is a pre-built string set with the operations that you want.

DirectXMan12 · 2017-01-31T19:57:31Z

pkg/ingress/admission/ingress_admission_test.go

+			newHost:  "foo.com",
+			oldHost:  "bar.com",
+			testName: "Upstream Hostname updates enabled",
+		},


should you also test adding a hostname?

JacobTanenbaum · 2017-02-01T18:58:02Z

[test]

knobunc · 2017-02-02T14:51:20Z

[test]

knobunc · 2017-02-02T16:39:45Z

--- FAIL: TestAdmissionPluginNames (0.00s)
	admissionconfig_test.go:94: openshift admission plugins must be prefixed with openshift.io/ IngressAdmission

…name field. This ensures that openshift's rules regarding who can serve a hostname does not get violated by ingress objects. If upstream behavior is desired this controller can be disabled in the master config file. I wrote this assuming that we might have other needs for an admission controller with ingress objects and it should not be difficult to extend this one.

… test for adding a hostname

github.com/openshift/origin/pkg/cmd/server/origin.TestAdmissionPluginNames github.com/openshift/origin/pkg/cmd/server/start.TestAdmissionOnOffCoverage

knobunc · 2017-02-03T15:43:01Z

@DirectXMan12 can you look over the recent change please?

knobunc · 2017-02-03T15:44:18Z

FAILURE: Generated docs up to date, but generated man pages out of date. Please run hack/update-generated-docs.sh

openshift-bot · 2017-02-03T16:29:49Z

Evaluated for origin test up to dfd9fae

openshift-bot · 2017-02-03T17:53:27Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13570/) (Base Commit: 01edb43)

openshift-bot · 2017-02-03T17:53:37Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13570/) (Base Commit: 01edb43)

DirectXMan12

LGTM 👍

knobunc · 2017-02-03T19:57:25Z

[merge]

openshift-bot · 2017-02-03T20:05:42Z

Evaluated for origin merge up to dfd9fae

openshift-bot · 2017-02-04T09:29:28Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/13592/) (Base Commit: c74f1b8) (Image: devenv-rhel7_5856)

lihongan · 2017-02-09T10:25:50Z

could you please give the example master config of disabling the ingress admission?

smarterclayton · 2017-02-20T20:33:19Z

Remember to squash these in the future - didn't catch it at the time, but could have been reduced to 1 or 2 commits.

knobunc requested changes Jan 25, 2017

View reviewed changes

JacobTanenbaum changed the title ~~add admission controller to restrict updates to ingresss objects host…~~ add ingress admission controlle rto restrict hostname updates Jan 26, 2017

JacobTanenbaum changed the title ~~add ingress admission controlle rto restrict hostname updates~~ add ingress admission controller to restrict hostname updates Jan 26, 2017

JacobTanenbaum force-pushed the IngressAdmissionController branch from f3ee6cd to e4bd6b4 Compare January 27, 2017 11:02

liggitt reviewed Jan 30, 2017

View reviewed changes

knobunc requested a review from DirectXMan12 January 31, 2017 18:45

DirectXMan12 suggested changes Jan 31, 2017

View reviewed changes

JacobTanenbaum force-pushed the IngressAdmissionController branch from 097e6e5 to 2366482 Compare February 1, 2017 01:41

knobunc approved these changes Feb 2, 2017

View reviewed changes

JacobTanenbaum force-pushed the IngressAdmissionController branch from 0a38587 to 4d3e590 Compare February 2, 2017 18:02

JacobTanenbaum added 7 commits February 2, 2017 14:28

updated generated docs

4de290c

added swagger docs and verified the gofmt

720dab1

took out the decription in api/v1/types.go because govet.sh complained

8cffcc2

addresses Ben's initial comments

2695daa

Changed using a map to k8s.io/apimachinery/pkg/utils/sets and added a…

be1017e

… test for adding a hostname

Attempt at resolving

8c59f37

github.com/openshift/origin/pkg/cmd/server/origin.TestAdmissionPluginNames github.com/openshift/origin/pkg/cmd/server/start.TestAdmissionOnOffCoverage

JacobTanenbaum force-pushed the IngressAdmissionController branch from 4d3e590 to 8c59f37 Compare February 2, 2017 19:29

updated man page

dfd9fae

DirectXMan12 approved these changes Feb 3, 2017

View reviewed changes

openshift-bot merged commit ff53795 into openshift:master Feb 4, 2017

add ingress admission controller to restrict hostname updates #12653

add ingress admission controller to restrict hostname updates #12653

Conversation

JacobTanenbaum commented Jan 24, 2017 • edited Loading

JacobTanenbaum commented Jan 24, 2017

knobunc commented Jan 25, 2017

knobunc 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

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

JacobTanenbaum commented Jan 30, 2017

JacobTanenbaum commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacobTanenbaum commented Feb 1, 2017

knobunc commented Feb 2, 2017

knobunc commented Feb 2, 2017

knobunc commented Feb 3, 2017

knobunc commented Feb 3, 2017

openshift-bot commented Feb 3, 2017

openshift-bot commented Feb 3, 2017

openshift-bot commented Feb 3, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

knobunc commented Feb 3, 2017

openshift-bot commented Feb 3, 2017

openshift-bot commented Feb 4, 2017 • edited Loading

lihongan commented Feb 9, 2017

smarterclayton commented Feb 20, 2017

JacobTanenbaum commented Jan 24, 2017 •

edited

Loading

openshift-bot commented Feb 4, 2017 •

edited

Loading