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

doc: add direct resource API validation guide #2610

Merged
merged 3 commits into from
Sep 18, 2024

Conversation

yuwenma
Copy link
Collaborator

@yuwenma yuwenma commented Sep 3, 2024

Change description

Fixes #

Tests you have done

  • Run make ready-pr to ensure this PR is ready for review.
  • Perform necessary E2E testing for changed resources.

@yuwenma yuwenma force-pushed the CR-validation branch 2 times, most recently from 2500c33 to ce526ba Compare September 16, 2024 14:39
@yuwenma yuwenma force-pushed the CR-validation branch 2 times, most recently from def5fda to b3c020f Compare September 16, 2024 14:52
Copy link
Collaborator

@jasonvigil jasonvigil left a comment

Choose a reason for hiding this comment

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

There seem to be a few formatting issues, but I am not sure if markdown will care. Extra newlines, spaces between words, etc. Overall seems good, have a few questions.


# TL;DR

ConfigConnector Direct Resource shall have each `spec` field validated.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is Config Connector supposed to be two words?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Changed to Config Connector instead (here and else where).


### Immutable

Since most parent is a reference to another resource which can either be `<kind>Ref.external `or `<kind>Ref.name` , the immutable validation needs to be done at the controller validation level. For example, switching `<kind>Ref.external `and `<kind>Ref.name `are allowed if referring to the same resource.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why < here ..? Formatting seems weird

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, I used an extension to convert the google doc to Markdown.


Since most parent is a reference to another resource which can either be `<kind>Ref.external `or `<kind>Ref.name` , the immutable validation needs to be done at the controller validation level. For example, switching `<kind>Ref.external `and `<kind>Ref.name `are allowed if referring to the same resource.

See for general reference guide;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing link ..?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, because the reference doc is not merged. added a marker as reminder


See for general reference guide;

See for parent validation with `status.externalRef`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.


OpenAPI supports `oneOf`, `anyOf` and `allOf`. Thus, in theory these should be the CRD level rules. The Direct Resource CRD is auto-generated by controller-gen, which only reads the kubebuilder tag rules. But the kubebuilder tags [do not (and most likely won’t)](https://github.com/kubernetes-sigs/controller-tools/issues/461) support `oneOf`, `anyOf` or `allOf`.

To set this as CRD level rule, ConfigConnector shall build its own CRD transformer. Ideally, this could be a ConfigConnector tag similar to kubebuilder, but this requires integrating with controller-gen which is a non-trivial amount of work. Other options like webhook, OpenAPI schema modifier are not self-explaining and easy enough to use, considering that Direct Resource shall be open to external contributors. Thus, ConfigConnector does not use CRD level validation for `oneOf`, `anyOf` and `allOf`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this exist yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not yet. We may want to build it someday. Defer to the requirements


## Rule 1: Resource Reference

ConfigConnector shall validate the resource reference based on
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on what..? Seems to be missing info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah, looks like Docs™ to Markdown cannot render the inline google doc. Fixed

@google-oss-prow google-oss-prow bot added the lgtm label Sep 18, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasonvigil

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

@google-oss-prow google-oss-prow bot merged commit 5b1307c into GoogleCloudPlatform:master Sep 18, 2024
3 checks passed
@yuwenma yuwenma added this to the 1.123 milestone Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants