feat: introduce v1beta1 types #1073
Conversation
Signed-off-by: mehabhalodiya <[email protected]>
k8s-ci-robot
added
kind/feature
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mehabhalodiya The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @robscott |
I fooled myself into thinking this wasn't actually doing anything, so I figure it would be good to add a reassuring success message: $ ./hack/verify-boilerplate.sh Verified 111 file headers match boilerplate
Add a success message to verify-boilerplate
| type GatewaySpec struct { | ||
| // GatewayClassName used for this Gateway. This is the name of a | ||
| // GatewayClass resource. | ||
| GatewayClassName ObjectName `json:"gatewayClassName"` |
There was a problem hiding this comment.
Is there a reason to not just name the field ClassName? Or even just GatewayClass? The latter would be consistent with Service.Spec.LoadBalancerClass. That said, the Name suffix is used by Ingress.Spec.IngressClassName. In fact, the Name suffix is also used by PersistentVolumeClaim.Spec.StorageClassName, so it appears as if the outlier is Service.Spec.LoadBalancerClass and the convention is to use the Name suffix. Carry on 😃
Also, the comment could be simplified:
// GatewayClassName is the name of the GatewayClass resource used by
// this Gateway.Finally, the use of ObjectName is interesting. I see it has a validation on it for min and max length. However, the comments on the type also indicate additional, possible validations:
// ObjectName refers to the name of a Kubernetes object.
// Object names can have a variety of forms, including RFC1123 subdomains,
// RFC 1123 labels, or RFC 1035 labels.
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253I wonder if there is a reason to not just use string and let the webhook that will likely do the additional validation also be responsible for the min/max length validation?
There was a problem hiding this comment.
Hey @akutz! We're coming to the end of the v1beta1 release window, and both @robscott and I realized that your feedback had slipped through the cracks. So, I'll respond now where I think we probably won't make changes, and make issues for ones where we will.
For the min-max length validations and others, the general guideline we're using is to only add custom validation when we can't get it from the kubebuilder/OpenAPI validation.
| // | ||
| // Support: Core | ||
| // | ||
| // +listType=map |
There was a problem hiding this comment.
Ah, cool. I have not seen this used yet, so it's neat to see it!
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=64 |
There was a problem hiding this comment.
So this is odd to me. Why a max of 64 listeners? That seems arbitrary?
There was a problem hiding this comment.
We haven't written this down anywhere, I don't think, but we prefer not to have unbounded lists, since people will inevitably try to stuff 1000 things in there.
For times when we don't have a good idea, we do just pick an arbitrary limit that seems larger than what we think most uses will need, and accept that we may need to increase later. This is the case for all the limits you've called out here. Yes, they are arbitrary, but we needed a value >1 and < infinity, and we have no data about actual usage, so any number we pick is arbitrary.
I'll summarize this in other replies as "No unbounded lists".
There was a problem hiding this comment.
I just realized that I didn't make clear that the other reason for the limits is the max record size in etcd, which defaults to about 1MB of text in deserialized form.
| // | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MinItems=1 |
There was a problem hiding this comment.
I do not think a non-zero list should be required to set up a gateway. I can easily see a gateway being created to reserve or grab an IP address before any listeners are defined. I would like there to be an allowed min items of zero and mark this field as +optional and omitempty. Maybe there was some discussion about this at one point that I missed? Thanks!
| // Support: Core | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=16 |
There was a problem hiding this comment.
Once again the use of a max here seems arbitrary. Why would this be limited?
There was a problem hiding this comment.
| // Group is the group of the Route. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:default=gateway.networking.k8s.io |
There was a problem hiding this comment.
In any other case an empty group would default to v1. I get why it is defaulting to the group you have, but should it?
There was a problem hiding this comment.
The group is the domain part of the apiVersion field, v1 is the version. We've chosen to leave out version here to make it so that users don't have to manually update versions as we move through them - on the assumption we'll move through versions much faster than upstream Kubernetes does.
| } | ||
|
|
||
| // RouteGroupKind indicates the group and kind of a Route resource. | ||
| type RouteGroupKind struct { |
There was a problem hiding this comment.
I think this should be renamed to RouteGroupVersionKind and enforce the use of the version in this struct as well. Types at specific versions can sometimes be entirely different types, and we need to know the version of the type we're selecting.
| // Type of the address. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=IPAddress;Hostname;NamedAddress |
There was a problem hiding this comment.
Why is the validation not on the type AddressType string declaration just like it is for type FromNamespaces string and other declarations in this PR?
Also, I think we may want to address dual-stack and explicit IPv6 before going beta. This has been a pain in Kube proper for a while, and for the GW APIs to not have parity would be a step back IMO.
There was a problem hiding this comment.
I've addressed this and other concerns in #1178.
| // assigns an address from a reserved pool. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=16 |
There was a problem hiding this comment.
Again, why this seemingly arbitrary max?
There was a problem hiding this comment.
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=name | ||
| // +kubebuilder:validation:MaxItems=64 |
There was a problem hiding this comment.
Again, why this seemingly arbitrary max?
There was a problem hiding this comment.
|
It may be best to not review such humungous PRs and find other alternatives to review the patch-diff between v1alpha2 and v1beta1. Thoughts? |
|
Yep, sorry for the confusion on this PR! These are great comments @akutz and I'll try to respond to them soon, but this particular PR was actually intended to be more of a checklist item on the milestone than the actual API review. I've been super busy, but my goal is to get a PR out that is just a diff of aps/v1alpha2 between v0.4.0 release and now, like @hbagdi suggested above. Of course if anyone else wants to take that on, I'm happy to let someone else own it. Either way, hopefully we can have a smaller diff ready for review on Monday. |
@hbagdi, I absolutely know what you mean, but at the same time I could not help but smile at just this part 😄 |
|
Hey @mehabhalodiya, sorry for the confusion! Your PR is headed in the right direction, I'd just missed creating the corresponding PR that compared v0.4.0 and master API types for the sake of a simpler API review. I'm hoping to have that in place on Monday, but will also work to respond to the great feedback here as well. |
|
If y'all want to ignore my feedback on this PR, I will post it on the newer PR when Robb posts it. I will still wait to resolve this PR's feedback until the other is posted, just so it is a little easier for me to find the comments. |
Provide more details on route parentRef in documentation for routes <> Gateways attachment.
…-core-tls-support clarify core support of secret reference
Bumping version references to v0.4.3
Adds the status of the "Accepted" condition to the GatewayClass print columns list. Signed-off-by: Steve Kriss <[email protected]>
- fixes path prefix match YAML - removes check for HTTPRoute to be Accepted pending kubernetes-sigs#1112 Signed-off-by: Steve Kriss <[email protected]>
Signed-off-by: Steve Kriss <[email protected]>
…policy-fix conformance: fixes for invalid reference policy test
fix: typo for ambassador API gateway
…pted-printcolumn add GatewayClass Accepted print column
Cleaning up godocs after v0.5.0 API Review
The GatewayTLSConfig CertificateRefs member is a slice of pointer to struct which implies that the struct is optional. Change this to a slice of struct to better fit with API type conventions.
We would like our example YAML files to include comments that do not show up in the guides where the files are used as jinja templates, while still ensuring they are valid YAML files. Configuring the jinja Environment with "line_comment_prefix" allows you to control the syntax of the jinja comment directive. If we set that to "##", then such lines are ommitted during jinja templating, and are treated as YAML comments. See also https://jinja.palletsprojects.com/en/2.11.x/templates/#line-statements
Add Jinja "line comments" to our example manifests which reference the
guides which use them.
Generated using this awfulness:
$ sed -i '1s/^/## Used in:\n/' $( \
for iii in $(git grep -l '{% include' site-src/); do \
awk -F "'" '/{% include/ { print "examples/"$2 }' $iii ; \
done | sort | uniq)
$ for iii in $(git grep -l '{% include' site-src/); do \
for jjj in $(awk -F "'" '/{% include/ { print "examples/"$2 }' $iii); do \
sed -i "2s|^|## - $iii\n|" $jjj; \
done; \
done
Co-authored-by: Mark McLoughlin <[email protected]>
…s-backlinks docs: backlinks from example manifests to guides
Fix certificateRefs optionality.
This doc's intended audience is users rather than implementors, so these keywords aren't really appropriate.
docs: add note on RFC2119 keyword semantics
…me-spec Clarifying that wildcard hostname matching is suffix matching
…odifier Adding validation to ensure prefix path match exists for path modifier
…e-policy Renaming ReferencePolicy to ReferenceGrant
Signed-off-by: mehabhalodiya <[email protected]>
Signed-off-by: mehabhalodiya <[email protected]>
|
Sorry for the delay! |
|
I think I've hit most of Andrew's concerns. I think that we should actually get this merged into master asap, and then if we need to make any further v1alpha2 changes, we copy them over. |
Signed-off-by: mehabhalodiya <[email protected]>
Signed-off-by: mehabhalodiya [email protected]
What type of PR is this?
/kind feature
What this PR does / why we need it:
As suggested by @robscott, this is a follow-up PR on one of our last steps before the v1beta1 (v0.5.0) release.
The key is that we’d need
GatewayClass,Gateway, andHTTPRoutetype definitions inapis/v1beta1in addition to the alpha directories they already exist in.Which issue(s) this PR fixes:
Fixes #1041
Does this PR introduce a user-facing change?: