Don't let users create endpoints inside ClusterNetworkCIDR #9383

danwinship · Jun 16, 2016

If a service has a selector then it's pointless to manually create endpoints for it, because the endpoints controller will (eventually) just overwrite your manually-created endpoints with the correct value according to the selector. ("Eventually" meaning 30 seconds at the most, or sooner if something related to the service changes before then.) Nonetheless, it's currently possible for a user to temporarily override the Endpoints with an incorrect value. This patch adds an admission controller that prevents that.

This is part of the fix for #9255; the only reason openshift-sdn needs to track IP-to-pod/namespace mappings is because it can't 100% reliably distinguish endpoints-that-point-to-pod-IPs that were added by the endpoints controller (which are always safe) from endpoints-that-point-to-pod-IPs that were added by users (which probably indicate an attempt to bypass isolation), so it has to check that each endpoint points into the right namespace. With the admission controller in place, we can just make it accept all auto-created endpoints and reject all manually-created ClusterNetwork-internal endpoints.

(#9255 talked about making the admission controller do the pointing-into-ClusterNetworkCIDR check itself, and then getting rid of openshift-sdn's endpoint filter entirely. However, it looks like we're going to need to keep the endpoint filter around for 1.3 to do egress-firewall-related endpoint filtering anyway, so we might as well keep the other endpoint filtering there as well and just go with the simpler admission controller at least for now. The discussion there also suggested allowing cluster admins to override the admission controller, but I didn't bother to do that here, since there isn't much point in allowing even admins to manually create these Endpoints given that they'll just get overwritten anyway.)

Not sure about the use of a.GetUserInfo().GetName() == bootstrappolicy.MasterUsername to identify the endpoints controller... (It works but I don't know if it's "right".)

@smarterclayton @openshift/networking PTAL

I like the concept and implementation.

knobunc · Jun 16, 2016

I like the concept and implementation.

liggitt · Jun 16, 2016

liggitt reviewed Jun 16, 2016

View changes

liggitt · Jun 16, 2016

pkg/service/admission/endpoint_admission.go

+		return apierrs.NewForbidden(a.GetResource().GroupResource(), ep.Name, fmt.Errorf("can't add explicit Endpoints to Service with a selector"))
+	}
+
+	return nil


I'm not seeing how this prevents a user from updating an endpoint to include questionable IPs...

Unfortunately this is racy and can't guarantee you observe the right
service state. A user can flip the service state rapidly and inject
endpoints. We probably have to do the whitelist on endpoints plus control
who can set them.

On Jun 16, 2016, at 12:07 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/service/admission/endpoint_admission.go
#9383 (comment):

// The Endpoint controller is always allowed to add Endpoints

if a.GetUserInfo().GetName() == bootstrappolicy.MasterUsername {

return nil

}
+

svc, err := r.kclient.Core().Services(a.GetNamespace()).Get(ep.Name)

if err != nil {

return apierrs.NewNotFound(a.GetResource().GroupResource(), ep.Name)

}
+

if svc.Spec.Selector != nil {

return apierrs.NewForbidden(a.GetResource().GroupResource(), ep.Name, fmt.Errorf("can't add explicit Endpoints to Service with a selector"))

}
+

return nil

I'm not seeing how this prevents a user from updating an endpoint to
include questionable IPs...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9383/files/b21ea8af78d57db74065dacec6a4bc8f93496912#r67373975,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p9qRkygJOBp7B5uhAc7VyHn3PZ2Xks5qMXSsgaJpZM4I3hI-
.

We want to do a policy check - will try to find a relevant example (the build config strategy admission controller does something similar)

I'm not seeing how this prevents a user from updating an endpoint to include questionable IPs...

The admission controller runs on both creates and updates, though I guess the error message text doesn't reflect that

Unfortunately this is racy and can't guarantee you observe the right service state. A user can flip the service state rapidly and inject endpoints.

My original idea had been to have the endpoints controller set a particular annotation on the Endpoints object, and then enforce that only the endpoints controller is allowed to set that annotation, and openshift-sdn would check for that to know whether to trust it. Is that any better?

I wasn't expecting a user to be able to point to any IP they want just because the service doesn't have a selector. I was expecting something to prevent regular users from setting endpoint IPs within the service or pod CIDRs

Yeah, that's the safer and more compact change - doesn't require us to
solve races.

On Jun 16, 2016, at 12:45 PM, Jordan Liggitt notifications@github.com
wrote:

In pkg/service/admission/endpoint_admission.go
#9383 (comment):

// The Endpoint controller is always allowed to add Endpoints

if a.GetUserInfo().GetName() == bootstrappolicy.MasterUsername {

return nil

}
+

svc, err := r.kclient.Core().Services(a.GetNamespace()).Get(ep.Name)

if err != nil {

return apierrs.NewNotFound(a.GetResource().GroupResource(), ep.Name)

}
+

if svc.Spec.Selector != nil {

return apierrs.NewForbidden(a.GetResource().GroupResource(), ep.Name, fmt.Errorf("can't add explicit Endpoints to Service with a selector"))

}
+

return nil

I wasn't expecting a user to be able to point to any IP they want just
because the service doesn't have a selector. I was expecting something to
prevent regular users from setting endpoint IPs within the service or pod
CIDRs

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9383/files/b21ea8af78d57db74065dacec6a4bc8f93496912#r67381183,
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p3vGWwVyHjYYmBgNTWvd3SzrrzHTks5qMX20gaJpZM4I3hI-
.

In some of our storage testing, we will create the endpoints manually to point to the external/internal glusterfs server. Seems this kind of testing will be blocked by the change. And not sure if the real users will also have this kind of requirement.

Our testing step like this:
oc create -f https://raw.githubusercontent.com/openshift-qe/v3-testfiles/master/persistent-volumes/gluster/security/endpoints_2.json

bmeng · Jun 17, 2016

In some of our storage testing, we will create the endpoints manually to point to the external/internal glusterfs server. Seems this kind of testing will be blocked by the change. And not sure if the real users will also have this kind of requirement.

Our testing step like this:
oc create -f https://raw.githubusercontent.com/openshift-qe/v3-testfiles/master/persistent-volumes/gluster/security/endpoints_2.json

@bmeng

@bmeng This only prevents you from creating endpoints on services that have a "selector" field; on those services, openshift will overwrite your endpoints at some unpredictable point in the future anyway, so that wouldn't be suitable for testing.

danwinship · Jun 17, 2016

@bmeng This only prevents you from creating endpoints on services that have a "selector" field; on those services, openshift will overwrite your endpoints at some unpredictable point in the future anyway, so that wouldn't be suitable for testing.

@liggitt

So the other proposal (which is not racy) is to make this two parts:

Check whether endpoints is over a "protected" range of IPs (disjoint sets, including ClusterNetworkCIDR and ServiceNetworkCIDR, but potentially others).
If the endpoints are protected, do a LSAR on a virtual subresource of endpoints (like "POST" on "endpoints/internal" or something, @liggitt can recommend better). See pkg/build/admission/strategyrestrictions/admission.go#checkBuildConfigAuthorization for a rough analogue.
Give the endpoints controller that "endpoints/internal" "POST" permission.

That ensures that admins can set any external endpoints they like, admins can grant to specific users, and regular users can't set.

smarterclayton · Jun 17, 2016

So the other proposal (which is not racy) is to make this two parts:

Check whether endpoints is over a "protected" range of IPs (disjoint sets, including ClusterNetworkCIDR and ServiceNetworkCIDR, but potentially others).
If the endpoints are protected, do a LSAR on a virtual subresource of endpoints (like "POST" on "endpoints/internal" or something, @liggitt can recommend better). See pkg/build/admission/strategyrestrictions/admission.go#checkBuildConfigAuthorization for a rough analogue.
Give the endpoints controller that "endpoints/internal" "POST" permission.

That ensures that admins can set any external endpoints they like, admins can grant to specific users, and regular users can't set.

@smarterclayton

@smarterclayton Can you explain 'LSAR' and how it would be used in this context? I'm not familiar with it and I'm having trouble finding a good explanation.

marun · Jun 17, 2016

@smarterclayton Can you explain 'LSAR' and how it would be used in this context? I'm not familiar with it and I'm having trouble finding a good explanation.

@smarterclayton

It's a local subject access review - subject access review is basically a
removable "ACL check" that the component invokes to check access to
something.

On Fri, Jun 17, 2016 at 11:07 AM, Maru Newby notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton Can you explain
'LSAR' and how it would be used in this context? I'm not familiar with it
and I'm having trouble finding a good explanation.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9383 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6nXkMHpxTHh8EAQqW9Xoxic6eEEks5qMrgmgaJpZM4I3hI-
.

smarterclayton · Jun 17, 2016

It's a local subject access review - subject access review is basically a
removable "ACL check" that the component invokes to check access to
something.

On Fri, Jun 17, 2016 at 11:07 AM, Maru Newby notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton Can you explain
'LSAR' and how it would be used in this context? I'm not familiar with it
and I'm having trouble finding a good explanation.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9383 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/ABG_p6nXkMHpxTHh8EAQqW9Xoxic6eEEks5qMrgmgaJpZM4I3hI-
.

OK, branch updated (and PR renamed):

It now works as originally suggested, blocking unauthorized users from adding endpoints in ClusterNetworkCIDR and ServiceNetworkCIDR.
- There's currently no config option to add other CIDR blocks to this list. I could add it, but what's the use case? It seems like if a user can connect to the IP directly, then they should be able to create a service pointing to it as well; and if they can't connect to the IP directly (for some reason) then the admin shouldn't have to configure any additional thing to make it so they can't connect via a service...
The endpoints controller is explicitly given permission to bypass the check.
- Do I need to do something to upgrade existing clusters to add this permission?
- Although I didn't add it on purpose, cluster admins seem to get to bypass the check by default too. (I guess because the ClusterAdminRoleName role applies to Resources("*")? So this is a feature, not a bug?)
Existing endpoints are not revalidated if you change ClusterNetworkCIDR, since we can't tell at that point whether the person who added them would have had permission. So that should be documented.
No tests yet but I'll add some once you're happy with the rest of it

danwinship · Jun 21, 2016

OK, branch updated (and PR renamed):

It now works as originally suggested, blocking unauthorized users from adding endpoints in ClusterNetworkCIDR and ServiceNetworkCIDR.
- There's currently no config option to add other CIDR blocks to this list. I could add it, but what's the use case? It seems like if a user can connect to the IP directly, then they should be able to create a service pointing to it as well; and if they can't connect to the IP directly (for some reason) then the admin shouldn't have to configure any additional thing to make it so they can't connect via a service...
The endpoints controller is explicitly given permission to bypass the check.
- Do I need to do something to upgrade existing clusters to add this permission?
- Although I didn't add it on purpose, cluster admins seem to get to bypass the check by default too. (I guess because the ClusterAdminRoleName role applies to Resources("*")? So this is a feature, not a bug?)
Existing endpoints are not revalidated if you change ClusterNetworkCIDR, since we can't tell at that point whether the person who added them would have had permission. So that should be documented.
No tests yet but I'll add some once you're happy with the rest of it

liggitt · Jun 21, 2016

liggitt reviewed Jun 21, 2016

View changes

liggitt · Jun 21, 2016

pkg/cmd/server/bootstrappolicy/constants.go

@@ -70,6 +70,8 @@ const (
 	BuildStrategySourceRoleName          = "system:build-strategy-source"
 	BuildStrategyJenkinsPipelineRoleName = "system:build-strategy-jenkinspipeline"

+	RestrictedEndpointsAdmissionRoleName = "system:endpoints-admission"


instead of this, we want a role, associated with a service account, that actually includes all the permissions the endpoints controller needs. I assume that is get/list/watch access to services and pods, full access to endpoints, and now create access to our virtual endpoints/restricted resource (or whatever we call it)

liggitt · Jun 21, 2016

liggitt reviewed Jun 21, 2016

View changes

liggitt · Jun 21, 2016

pkg/cmd/server/bootstrappolicy/policy.go

@@ -607,6 +607,14 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule("get").Groups(projectGroup).Resources("projects").RuleOrDie(),
 			},
 		},
+		{


move the definition and binding to infra_sa_policy.go, and follow the pattern there... we define a service account name, role name, and permissions we expect that service account to have

liggitt · Jun 21, 2016

liggitt reviewed Jun 21, 2016

View changes

liggitt · Jun 21, 2016

pkg/service/admission/endpoint_admission.go

+	}
+
+	resource := kapi.Resource(authorizationapi.RestrictedEndpointsResource)
+	subjectAccessReview := authorizationapi.AddUserToLSAR(a.GetUserInfo(),


since 99% of restricted writes will be from the endpoints controller it would be nice to avoid checking this permission on the same user super frequently... @deads2k, do we have an LRU/expiring cache around SAR results anywhere else?

since 99% of restricted writes will be from the endpoints controller it would be nice to avoid checking this permission on the same user super frequently... @deads2k, do we have an LRU/expiring cache around SAR results anywhere else?

No, we don't. What about using the WantsAuthorizer at least to avoid the API server call and keep everything in-cache?

+1, forgot about that

@danwinship, have your admission plugin implement a method like this:

func (o *podNodeConstraints) SetAuthorizer(a authorizer.Authorizer) { o.authorizer = a }

add a check to make sure you conform to the WantsAuthorizer interface:

var _ = oadmission.WantsAuthorizer(&restrictedEndpointsAdmission{})

then, rather than an API call to do the access check, you can check against the same authorizer the API guards itself with (see podNodeConstraints#checkPodsBindAccess for an example how to do this)

Do I need to do something to upgrade existing clusters to add this permission?

If you define it as an infra service account role, the new role, role binding, and service account will be created automatically on startup

liggitt · Jun 21, 2016

Do I need to do something to upgrade existing clusters to add this permission?

If you define it as an infra service account role, the new role, role binding, and service account will be created automatically on startup

Although I didn't add it on purpose, cluster admins seem to get to bypass the check by default too. (I guess because the ClusterAdminRoleName role applies to Resources("*")? So this is a feature, not a bug?)

That's a feature... * on * means root access. For this, we'll want to switch the endpoints controller to run using a service account (new controllers all get their own service account, and we've been switching old ones piecemill)

liggitt · Jun 21, 2016

Although I didn't add it on purpose, cluster admins seem to get to bypass the check by default too. (I guess because the ClusterAdminRoleName role applies to Resources("*")? So this is a feature, not a bug?)

That's a feature... * on * means root access. For this, we'll want to switch the endpoints controller to run using a service account (new controllers all get their own service account, and we've been switching old ones piecemill)

@liggitt

@liggitt updated to use a service account

danwinship · Jun 22, 2016

@liggitt updated to use a service account

liggitt · Jun 22, 2016

liggitt reviewed Jun 22, 2016

View changes

liggitt · Jun 22, 2016

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+				// Managing endpoints
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("get", "list", "create", "update", "delete"),


do we know if the endpoints controller uses patch?

It doesn't. If the endpoint exists but is wrong it just overwrites it with Update().

liggitt · Jun 22, 2016

liggitt reviewed Jun 22, 2016

View changes

liggitt · Jun 22, 2016

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+					Verbs:     sets.NewString("create"),
+					Resources: sets.NewString("endpoints/restricted"),
+				},
+			},


does the endpoints controller need permissions to create/get events?

I don't think so? There's nothing relevant about "events" in endpoints_controller.go, and it doesn't log any errors with the current set of permissions.

pravisankar · Jun 23, 2016

pravisankar reviewed Jun 23, 2016

View changes

pravisankar · Jun 23, 2016

pkg/service/admission/endpoint_admission.go

+func (r *restrictedEndpointsAdmission) Admit(a kadmission.Attributes) error {
+	if a.GetResource().GroupResource() != kapi.Resource("endpoints") {
+		return nil
+	}


We want only create and update endpoints to perform these checks?

if (a.GetOperation() != admission.Create) && (a.GetOperation() != admission.Update) { return nil }

Ignore, I see L49: kadmission.NewHandler(kadmission.Create, kadmission.Update) does the trick

pravisankar · Jun 23, 2016

pravisankar reviewed Jun 23, 2016

View changes

pravisankar · Jun 23, 2016

pkg/service/admission/endpoint_admission.go

+	subjectAccessReview := authorizationapi.AddUserToLSAR(a.GetUserInfo(),
+		&authorizationapi.LocalSubjectAccessReview{
+			Action: authorizationapi.AuthorizationAttributes{
+				Verb:         "create",


This could be create or update...Verb: strings.ToLower(a.GetOperation())?

No, this isn't a real resource that actually gets created/updated; it's a fake resource that exists only to have permissions checked against it. (This is basically copied from the build strategy admission controller.)

pravisankar · Jun 23, 2016

pravisankar reviewed Jun 23, 2016

View changes

pravisankar · Jun 23, 2016

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+				// Permission for RestrictedEndpointsAdmission
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("create"),


Don't we want to allow "update" here?

pravisankar · Jun 23, 2016

pravisankar reviewed Jun 23, 2016

View changes

pravisankar · Jun 23, 2016

pkg/service/admission/endpoint_admission.go

+const RestrictedEndpointsPluginName = "RestrictedEndpointsAdmission"
+
+func init() {
+	kadmission.RegisterPlugin("RestrictedEndpointsAdmission", func(client clientset.Interface, config io.Reader) (kadmission.Interface, error) {


Why do we need to register if we are explicitly adding and initializing the plugin
(https://github.com/danwinship/origin/blob/13cc03e2f32c23521035a4ac1ba0bc157e834e07/pkg/cmd/server/kubernetes/master_config.go#L60 and https://github.com/danwinship/origin/blob/13cc03e2f32c23521035a4ac1ba0bc157e834e07/pkg/cmd/server/kubernetes/master_config.go#L192)

I don't know. Possibly we don't, but all of the other explicitly-created admission controller types do it too.

LGTM, seems like all the other comments have already been addressed with code or replies, right?

dcbw · Jun 23, 2016

LGTM, seems like all the other comments have already been addressed with code or replies, right?

there's the question of perf impact of doing an API call on every update by the endpoints controller: #9383 (comment)

liggitt · Jun 23, 2016

there's the question of perf impact of doing an API call on every update by the endpoints controller: #9383 (comment)

@liggitt

@liggitt updated to use authorizer.Authorize()

danwinship · Jun 27, 2016

@liggitt updated to use authorizer.Authorize()

Thanks, looks close. This will prevent a user from making an unrelated change (like adding an annotation) to an Endpoints object created by the endpoints controller. To avoid blocking that, I think we'd need to pay attention to the existing addresses on the old object during an update, and only restrict new/modified addresses. That depends on an enhancement to the info available to admission which we'll get in the next rebase, so I think a follow-up would be ok there.

Can you add an integration test that makes sure the cluster admin and endpoints controller service account can create endpoints with whatever addresses they want, and a project admin is denied permission for addresses in the service/cluster ranges, and allowed outside those ranges?

liggitt · Jun 27, 2016

Thanks, looks close. This will prevent a user from making an unrelated change (like adding an annotation) to an Endpoints object created by the endpoints controller. To avoid blocking that, I think we'd need to pay attention to the existing addresses on the old object during an update, and only restrict new/modified addresses. That depends on an enhancement to the info available to admission which we'll get in the next rebase, so I think a follow-up would be ok there.

Can you add an integration test that makes sure the cluster admin and endpoints controller service account can create endpoints with whatever addresses they want, and a project admin is denied permission for addresses in the service/cluster ranges, and allowed outside those ranges?

also needs hack/update-generated-docs.sh run

liggitt · Jun 27, 2016

also needs hack/update-generated-docs.sh run

liggitt · Jun 29, 2016

liggitt reviewed Jun 29, 2016

View changes

liggitt · Jun 29, 2016

test/integration/endpoint_admission_test.go

+	}
+}
+
+func TestEndpointAdmissionClusterAdmin(t *testing.T) {


test setup is a little expensive (~7-10 seconds per test on Jenkins), so I would unify the tests to a single Test* method and do this as a table test instead

LGTM pending integration test combining (and needs a rebase). can you open a follow up issue to track allowing updates unrelated to address changes

liggitt · Jun 29, 2016

LGTM pending integration test combining (and needs a rebase). can you open a follow up issue to track allowing updates unrelated to address changes

liggitt · Jun 29, 2016

liggitt reviewed Jun 29, 2016

View changes

liggitt · Jun 29, 2016

pkg/authorization/api/synthetic.go

@@ -10,4 +10,6 @@ const (
 	NodeMetricsResource = "nodes/metrics"
 	NodeStatsResource   = "nodes/stats"
 	NodeLogResource     = "nodes/log"
+
+	RestrictedEndpointsResource = "endpoints/restricted"


@deads2k any better ideas for the virtual resource to do authz checks against to cover restricted endpoint addresses?

@deads2k any better ideas for the virtual resource to do authz checks against to cover restricted endpoint addresses?

This seems fine.

do we want this enabled by default? (I think we do)
do we need a way to disable it? (not sure we do)

we need release notes calling out the behavior change (some users might not be able to create the same endpoints they used to be able to, without additional permission grants), and documentation about how to give users the ability to create restricted endpoints

liggitt · Jun 29, 2016

do we want this enabled by default? (I think we do)
do we need a way to disable it? (not sure we do)

we need release notes calling out the behavior change (some users might not be able to create the same endpoints they used to be able to, without additional permission grants), and documentation about how to give users the ability to create restricted endpoints

[Test]ing while waiting on the merge queue

openshift-bot · Jun 30, 2016

[Test]ing while waiting on the merge queue

failures appear to be spurious network/jenkins issues

danwinship · Jul 1, 2016

failures appear to be spurious network/jenkins issues

LGTM, re[merge]

knobunc · Jul 8, 2016

LGTM, re[merge]

test_pull_requests_origin_check failure is #9371
test_pull_requests_origin_conformance failure is #9681
re-[merge]

danwinship · Jul 11, 2016

test_pull_requests_origin_check failure is #9371
test_pull_requests_origin_conformance failure is #9681
re-[merge]

9681 again... [test]

danwinship · Jul 11, 2016

9681 again... [test]

Flake was #9780

smarterclayton · Jul 11, 2016

Flake was #9780

Admission now has the appropriate info for mutations (saw some of the earlier comments).

smarterclayton · Jul 11, 2016

Admission now has the appropriate info for mutations (saw some of the earlier comments).

rebased, and added a fixup commit (to be squashed) using OldObject to allow non-IP-related endpoint modifications, and a corresponding update to the integration test

danwinship · Jul 12, 2016

rebased, and added a fixup commit (to be squashed) using OldObject to allow non-IP-related endpoint modifications, and a corresponding update to the integration test

Flake in #9803

smarterclayton · Jul 12, 2016

Flake in #9803

Another flake #9804

smarterclayton · Jul 12, 2016

Another flake #9804

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6169/) (Image: devenv-rhel7_4576)

openshift-bot · Jul 12, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6169/) (Image: devenv-rhel7_4576)

Evaluated for origin merge up to 6afa180

openshift-bot · Jul 12, 2016

Evaluated for origin merge up to 6afa180

Evaluated for origin test up to 6afa180

openshift-bot · Jul 12, 2016

Evaluated for origin test up to 6afa180

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

openshift-bot · Jul 12, 2016

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

um... I had assumed that pushing new commits would cancel the openshift-bot merge request... so anyway, this got merged with the change to allow unpriviliged users to still make non-IP-address-related updates to restricted Endpoints objects, but I don't think anyone ever reviewed that new code. FYI.

danwinship · Jul 13, 2016

um... I had assumed that pushing new commits would cancel the openshift-bot merge request... so anyway, this got merged with the change to allow unpriviliged users to still make non-IP-address-related updates to restricted Endpoints objects, but I don't think anyone ever reviewed that new code. FYI.

it's just the 3-4 lines that sees if we have an old object with identical subsets? that seems fine, but a test to make sure non-admins can annotate/label an endpoints object like that would be good

liggitt · Jul 13, 2016

it's just the 3-4 lines that sees if we have an old object with identical subsets? that seems fine, but a test to make sure non-admins can annotate/label an endpoints object like that would be good

Yes, I added that too: https://github.com/openshift/origin/pull/9383/files#diff-d4f8c3b0d30600cf18dc3508614f09fcR115

danwinship · Jul 13, 2016

Yes, I added that too: https://github.com/openshift/origin/pull/9383/files#diff-d4f8c3b0d30600cf18dc3508614f09fcR115

ah, missed that. looks fine

liggitt · Jul 13, 2016

ah, missed that. looks fine

danwinship changed the title from Don't let users manually create endpoints for services with selectors to Don't let users create endpoints inside ClusterNetworkCIDR Jun 21, 2016

danwinship referenced this pull request Jun 30, 2016
Merged
Document the endpoint admission controller #2443

openshift-bot added needs-rebase and removed needs-rebase labels Jul 11, 2016

openshift-bot added the needs-rebase label Jul 11, 2016

Add a service account for the endpoints controller

ad70f32

openshift-bot removed the needs-rebase label Jul 12, 2016

danwinship deleted the danwinship:endpoint-admission branch Jul 13, 2016

This was referenced Jul 21, 2016

Merged

Drop cluster-internal endpoint filtering / pod monitoring #9982

Closed

OCP 3.3 release notes tracker openshift/openshift-docs#2507

openshift/origin

Join GitHub today

Don't let users create endpoints inside ClusterNetworkCIDR #9383

Conversation

danwinship commented Jun 16, 2016

This comment has been minimized.

knobunc commented Jun 16, 2016

liggitt reviewed Jun 16, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

smarterclayton via email Jun 16, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bmeng commented Jun 17, 2016

This comment has been minimized.

danwinship commented Jun 17, 2016

This comment has been minimized.

smarterclayton commented Jun 17, 2016

This comment has been minimized.

marun commented Jun 17, 2016

This comment has been minimized.

smarterclayton commented Jun 17, 2016

danwinship changed the title from Don't let users manually create endpoints for services with selectors to Don't let users create endpoints inside ClusterNetworkCIDR Jun 21, 2016

This comment has been minimized.

danwinship commented Jun 21, 2016

liggitt reviewed Jun 21, 2016 View changes

This comment has been minimized.

liggitt reviewed Jun 21, 2016 View changes

This comment has been minimized.

liggitt reviewed Jun 21, 2016 View changes

This comment has been minimized.

liggitt Jun 21, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

liggitt commented Jun 21, 2016

This comment has been minimized.

liggitt commented Jun 21, 2016

This comment has been minimized.

danwinship commented Jun 22, 2016

liggitt reviewed Jun 22, 2016 View changes

This comment has been minimized.

This comment has been minimized.

liggitt reviewed Jun 22, 2016 View changes

This comment has been minimized.

liggitt Jun 22, 2016 • edited

This comment has been minimized.

pravisankar reviewed Jun 23, 2016 View changes

This comment has been minimized.

pravisankar Jun 23, 2016 • edited

This comment has been minimized.

pravisankar reviewed Jun 23, 2016 View changes

This comment has been minimized.

This comment has been minimized.

pravisankar reviewed Jun 23, 2016 View changes

This comment has been minimized.

pravisankar reviewed Jun 23, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dcbw commented Jun 23, 2016

This comment has been minimized.

liggitt commented Jun 23, 2016

This comment has been minimized.

danwinship commented Jun 27, 2016

This comment has been minimized.

liggitt commented Jun 27, 2016

This comment has been minimized.

liggitt commented Jun 27, 2016 • edited

liggitt reviewed Jun 29, 2016 View changes

This comment has been minimized.

liggitt Jun 29, 2016 • edited

This comment has been minimized.

liggitt commented Jun 29, 2016

liggitt reviewed Jun 29, 2016 View changes

This comment has been minimized.

liggitt reviewed Jun 16, 2016

View changes

liggitt reviewed Jun 21, 2016

View changes

liggitt reviewed Jun 21, 2016

View changes

liggitt reviewed Jun 21, 2016

View changes

liggitt Jun 21, 2016 •

edited

liggitt reviewed Jun 22, 2016

View changes

liggitt reviewed Jun 22, 2016

View changes

liggitt Jun 22, 2016 •

edited

pravisankar reviewed Jun 23, 2016

View changes

pravisankar Jun 23, 2016 •

edited

pravisankar reviewed Jun 23, 2016

View changes

pravisankar reviewed Jun 23, 2016

View changes

pravisankar reviewed Jun 23, 2016

View changes

liggitt commented Jun 27, 2016 •

edited

liggitt reviewed Jun 29, 2016

View changes

liggitt Jun 29, 2016 •

edited

liggitt reviewed Jun 29, 2016

View changes