SIG-NETWORK v1alpha2 review round 2 #861
Conversation
k8s-ci-robot
requested review from
andrewsykim,
danwinship,
khenidak and
thockin
k8s-ci-robot
added
kind/api-change
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott 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 |
k8s-ci-robot
added
do-not-merge/work-in-progress
| // +kubebuilder:validation:MaxLength=253 | ||
| Name string `json:"name"` | ||
|
|
||
| // SectionName is the name of a section within the target resource. In the |
There was a problem hiding this comment.
This seems like a rather complicated relationship thing - do we REALLY need it? Do we really need it NOW? What happens if we left this out and add it when we KNOW what it is for?
There was a problem hiding this comment.
This is a good question. Although I'd like to simplify the API, I think this particular field is more helpful than it might seem on the surface. For HTTP and TLS routes, the hostnames field provide a means of connecting Routes to the subset of Listeners that support at least one of their hostnames. Of course that does require each Listener and Route to specify meaningful hostnames. Although less precise, this could potentially pass for initial usage.
Unfortunately this doesn't work for L4 routing where we don't have a hostname concept. In that case, sectionName is our only mechanism to attach a Route to a specific listener. This becomes especially important with the lack of L4 matching capabilities - we're really expecting a 1:1 Listener - Route relationship at this level.
Even for L7, while hostnames could act as an alternative to sectionName, they don't provide the same level of control that sectionName does. I think matches that relied purely on hostnames could become difficult to manage.
| // | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| ParentRefs []ParentRef `json:"parentRefs,omitempty"` |
There was a problem hiding this comment.
Is it fair to say that every type of Route must have a field which is a list of ParentRef? E.g. can I assert that, for any given route, I can use reflect to find that? If so, let's say that. If not, ... ?
There was a problem hiding this comment.
I've been thinking about adding a skeleton Route object, that has the minimum fields required for any route object, which would seem to fit this one?
There was a problem hiding this comment.
#874 covers the work to clear this one up.
| // | ||
| // +optional | ||
| Port *PortNumber `json:"port,omitempty"` | ||
|
|
There was a problem hiding this comment.
In the works. Rob started #726 but I requested to punt since path munging in redirects has many possibilities.
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
|
|
||
| // Spec defines the desired state of ReferencePolicy. | ||
| Spec ReferencePolicySpec `json:"spec,omitempty"` |
There was a problem hiding this comment.
Does this need the extra level of nest? Status seems unlikely
There was a problem hiding this comment.
Discussed at community meeting yesterday. Consensus seemed to be that we wanted to leave room for status since we saw at least some plausible uses of it. Also worth noting that there's a proposal coming that would add status to NetworkPolicy.
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
k8s-ci-robot
added
the
cncf-cla: yes
robscott
force-pushed
the
v1alpha2-review-bkp
branch
from
5bb78f1
to
a4431d4
Compare
| // - Terminate: The TLS session between the downstream client | ||
| // and the Gateway is terminated at the Gateway. This mode requires | ||
| // certificateRefs to be set and contain at least one element. | ||
| // - Passthrough: The TLS session is NOT terminated by the Gateway. This |
There was a problem hiding this comment.
There is a third option. which is re-encrypt with a different certs while moving request to upstream server. This is in more-secure environment where user wants to terminate at ingress but don't want to use clear-text to upstream servers (and don't want to share edge listeners certs with upstream servers). It is not as common as terminate/passthrough but it exists.
There was a problem hiding this comment.
I 100% agree this is an important use case but I don't think its a separate tls option in the API. The upstream configuration and downstream configuration is separate. For a given incoming request, we can either terminate the TLS or pass it through. Then for a given outgoing request, we can either send it as is or encap with TLS.
What we do on the outgoing request side should not be coupled here, just like the outgoing protocol is not (for example, terminate HTTP/2 and send HTTP/1.1 upstream).
Note this also means you can passthrough AND encap in TLS. A bit obscure for other cases, but this can be used for service mesh mTLS when the application itself still expects a TLS connection.
There was a problem hiding this comment.
Agree with everything @howardjohn said above. @khenidak do you think that makes sense? Would clarifying this distinction in the comments here be sufficient?
|
@robscott i relooked. Nothing jumped at me that has not been pointed out by other folks, or previously in the last round of review. One issue. other than that i think we should move this forward and start build test controllers (and ask ingress providers to build some testing against it) to flush out any issue we may have messed by statically reviewing the api. |
|
I think sig-network review is effectively complete at this point, so I'm going to close this out. Let me know if I've missed anything. |
There was a problem hiding this comment.
OK, I have a few comments about documentation, but the only comment about the API itself (that needs to be addressed or dismissed before merging) is the last comment, about HttpRequestRedirect.StatusCode
[EDIT: Rob pointed out that we can loosen the validation later, so we don't need to do anything about StatusCode now.]
I am still concerned about security issues in two places, but these can be handled with documentation rather than API changes:
- The documentation about how to handle invalid filter/routes/rules is pretty vague and still wants to err on the side of "just guess what the user meant" rather than "avoid introducing massive security holes into the cluster". It should explicitly warn that the implementation may create security holes if it incorrectly accepts a partially-incorrect rule.
- The rules about how to handle multiple matching rules have kept the "stealable" semantics, so we need to explicitly warn users that it is not safe to attach a Route to a Gateway that also accepts Routes from Namespaces that they do not control.
| // GatewayClass resource. | ||
| GatewayClassName ObjectName `json:"gatewayClassName"` | ||
|
|
||
| // Listeners associated with this Gateway. Listeners define |
There was a problem hiding this comment.
So there is still a problem throughout the API where all of the documentation is at the level of individual fields, but many of the concepts can only be explained in terms of combinations of fields, and then given that it's not always obvious where a particular thing is documented, etc. I think there really needs to be more "overview" documentation separate from the individual objects and fields, so that the field-level API documentation can be simplified.
There was a problem hiding this comment.
Is it sufficient to have that kind of documentation covered in gateway-api.sigs.k8s.io? If so, maybe we can just simplify some of our field definitions? This one in particular is quite complex.
There was a problem hiding this comment.
I'm not really sure where the right place for this is... I mean, really, my comment applies to some core types too. (eg, I don't think the exact rules for how NetworkPolicy is interpreted are written down anywhere in the current k8s docs.)
| // case sensitive and done on a path element by element basis. A | ||
| // path element refers to the list of labels in the path split by | ||
| // the `/` separator. A request is a match for path _p_ if every | ||
| // _p_ is an element-wise prefix of the request path. |
There was a problem hiding this comment.
The word "every" there seems wrong? Or if it's not wrong, it's very confusing and I don't know what it means.
Is the Value of a PathMatchPrefix allowed to end with /? If not then you can just say that it matches if either *Value equals the request path or else *Value + "/" is a prefix of the request path.
(If the Value is allowed to end with / then does Value: "/abc/" match request path "/abc"?)
There was a problem hiding this comment.
This doesn't actually make it better, but this particular description is copied verbatim from the Ingress spec: https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/networking/types.go#L470. Will work on making this easier to understand though.
Is the Value of a PathMatchPrefix allowed to end with /?
Yes, although we could prevent that, it might be strange for examples where "/" feels more common/natural than just "".
If not then you can just say that it matches if either *Value equals the request path or else *Value + "/" is a prefix of the request path.
I don't think we can be quite that loose. We are trying to state that /abc should match /abc or /abc/foo but not match /abc-foo.
There was a problem hiding this comment.
I think Dan's words are OK?
it matches if either *Value equals the request path or else *Value + "/" is a prefix
"/abc" matches "/abc" or "/abc/.*"
There was a problem hiding this comment.
Yeah that's a good point, I think I misunderstood what he was suggesting. I can work on cleaning this up in both the Ingress and HTTPRoute spec.
| // can support POSIX, PCRE, RE2 or any other regular expression dialect. | ||
| // Please read the implementation's documentation to determine the supported | ||
| // dialect. | ||
| PathMatchRegularExpression PathMatchType = "RegularExpression" |
There was a problem hiding this comment.
Is there any requirement/expectation about whether the RE is anchored? (ie, is it possible that `.*\.html` matches "foo.html.txt" in some implementations but not in other implementations?) Might be worth mentioning.
(Likewise for HeaderMatchRegularExpression etc)
There was a problem hiding this comment.
Since there are so many such cases when it comes to regex, we are not at all prescriptive and let implementations decide the behavior that makes the most sense for them.
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=256 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$` |
There was a problem hiding this comment.
Wow... TIL that !#$%&'* is a valid HTTP header name...
There was a problem hiding this comment.
That's my understanding based on https://datatracker.ietf.org/doc/html/rfc7230#section-3.2.6, hopefully I got it right. It looks like header names can include any tchar as defined there.
There was a problem hiding this comment.
I started writing about how you are wrong, and ... then... you're not.
TIL.
| // | ||
| // +optional | ||
| // +kubebuilder:default=302 | ||
| // +kubebuilder:validation:Enum=301;302 |
There was a problem hiding this comment.
so I said in #780 that this should allow 301-399, and you commented that some implementations can't support other values, but I think the API should allow it and you should just require the implementation to reject the rule if it can't implement the requested StatusCode. There are reasons the other 3xx status codes exist, and people may want to use them, and it seems weird to say they can't just because someone else's implementation wouldn't be able to handle it.
There was a problem hiding this comment.
I personally don't think this needs to block the initial release of v1alpha2, but I do agree that we should expand what's possible here in a future release. I believe this limitation originally came from a thread with @hbagdi and I around the support level of this field. We had to choose between either considering this "Core" and therefore broadly supportable, or making this a field that was considered either fully or partially "Extended". We were both hesitant about adding a caveat that this is "Core" for 301 and 302 but "Extended" for everything else. I'm not sure how valid those concerns are, but I am confident that this is something we can open up later. We already plan to expand the scope of what's possible in this filter with #731, so I'm sure we can fit this loosened validation in at that point.
There was a problem hiding this comment.
As this API progresses, we will have to figure out where to draw the line in terms of features.
The core having features that satisfy 90% of users is more than enough IMHO. Having a complicated core that implementations can't conform to or where users get confused is worse than having a core that is lacking features.
Now having said that, we don't really know where the line today is. That will come with user feedback. In this case, I agree with Rob that we should continue with what we have today and open up the surface as we learn more.
robscott
changed the title
| // can support POSIX, PCRE, RE2 or any other regular expression dialect. | ||
| // Please read the implementation's documentation to determine the supported | ||
| // dialect. | ||
| PathMatchRegularExpression PathMatchType = "RegularExpression" |
There was a problem hiding this comment.
"RegularExpression" vs "Regex" or "Regexp"? Discuss... (and other places like headers)
There was a problem hiding this comment.
RegularExpression seems least ambiguous to me and has the added benefit of matching the v1a1 API. I think we'd need a good reason to move away from it, and I can't think of one. Did a quick search through k/k and not seeing any of these used in existing APIs.
| // | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=256 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z0-9!#$%&'*+\-.^_\x60|~]+$` |
There was a problem hiding this comment.
I started writing about how you are wrong, and ... then... you're not.
TIL.
|
Thanks for the great feedback here! With v1alpha2 approved, I'm going to close this one out. |
What type of PR is this?
/kind api-change
What this PR does / why we need it:
Despite our best efforts, #780 has become remarkably slow to load. If that ends up making reviews on that PR too difficult, this now exists as a backup.
References:
/hold
/cc @andrewsykim @danwinship @khenidak @thockin