Register OpenShift resources to API groups #12986

mfojtik · 2017-02-16T10:12:29Z

This is a work-in-progress PR that registers the OpenShift types into API groups.

guangxuli · 2017-02-16T11:15:53Z

test/integration/master_routes_test.go

 	knet "k8s.io/kubernetes/pkg/util/net"

+	build "github.com/openshift/origin/pkg/build/api"
+	buildv1 "github.com/openshift/origin/pkg/build/api/v1"


buildapi and buildapiv1 may be more appropriate.

guangxuli · 2017-02-16T11:17:12Z

pkg/deploy/api/v1/defaults_test.go

@@ -12,7 +12,6 @@ import (
 	"k8s.io/kubernetes/pkg/util/diff"
 	"k8s.io/kubernetes/pkg/util/intstr"

-	v1 "github.com/openshift/origin/pkg/api/v1"
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
 	_ "github.com/openshift/origin/pkg/deploy/api/install"
 	deployv1 "github.com/openshift/origin/pkg/deploy/api/v1"


deployapi and deployapiv1 may be more appropriate.

guangxuli · 2017-02-16T11:28:28Z

test/integration/master_routes_test.go

+	found := false
+	for _, g := range groups.Groups {
+		if g.Name == buildv1.GroupName {
+			found = true


add a break directly

guangxuli · 2017-02-16T12:15:03Z

test/integration/master_routes_test.go

@@ -171,6 +175,124 @@ func TestWellKnownOAuthOff(t *testing.T) {
 	}
 }

+func TestApiGroups(t *testing.T) {
+	testutil.RequireEtcd(t)


Split this test function into three function TestBuildApiGroup TestDeployApiGroup TestTemplateApiGroup maybe better, and add another common test config function. :)

@guangxuli mind if all this refactoring will be a follow up? don't want to increase the diff size of already big change :-)

pweil- · 2017-02-16T13:24:06Z

@bparees @knobunc @legionus @enj FYI since this affects api types in your areas

bparees · 2017-02-16T18:44:07Z

pkg/build/api/v1/register.go

@@ -7,7 +7,7 @@ import (

 const GroupName = ""

-// SchemeGroupVersion is group version used to register these objects
+// LegacySchemeGroupVersion is group version used to register these objects
 var SchemeGroupVersion = unversioned.GroupVersion{Group: GroupName, Version: "v1"}


godoc misaligned w/ field name (i'm not sure which is correct....)

bparees · 2017-02-16T18:48:40Z

please make sure the extended builds tests get run on this PR before it merges.

mfojtik · 2017-02-16T20:22:01Z

@bparees this is a long way to be testable :)

mfojtik · 2017-02-20T16:57:11Z

@sttts @deads2k this should now include all api groups for all origin resources. Besides broken tests, there are couple things that are worrying me:

When I delete project, the default clusterPolicies are removed and breaking the cluster. I suspect the namespace finalizer it not working properly.
The impersonification is not working (Error from server (Forbidden): User "test-admin" cannot "impersonate" "user.openshift.io.systemusers" with name "system:admin" in project ""). I suspect the newRESTMapper and the "rootScoped" resources...

@smarterclayton @liggitt PTAL if you have any comments/concerns/etc. before I go on the journey of fixing all tests...

mfojtik · 2017-02-20T17:08:00Z

pkg/api/latest/latest.go

@@ -21,7 +21,20 @@ var OldestVersion = unversioned.GroupVersion{Group: "", Version: "v1"}
 // may be assumed to be most preferred to least preferred, and clients may
 // choose to prefer the earlier items in the list over the latter items when presented
 // with a set of versions to choose.
-var Versions = []unversioned.GroupVersion{{Group: "", Version: "v1"}}
+var Versions = []unversioned.GroupVersion{


@deads2k is there any less-uglier way to determine if a resource is "openshift"?

(or this will go away once we have client-go that includes k8s client as well?).

I don't even remember what this is used for. I thought we used reflection to find out if it was an openshift type.

i found this in our client that use this to distinguish between kube and origin resources...

mfojtik · 2017-02-20T21:37:59Z

@sttts ./pkg/api/... unit tests are working \o/

mfojtik · 2017-02-20T21:55:20Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -133,7 +133,7 @@ func GetBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				},
 			},
 			Rules: []authorizationapi.PolicyRule{
-				authorizationapi.NewRule("impersonate").Groups(kapiGroup).Resources(authorizationapi.SystemUserResource).Names(SystemAdminUsername).RuleOrDie(),
+				authorizationapi.NewRule("impersonate").Groups(authzGroup, legacyAuthzGroup, userGroup).Resources(authorizationapi.SystemUserResource).Names(SystemAdminUsername).RuleOrDie(),


@deads2k PTAL (i think authzGroup is not required, what fixed this was addition of userGroup)

@deads2k PTAL (i think authzGroup is not required, what fixed this was addition of userGroup)

Our system users kind should be grouped, so the new code is probably overly loose.

mfojtik · 2017-02-21T14:27:51Z

[test]

smarterclayton · 2017-02-22T02:51:09Z

pkg/deploy/api/test/ok.go

@@ -240,7 +240,7 @@ func OkHPAForDeploymentConfig(config *deployapi.DeploymentConfig, min, max int)
 		Spec: autoscaling.HorizontalPodAutoscalerSpec{
 			ScaleTargetRef: autoscaling.CrossVersionObjectReference{
 				Name: config.Name,
-				Kind: "DeploymentConfig",
+				Kind: "DeploymentConfig.deploy.openshift.io",


Um... this is wrong, Kind does not get group versions appended to it.

CrossVersionObjectReference should have a Group field.

yeah.. it only have APIVersion

I will open upstream issue

I'm pretty sure that's the API version field that has the group and version squashed together with a slash :-(

sttts · 2017-02-22T10:48:56Z

pkg/build/api/v1/register.go

 	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes, addConversionFuncs, addDefaultingFuncs)
 	AddToScheme   = SchemeBuilder.AddToScheme
 )

-// Adds the list of known types to api.Scheme.
+// addKnownTypes adds types to API group


... adds the API group types to the scheme

sttts · 2017-02-22T12:48:59Z

pkg/image/api/install/apigroup.go

@@ -14,11 +17,17 @@ func init() {
 			GroupName:              api.GroupName,
 			VersionPreferenceOrder: []string{v1.SchemeGroupVersion.Version},


why not here?

sttts · 2017-02-22T12:49:30Z

pkg/image/api/install/apigroup.go

@@ -14,11 +17,17 @@ func init() {
 			GroupName:              api.GroupName,
 			VersionPreferenceOrder: []string{v1.SchemeGroupVersion.Version},
 			ImportPrefix:           importPrefix,
-			//AddInternalObjectsToScheme: api.AddToScheme,
+			AddInternalObjectsToScheme: func(scheme *runtime.Scheme) error {
+				docker10.AddToScheme(scheme)


aggregate errors... ok for testing

mfojtik · 2017-02-22T15:28:55Z

@deads2k @liggitt @sttts @smarterclayton squashed and prettified for easier reviewing. all unit tests should be green by now.

mfojtik · 2017-02-22T20:00:55Z

(starting integration manually: https://ci.openshift.redhat.com/jenkins/view/Origin%20Test%20Jobs/job/test_pull_requests_origin_integration/11177/console)

smarterclayton · 2017-03-17T00:28:54Z

For admission, why don't we put something early in the chain that collapses groups to core for now and slate a follow up?

liggitt · 2017-03-17T01:07:55Z

For admission, why don't we put something early in the chain that collapses groups to core for now and slate a follow up?

Mutable admission attributes aren't tech debt I'm eager to take on

smarterclayton · 2017-03-17T01:14:40Z

And if we push to prod? I'll take a targeted protection for cohabitation over a cluster compromise... On Mar 16, 2017, at 9:08 PM, Jordan Liggitt <[email protected]> wrote: For admission, why don't we put something early in the chain that collapses groups to core for now and slate a follow up? Mutable admission attributes aren't tech debt I'm eager to take on — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12986 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p2jWAtZH7jJ7Ctw95MIvtdfBtYv9ks5rmdztgaJpZM4MC0WX> .

smarterclayton · 2017-03-17T01:15:15Z

Alternative - transform at the begin lining of each, go through and selectively audit until all transforms are removed.

openshift-bot · 2017-03-17T08:37:25Z

continuous-integration/openshift-jenkins/merge SUCCESS (Image: devenv-rhel7_6074)

…sionResources in namespace controller

mfojtik · 2017-03-17T10:36:41Z

re-[merge]

(forgot to update bootstrap test fixture)

openshift-bot · 2017-03-17T10:37:22Z

Evaluated for origin test up to 3e9c0f0

openshift-bot · 2017-03-17T10:37:39Z

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/283/) (Base Commit: bd446c7)

openshift-bot · 2017-03-17T10:37:39Z

Evaluated for origin testextended up to 3e9c0f0

openshift-bot · 2017-03-17T10:37:50Z

Evaluated for origin merge up to 3e9c0f0

mfojtik · 2017-03-17T11:02:10Z

@smarterclayton @liggitt will

mfojtik@9a9032f do?

I looked at other admission controllers and it seems image policy is special as it takes list of resources to enforce the policy as part of configuration... I don't see any other admission plugin having the same problem (but maybe I'm wrong ;-)

openshift-bot · 2017-03-17T11:53:17Z

continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/46/) (Base Commit: bd446c7) (Extended Tests: core)

openshift-bot · 2017-03-17T12:03:25Z

continuous-integration/openshift-jenkins/test SUCCESS

mfojtik · 2017-03-17T12:18:27Z

sttts · 2017-03-17T12:32:00Z

🎉

smarterclayton · 2017-03-17T14:20:00Z

This did not fix the concern I had with image policy - that actually exposes another hole. See my comments on the linked commit. On Mar 17, 2017, at 8:32 AM, Dr. Stefan Schimanski <[email protected]> wrote: 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12986 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pyGtFTzDwhvsZyN8pTOB-P79J_tiks5rmn1CgaJpZM4MC0WX> .

mfojtik requested review from soltysh and sttts February 16, 2017 10:15

mfojtik added this to the 1.6.0 milestone Feb 16, 2017

guangxuli reviewed Feb 16, 2017

View reviewed changes

mfojtik requested a review from deads2k February 16, 2017 12:45

bparees reviewed Feb 16, 2017

View reviewed changes

mfojtik force-pushed the apigroups branch 4 times, most recently from ece8195 to 29c7713 Compare February 20, 2017 16:47

mfojtik commented Feb 20, 2017

View reviewed changes

smarterclayton reviewed Feb 22, 2017

View reviewed changes

mfojtik force-pushed the apigroups branch 2 times, most recently from af5aadc to e01344a Compare February 22, 2017 10:40

sttts reviewed Feb 22, 2017

View reviewed changes

mfojtik force-pushed the apigroups branch 3 times, most recently from baaba1a to bf2e2c2 Compare February 22, 2017 15:26

mfojtik added 8 commits March 17, 2017 11:20

UPSTREAM: <drop>: add appliedclusterresourcequotas to ignoredGroupVer…

7a7d402

…sionResources in namespace controller

api: register and enable Origin API groups

49c0059

api: update swagger and protobuf to support api groups

2d1f794

fix unit tests to support both legacy and api groups

7c0d2dc

fix integration tests to support both legacy and api groups

cbbbfbb

fix test/cmd tests to support both legacy and api groups

2465161

api groups interesting changes

aeb26b4

api groups boring changes

3e9c0f0

mfojtik force-pushed the apigroups branch from b55c044 to 3e9c0f0 Compare March 17, 2017 10:30

openshift-bot merged commit 75c6750 into openshift:master Mar 17, 2017

spadgett mentioned this pull request Mar 19, 2017

Other Resources dropdown has duplicates openshift/origin-web-console#1364

Closed

liggitt mentioned this pull request Mar 24, 2017

OpenShift 3.6 Release Notes Tracker openshift/openshift-docs#4021

Closed

bjartek mentioned this pull request Mar 29, 2017

Support for API-groups and other kinds like the openshift objects kubernetes/kube-state-metrics#116

Closed

mfojtik deleted the apigroups branch September 5, 2018 21:07

		@@ -14,11 +17,17 @@ func init() {
		GroupName: api.GroupName,
		VersionPreferenceOrder: []string{v1.SchemeGroupVersion.Version},

Register OpenShift resources to API groups #12986

Register OpenShift resources to API groups #12986

Conversation

mfojtik commented Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guangxuli Feb 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pweil- commented Feb 16, 2017

Choose a reason for hiding this comment

bparees commented Feb 16, 2017

mfojtik commented Feb 16, 2017

mfojtik commented Feb 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Feb 21, 2017

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

mfojtik commented Feb 22, 2017

mfojtik commented Feb 22, 2017

smarterclayton commented Mar 17, 2017 via email

liggitt commented Mar 17, 2017

smarterclayton commented Mar 17, 2017 via email

smarterclayton commented Mar 17, 2017 via email

openshift-bot commented Mar 17, 2017 • edited Loading

mfojtik commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

mfojtik commented Mar 17, 2017 • edited Loading

openshift-bot commented Mar 17, 2017

openshift-bot commented Mar 17, 2017

mfojtik commented Mar 17, 2017

sttts commented Mar 17, 2017

smarterclayton commented Mar 17, 2017 via email

mfojtik commented Feb 16, 2017 •

edited

Loading

guangxuli Feb 16, 2017 •

edited

Loading

mfojtik commented Feb 20, 2017 •

edited

Loading

openshift-bot commented Mar 17, 2017 •

edited

Loading

mfojtik commented Mar 17, 2017 •

edited

Loading