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

Add status subresource to HTTPProxy #2496

Merged
merged 1 commit into from
May 7, 2020

Conversation

youngnick
Copy link
Member

@youngnick youngnick commented May 4, 2020

This commit enables the status subresource for HTTPProxy.
This means that:

  • normal HTTPProxy changes that include updates to the status stanza will have the status updates dropped
  • updates to status can only be made using requests to the status subresource.

Also changes the HTTPProxy status updating to use the status subresource,
and adds RBAC for Contour to be able to update it.

Having the status subresource enabled is best-practice for CRDs and will be a foundation for further status work.

It also means that HTTPProxy objects no longer need to include the status stanza to be valid.

Updates #2495

Signed-off-by: Nick Young ynick@vmware.com

@youngnick youngnick self-assigned this May 4, 2020
@youngnick
Copy link
Member Author

youngnick commented May 4, 2020

Adding as a draft PR so that we can talk about if this is a good idea; I'm pretty sure that it is, and although it's kind of a GA guarantee violation, it should be worthwhile as we do more with status.

Importantly, it doesn't change any behaviour about retrieving the status, only about setting it, which only Contour should be doing.

I've updated the one place we update HTTPProxy objects' status right now, but anyone else consuming HTTPProxy objects and updating their status would also need to change to using a status update instead of patching the object.

I noticed this while doing research for #2495.

@stevesloka
Copy link
Member

This also limits us to Kubernetes v1.16 which is where subresources went GA (https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#status-subresource).

@youngnick
Copy link
Member Author

Ah, thanks for that Steve. Subresources are available in the previous CRD versions though, I think. I will check that out.

@codecov
Copy link

codecov bot commented May 5, 2020

Codecov Report

Merging #2496 into master will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2496      +/-   ##
==========================================
+ Coverage   76.61%   76.63%   +0.01%     
==========================================
  Files          68       68              
  Lines        5513     5513              
==========================================
+ Hits         4224     4225       +1     
  Misses       1192     1192              
+ Partials       97       96       -1     
Impacted Files Coverage Δ
cmd/contour/serve.go 0.00% <0.00%> (ø)
internal/k8s/syncer.go 0.00% <ø> (ø)
internal/k8s/converter.go 55.55% <40.00%> (+0.26%) ⬆️
internal/k8s/status.go 85.00% <83.33%> (+3.60%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2672fac...9a251f1. Read the comment docs.

@youngnick
Copy link
Member Author

The status subresource on CRDs has been beta since at least 1.14, so I'm confident we are okay to include it.

@youngnick youngnick marked this pull request as ready for review May 5, 2020 03:52
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Have a few suggesting, but this seems generally good to me.

internal/k8s/status.go Outdated Show resolved Hide resolved
internal/k8s/status.go Outdated Show resolved Hide resolved
internal/k8s/status.go Show resolved Hide resolved
internal/k8s/status.go Outdated Show resolved Hide resolved
@youngnick youngnick force-pushed the status-subresource-test branch 2 times, most recently from c63e271 to f2ae3f4 Compare May 7, 2020 00:50
@youngnick youngnick requested a review from jpeach May 7, 2020 00:50
Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

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

Looks great, just an error message nit!

internal/k8s/status.go Outdated Show resolved Hide resolved
This commit enables the status subresource for HTTPProxy.
This means that:
- normal HTTPProxy changes that include updates to the `status` stanza will have the status updates dropped
- updates to `status` can only be made using requests to the status subresource.

Also changes the HTTPProxy status updating to use the status subresource,
and adds RBAC for Contour to be able to update it.

Having the status subresource enabled is best-practice for CRDs and will be a foundation for further status
work.

It also means that HTTPProxy objects no longer need to include the `status` stanza to be valid.

Refactors the `k8s.UnstructuredConverter` to have FromUnstructured and ToUnstructured methods, and uses ToUnstructured where required.

Updates projectcontour#2495

Signed-off-by: Nick Young <ynick@vmware.com>
@youngnick youngnick merged commit c11ed7b into projectcontour:master May 7, 2020
@jpeach jpeach added this to the 1.5.0 milestone May 28, 2020
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.

3 participants