Add replication controller status subresource #14672

derekwaynecarr · 2015-09-28T18:58:21Z

PR does the following:

Add a /status endpoint for replication controller.
Disable editing rc.Status when doing update
Disable editing rc.Spec when doing updateStatus

Fixes #4909

derekwaynecarr · 2015-09-28T19:04:22Z

@kubernetes/rh-platform-management in case of any downstream issue.

k8s-bot · 2015-09-28T19:29:11Z

Unit, integration and GCE e2e test build/test passed for commit afe8a2159458d61f9b7ff18f7161ad05728104cc.

k8s-bot · 2015-09-28T19:57:18Z

Unit, integration and GCE e2e build/test failed for commit ab5a3b83426609d58de5063c9393891d951ed0bd.

derekwaynecarr · 2015-09-28T20:01:00Z

@k8s-bot test this

k8s-github-robot · 2015-09-28T20:02:50Z

Labelling this PR as size/L

k8s-bot · 2015-09-28T20:33:45Z

Unit, integration and GCE e2e test build/test passed for commit ab5a3b83426609d58de5063c9393891d951ed0bd.

derekwaynecarr · 2015-10-05T17:39:33Z

@thockin - can i get a quick review on this?

derekwaynecarr · 2015-10-06T18:34:25Z

Rebased again. @thockin - any eyeballs available or reassign? @smarterclayton - got cycles?

k8s-bot · 2015-10-06T18:59:00Z

GCE e2e test build/test passed for commit ba4d8335699f73e8eec65109fa07e40ceeb02cb6.

smarterclayton · 2015-10-06T18:59:11Z

pkg/registry/controller/strategy.go

 	newController := obj.(*api.ReplicationController)
 	oldController := old.(*api.ReplicationController)
+	// update is not allowed to set status
+	newController.Status = oldController.Status


This is a backwards-incompatible API change. I don't think that we can do this in v1 unless we believe there is a security risk to the cluster to not change it.

I had raised that concern prior to submitting the PR.

See @bgrant0607 comment here:
#4909 (comment)

I agree with his analysis.

It's a potential DoS vulnerability.

Ok, I agree as well.

On Wed, Oct 7, 2015 at 10:03 AM, Derek Carr notifications@github.com
wrote:

In pkg/registry/controller/strategy.go
#14672 (comment)
:

newController := obj.(*api.ReplicationController) oldController := old.(*api.ReplicationController)

// update is not allowed to set status

newController.Status = oldController.Status

I had raised that concern prior to submitting the PR.

See @bgrant0607 https://github.com/bgrant0607 comment here:
#4909 (comment)
#4909 (comment)

I agree with his analysis.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/14672/files#r41393580.

bgrant0607 · 2015-10-07T14:20:26Z

I'll take a look.

bgrant0607 · 2015-10-07T14:21:26Z

pkg/api/validation/validation.go

@@ -1338,6 +1339,23 @@ func ValidateReplicationControllerUpdate(oldController, controller *api.Replicat
 	allErrs := errs.ValidationErrorList{}
 	allErrs = append(allErrs, ValidateObjectMetaUpdate(&controller.ObjectMeta, &oldController.ObjectMeta).Prefix("metadata")...)
 	allErrs = append(allErrs, ValidateReplicationControllerSpec(&controller.Spec).Prefix("spec")...)
+	allErrs = append(allErrs, ValidateReplicationControllerStatus(&controller.Status).Prefix("status")...)


Should be ValidateReplicationControllerStatusUpdate.

This is an internal helper function that is used by both

ValidateReplicationController(...) ValidateReplicationControllerStatusUpdate(...)

to verify just the rc.status section of the resource.

the ValidateReplicationControllerStatusUpdate function (which I added in this PR) is what you expect.

bgrant0607 · 2015-10-07T14:30:24Z

Mostly looks pretty reasonable, thanks.

You need to regenerate the swagger spec again.

smarterclayton · 2015-10-07T15:20:37Z

No further comments from me.

bgrant0607 · 2015-10-07T16:15:05Z

@derekwaynecarr Please mention me explicitly when you push the fixes.

derekwaynecarr · 2015-10-07T18:42:10Z

@bgrant0607 - made requested updates.

I had confused myself with the Validate*** comments, things should be fixed now as you expect.
regenerated swagger from HEAD

this should be good to go.

Thanks!

k8s-bot · 2015-10-07T18:54:50Z

GCE e2e test build/test passed for commit d05b382d0852add12c8c7a50f2010c39aaa5bf78.

k8s-bot · 2015-10-07T18:55:03Z

GCE e2e build/test failed for commit 544b453.

derekwaynecarr · 2015-10-07T20:07:28Z

@k8s-bot - test this

bgrant0607 · 2015-10-07T20:59:37Z

pkg/apis/experimental/v1alpha1/types_swagger_doc_generated.go

@@ -356,7 +356,8 @@ func (JobStatus) SwaggerDoc() map[string]string {
 }

 var map_NodeUtilization = map[string]string{
-	"": "NodeUtilization describes what percentage of a particular resource is used on a node.",
+	"":      "NodeUtilization describes what percentage of a particular resource is used on a node.",


This has been fixed in head.

bgrant0607 · 2015-10-07T21:01:14Z

LGTM

bgrant0607 · 2015-10-07T21:01:49Z

@k8s-bot test this

k8s-bot · 2015-10-07T21:35:38Z

GCE e2e test build/test passed for commit 544b453.

bgrant0607 · 2015-10-08T02:11:44Z

Please create a cherrypick PR for 1.1 after this merges.

thockin · 2015-10-08T04:13:01Z

pkg/master/master.go

-		"nodes/status":           nodeStatusStorage,
-		"events":                 eventStorage,
+		"replicationControllers":        controllerStorage,
+		"replicationControllers/status": controllerStatusStorage,


didn't we convert everything to all-lowercase?

Case conversion happens automatically. Internally we have camel case in
many parts of the code.

On Thursday, October 8, 2015, Tim Hockin notifications@github.com wrote:

In pkg/master/master.go
#14672 (comment)
:

@@ -567,12 +567,13 @@ func (m *Master) init(c *Config) {

"podTemplates": podTemplateStorage,

"replicationControllers": controllerStorage,

"services": service.NewStorage(m.serviceRegistry, m.endpointRegistry, serviceClusterIPAllocator, serviceNodePortAllocator),

"endpoints": endpointsStorage,

"nodes": nodeStorage,

"nodes/status": nodeStatusStorage,

"events": eventStorage,

"replicationControllers": controllerStorage,

"replicationControllers/status": controllerStatusStorage,

didn't we convert everything to all-lowercase?

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/14672/files#r41474385.

Ugly, but true.

Now that we have dropped the old name, we can normalize the internal
constants. It would probably reduce confusion for code reviewers.

On Fri, Oct 9, 2015 at 12:16 PM, Brian Grant notifications@github.com
wrote:

In pkg/master/master.go
#14672 (comment)
:

@@ -567,12 +567,13 @@ func (m *Master) init(c *Config) {

"podTemplates": podTemplateStorage,

"replicationControllers": controllerStorage,

"services": service.NewStorage(m.serviceRegistry, m.endpointRegistry, serviceClusterIPAllocator, serviceNodePortAllocator),

"endpoints": endpointsStorage,

"nodes": nodeStorage,

"nodes/status": nodeStatusStorage,

"events": eventStorage,

"replicationControllers": controllerStorage,

"replicationControllers/status": controllerStatusStorage,

Ugly, but true.

—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/14672/files#r41648501.

Add replication controller status subresource

…f-#14672-upstream-release-1.1 Auto commit by PR queue bot

…rry-pick-of-#14672-upstream-release-1.1 Auto commit by PR queue bot

googlebot added the cla: yes label Sep 28, 2015

derekwaynecarr changed the title ~~WIP: Add replication controller status subresource~~ Add replication controller status subresource Sep 28, 2015

derekwaynecarr force-pushed the rc_status branch from afe8a21 to ab5a3b8 Compare September 28, 2015 19:41

k8s-github-robot assigned thockin Sep 28, 2015

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 28, 2015

nikhiljindal mentioned this pull request Sep 29, 2015

Why ReplicationControllerInterface, and DaemonSetInterface have no UpdateStatus method #14717

Closed

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2015

derekwaynecarr force-pushed the rc_status branch from ab5a3b8 to ba4d833 Compare October 6, 2015 18:32

derekwaynecarr removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2015

smarterclayton reviewed Oct 6, 2015
View reviewed changes

bgrant0607 assigned bgrant0607 and unassigned thockin Oct 7, 2015

bgrant0607 reviewed Oct 7, 2015
View reviewed changes

derekwaynecarr force-pushed the rc_status branch from ba4d833 to d05b382 Compare October 7, 2015 18:25

Add replication controller status subresource

544b453

derekwaynecarr force-pushed the rc_status branch from d05b382 to 544b453 Compare October 7, 2015 18:40

bgrant0607 reviewed Oct 7, 2015
View reviewed changes

bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 7, 2015

bgrant0607 added the cherrypick-candidate label Oct 8, 2015

thockin reviewed Oct 8, 2015
View reviewed changes

piosz added a commit that referenced this pull request Oct 8, 2015

Merge pull request #14672 from derekwaynecarr/rc_status

6217869

Add replication controller status subresource

piosz merged commit 6217869 into kubernetes:master Oct 8, 2015

derekwaynecarr mentioned this pull request Oct 9, 2015

Automated cherry pick of #14672 to upstream release 1.1 #15373

Merged

k8s-github-robot referenced this pull request Oct 10, 2015

Merge pull request #15373 from derekwaynecarr/automated-cherry-pick-o…

8e97619

…f-#14672-upstream-release-1.1 Auto commit by PR queue bot

eparis removed the cherrypick-candidate label Mar 8, 2016

shyamjvs referenced this pull request in shyamjvs/kubernetes Dec 1, 2016

Merge pull request kubernetes#15373 from derekwaynecarr/automated-che…

bdd0e57

…rry-pick-of-#14672-upstream-release-1.1 Auto commit by PR queue bot

shouhong referenced this pull request in shouhong/kubernetes Feb 14, 2017

Merge pull request kubernetes#15373 from derekwaynecarr/automated-che…

4ab1069

…rry-pick-of-#14672-upstream-release-1.1 Auto commit by PR queue bot

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add replication controller status subresource #14672

Add replication controller status subresource #14672

derekwaynecarr commented Sep 28, 2015

derekwaynecarr commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

derekwaynecarr commented Sep 28, 2015

k8s-github-robot commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

derekwaynecarr commented Oct 5, 2015

derekwaynecarr commented Oct 6, 2015

k8s-bot commented Oct 6, 2015

smarterclayton Oct 6, 2015

derekwaynecarr Oct 7, 2015

bgrant0607 Oct 7, 2015

smarterclayton Oct 7, 2015

bgrant0607 commented Oct 7, 2015

bgrant0607 Oct 7, 2015

derekwaynecarr Oct 7, 2015

bgrant0607 commented Oct 7, 2015

smarterclayton commented Oct 7, 2015

bgrant0607 commented Oct 7, 2015

derekwaynecarr commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

derekwaynecarr commented Oct 7, 2015

bgrant0607 Oct 7, 2015

bgrant0607 commented Oct 7, 2015

bgrant0607 commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

bgrant0607 commented Oct 8, 2015

thockin Oct 8, 2015

derekwaynecarr Oct 8, 2015

bgrant0607 Oct 9, 2015

smarterclayton Oct 9, 2015

Add replication controller status subresource #14672

Add replication controller status subresource #14672

Conversation

derekwaynecarr commented Sep 28, 2015

derekwaynecarr commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

derekwaynecarr commented Sep 28, 2015

k8s-github-robot commented Sep 28, 2015

k8s-bot commented Sep 28, 2015

derekwaynecarr commented Oct 5, 2015

derekwaynecarr commented Oct 6, 2015

k8s-bot commented Oct 6, 2015

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

bgrant0607 commented Oct 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Oct 7, 2015

smarterclayton commented Oct 7, 2015

bgrant0607 commented Oct 7, 2015

derekwaynecarr commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

derekwaynecarr commented Oct 7, 2015

Choose a reason for hiding this comment

bgrant0607 commented Oct 7, 2015

bgrant0607 commented Oct 7, 2015

k8s-bot commented Oct 7, 2015

bgrant0607 commented Oct 8, 2015

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