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.

Sign up for GitHub

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

Jump to bottom

✨ Add API types for Runtime SDK ExtensionConfig #6383

Merged
merged 1 commit into from
Apr 26, 2022
Merged

✨ Add API types for Runtime SDK ExtensionConfig #6383

merged 1 commit into from
Apr 26, 2022

Conversation

killianmuldoon
Copy link
Contributor

Signed-off-by: killianmuldoon [email protected]

Implement the Extension Config API types outlined in the [Runtime SDK proposal] #6181.

This PR contains:

  • A feature flag for Runtime SDK
  • Extension Config type and generated CRD
  • A webhook to block creation of the ExtensionConfig if the feature flag is disabled

I'll sync any changes from here back into the proposal as this PR progresses.
Part of #6330

@sbueringer @fabriziopandini @vincepri @CecileRobertMichon @enxebre
(as this is a new API)

/area runtime-sdk
/kind feature

@k8s-ci-robot k8s-ci-robot added area/runtime-sdk Issues or PRs related to Runtime SDK kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 6, 2022
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 6, 2022
sbueringer
sbueringer reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

A few comments but only nits. Overall the relevant parts look good to me!

main.go Outdated Show resolved Hide resolved
feature/feature.go Outdated Show resolved Hide resolved
config/manager/manager.yaml Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_webhook.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extension_types.go Outdated Show resolved Hide resolved
@killianmuldoon killianmuldoon force-pushed the runtimeSDK/add-extensionConfig-api branch 4 times, most recently from b7a80b2 to ee3855a Compare April 6, 2022 15:20
@killianmuldoon killianmuldoon mentioned this pull request Apr 6, 2022
Closed
@killianmuldoon killianmuldoon force-pushed the runtimeSDK/add-extensionConfig-api branch from ee3855a to 785924f Compare April 6, 2022 15:26
@sbueringer
Copy link
Member

Thx for the nice and quick work! :)

Apart from the two discussion topics above, lgtm from my side.

JoelSpeed
JoelSpeed reviewed Apr 6, 2022
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

As mentioned on slack, I've been working on API reviews for the last 6 months or so, and while I'm not sure you want to act on all of these right now, here's some initial API review style feedback for you to consider.

In general, this is pretty conformant, my main takeaway is I don't think we are leveraging the OpenAPI schema validation very much, and it would improve the UX if we were to do so.

In general, when reviewing, I also like to look at the comments authors are adding. We should consider the godocs as user facing (as they end up in kubectl explain), so we should add as much detail there are possible, these are the general sort of questions I think about for the documentation part of the review:

  • What is the purpose of this field? What does it allow them to achieve?
  • How does setting this field interact with other fields or features?
  • What are the limitations of this field?
    • Does it have any maximum or minimum value?
    • If it is a string value, are the values limited to a specific list or can it be free form, must it meet a certain regex?
    • Limitations should be written out within the Godoc as well as added within kubebuilder tags. Kubebuilder tags are used for validation but are not included within any generated documentation.
      See the validation docs for inspiration on more validations to apply.
  • Is the field optional or required?
  • When optional, what happens when the field is omitted?

exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
exp/runtime/api/v1beta1/extensionconfig_types.go Outdated
Comment on lines 120 to 126
// TimeoutSeconds defines the timeout duration for client calls to the ExtensionHandler.
// +optional
TimeoutSeconds *int32 `json:"timeoutSeconds,omitempty"`

// FailurePolicy defines how failures in calls to the ExtensionHandler should be handled by a client.
// +optional
FailurePolicy *FailurePolicy `json:"failurePolicy,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Do either of these need to be pointers, is there a semantic difference between the zero go value and the nil value?
If there is, it's worth adding a comment to point this out, eg, when timeoutSeconds is set to 0, this disables the timeout <- I have no idea if that's true, just an example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think these are pointers in line with the guidelines optional fields being pointers. Both of these will be defaulted if unset by users.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a strong opinion on this, but, upstream kube conventions say yes, these should be pointers, the CAPI conventions are loosely worded, Fields SHOULD be pointers if there is a good reason for it, for example:, which to me reads as we would prefer them not to be pointers unless we have a good reason, based on that I would say these shouldn't be pointers but willing to yield as it's pretty minor

exp/runtime/api/v1beta1/extensionconfig_types.go Outdated Show resolved Hide resolved
This was referenced Apr 8, 2022
Open
Open
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 11, 2022
@killianmuldoon killianmuldoon force-pushed the runtimeSDK/add-extensionConfig-api branch from 41bf904 to 29f4744 Compare April 11, 2022 12:00
@killianmuldoon killianmuldoon mentioned this pull request Apr 12, 2022
Merged
@killianmuldoon
Copy link
Contributor Author

/retest

@fabriziopandini
Copy link
Member

Great job!
lgtm pending squash
I'm really looking forward to see this new type merged (behind feature gate) so we can start moving forward with the RuntimeSDK implementation

@ykakarap ykakarap force-pushed the runtimeSDK/add-extensionConfig-api branch 2 times, most recently from 3578085 to 2163a02 Compare April 20, 2022 00:30
@ykakarap
Copy link
Contributor

Squashed the commits.

The one pending item from the comments I can see is if we want to keep the API version as v1beta1 or change it to v1alpha1. Ref: #6383 (comment)

@fabriziopandini
Copy link
Member

It seems the API is on v1alpha1 as requested
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2022
@fabriziopandini
Copy link
Member

@ykakarap please fix webhook names
@JoelSpeed ok for you moving forward with this given that the discussion on OpenAPI validation/defaulting is tracked on a separated issue?

sbueringer
sbueringer reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Found another v1beta1. Otherwise looks great and lgtm from my side

exp/runtime/api/v1alpha1/groupversion_info.go Outdated Show resolved Hide resolved
@ykakarap ykakarap force-pushed the runtimeSDK/add-extensionConfig-api branch from 2163a02 to ebeb2e2 Compare April 26, 2022 16:53
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2022
@JoelSpeed
Copy link
Contributor

@fabriziopandini All good from me, happy to proceed

@ykakarap ykakarap force-pushed the runtimeSDK/add-extensionConfig-api branch from ebeb2e2 to 9977653 Compare April 26, 2022 17:23
@sbueringer
Copy link
Member

Thx!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2022
@fabriziopandini
Copy link
Member

Thanks @JoelSpeed!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2022
@k8s-ci-robot k8s-ci-robot merged commit 192729a into kubernetes-sigs:main Apr 26, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.2 milestone Apr 26, 2022
@sbueringer sbueringer mentioned this pull request Apr 27, 2022
Closed
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ykakarap ykakarap ykakarap left review comments

@alexander-demicev alexander-demicev alexander-demicev left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@sbueringer sbueringer sbueringer left review comments

@enxebre enxebre enxebre left review comments

Assignees

@sbueringer sbueringer

@fabriziopandini fabriziopandini

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/runtime-sdk Issues or PRs related to Runtime SDK cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
8 participants
@killianmuldoon @sbueringer @fabriziopandini @ykakarap @JoelSpeed @k8s-ci-robot @enxebre @alexander-demicev