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 kubebuilder tags to Condition type #92660

Merged
merged 2 commits into from
Jul 9, 2020

Conversation

damemi
Copy link
Contributor

@damemi damemi commented Jun 30, 2020

What type of PR is this?
/kind api-change

What this PR does / why we need it:
This picks the changes from #92519 and adds kubebuilder validation tags for CRD generation

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Kubebuilder validation tags are set on metav1.Condition for CRD generation

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


related to kubernetes/enhancements#1623

@k8s-ci-robot k8s-ci-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 30, 2020
@damemi
Copy link
Contributor Author

damemi commented Jun 30, 2020

/hold

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/code-generation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 30, 2020
@@ -1367,28 +1368,39 @@ type Condition struct {
// Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
// useful (see .node.status.conditions), the ability to deconflict is important.
// +required
// +kubebuilder:validation:Required
// +kubebuilder:validation:Enum=Available;Progressing;Degraded
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a regex in the validation code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I based this on the comment above the type. The validation code looks like it uses different regexes conditionally which I think is more complex than the regex marker can do alone

Copy link
Member

Choose a reason for hiding this comment

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

using an enum here is not correct... far more types than Available, Progressing, Degraded are supported

The regex validation seems to be conditional based on whether the type is qualified or not. I think that can be expressed like this:

(DNS1123_PATTERN/)?(QUALIFIED_NAME_PATTERN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@liggitt Ah, I'm not great with complex regexes so I wasn't aware if it could handle conditionals

Since dns1123 pattern is dns1123LabelFmt + "(\\." + dns1123LabelFmt + ")*" and qualified name fmt is "(" + qnameCharFmt + qnameExtCharFmt + "*)?" + qnameCharFmt, I tried substituting these into (DNS1123_PATTERN/)?(QUALIFIED_NAME_PATTERN) (evaluated out). This ended up being pretty complex from what I can see, not sure if there's a good way to simplify it

@fedebongio
Copy link
Contributor

/assign @liggitt @DirectXMan12

Type string `json:"type" protobuf:"bytes,1,opt,name=type"`
// status of the condition, one of True, False, Unknown.
// +required
// +kubebuilder:validation:Required
Status ConditionStatus `json:"status" protobuf:"bytes,2,opt,name=status"`
Copy link
Member

Choose a reason for hiding this comment

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

this is an appropriate place for an enum (True, False, Unknown)

ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,3,opt,name=observedGeneration"`
// lastTransitionTime is the last time the condition transitioned from one status to another.
// This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable.
// +required
// +kubebuilder:validation:Required
Copy link
Member

@liggitt liggitt Jul 1, 2020

Choose a reason for hiding this comment

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

is there a time format or RFC3339 regex we can use here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it looks like we can use type: string and format: date-time

// The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
// +required
// +kubebuilder:validation:Required
// +kubebuilder:validation:Pattern=^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])
Copy link
Contributor

Choose a reason for hiding this comment

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

this matches asdfasfas.sadf.asdfas/bla$'_%.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what I got manually expanding the regexes currently used by the validation function (#92660 (comment)) Did I miss something or write that out wrong?

// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Pattern=`^[A-Za-z_][A-Za-z0-9_]*$`
Copy link
Contributor

Choose a reason for hiding this comment

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

this got expanded [A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@damemi
Copy link
Contributor Author

damemi commented Jul 7, 2020

@deads2k @sttts think I addressed the comments here. There was some offline talk about trying to come up with a regex to match specifically non-zero rfc3339, is that still something we need or is a date-time format validation sufficient? (#92660 (comment))

@damemi
Copy link
Contributor Author

damemi commented Jul 7, 2020

/retest

@deads2k
Copy link
Contributor

deads2k commented Jul 7, 2020

Can you rebase this on master please? You've got an old version of the validation PR

@damemi
Copy link
Contributor Author

damemi commented Jul 7, 2020

Can you rebase this on master please? You've got an old version of the validation PR

Ah I did rebase it on master, I needed to rebase it on your branch from the validation PR. Updated and squashed

@deads2k
Copy link
Contributor

deads2k commented Jul 8, 2020

also make update. your generated code is out of date

@damemi
Copy link
Contributor Author

damemi commented Jul 8, 2020

@deads2k updated my commit and rebased on yours again

Reason string `json:"reason" protobuf:"bytes,5,opt,name=reason"`
// message is a human readable message indicating details about the transition.
// This may be an empty string.
// +required
// +kubebuilder:validation:Required
Copy link
Contributor

Choose a reason for hiding this comment

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

we allow the message to be empty. See two lines up. The required bit is about serialization back to clients.

Copy link
Member

@liggitt liggitt Jul 8, 2020

Choose a reason for hiding this comment

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

This looks correct to me. The value can be an empty string, but the field must be specified or openapi validation will complain ("message":"")

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks correct to me. The value can be an empty string, but the field must be specified or openapi validation will complain ("message":"")

Found it in my live test. The CR validation doesn't give slice numbers, so I was looking at the wrong item.

@deads2k
Copy link
Contributor

deads2k commented Jul 8, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot 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. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jul 8, 2020
@deads2k deads2k added this to the v1.19 milestone Jul 8, 2020
@deads2k
Copy link
Contributor

deads2k commented Jul 8, 2020

/retest

// +kubebuilder:validation:Required
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:Pattern=`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`
Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing commas. Not a fan....

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for colon. Hrm... gonna have to think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing commas. Not a fan....

Matches the reality of the world I'm afraid. We have people interested in adopting now that have union reasons with delimiters

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess since we said "machine readable" this is the world we live in.

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jul 8, 2020

@damemi: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kind-ipv6 95570fa684dd49b2c5787b93e7bd8f894b76b3d0 link /test pull-kubernetes-e2e-kind-ipv6

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, smarterclayton

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 Jul 8, 2020
@deads2k
Copy link
Contributor

deads2k commented Jul 8, 2020

/retest

@deads2k deads2k removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 9, 2020
@deads2k
Copy link
Contributor

deads2k commented Jul 9, 2020

validation is finalized, removing hold.

@deads2k deads2k added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 9, 2020
@fejta-bot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@damemi
Copy link
Contributor Author

damemi commented Jul 9, 2020

/label api-review

@k8s-ci-robot k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jul 9, 2020
@k8s-ci-robot k8s-ci-robot merged commit cada2eb into kubernetes:master Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants