Adding Ingress PathType #2
Conversation
robscott
force-pushed
the
ingressv1-pathtype
branch
from
ff4f061
to
9701b93
Compare
robscott
force-pushed
the
ingressv1-pathtype
branch
from
9701b93
to
0131957
Compare
|
@cmluciano I think this is ready for review after #1 is merged in. If it's easier I can just merge all the things myself and review can happen on the main ingressv1 branch. |
robscott
force-pushed
the
ingressv1-pathtype
branch
from
0131957
to
671adff
Compare
|
@cmluciano Thanks for merging that previous PR! I've rebased this on that and it should be ready for a look whenever you have time. |
|
@robscott Have a review in progress that I will try to submit tomorrow. |
|
Sounds good, thanks! |
robscott
force-pushed
the
ingressv1-pathtype
branch
from
671adff
to
0160375
Compare
|
I've updated the implementation here to use the first approach/option I outlined in the corresponding enhancements PR here: kubernetes/enhancements#1413. Open to other options/approaches here, just wanted to get some reasonable validation in place. |
robscott
force-pushed
the
ingressv1-pathtype
branch
3 times, most recently
from
bab47ce
to
f6b4815
Compare
There was a problem hiding this comment.
I only skimmed this for now.
How do we want to do API reviews on this branch? It seems like it would be easier to do them one concept at a time, rather than all of it together.
Assuming that is true, please ping me when the PRs are ready, and I'll review.
This one was just cursory and needs more detailed review.
robscott
force-pushed
the
ingressv1-pathtype
branch
from
f6b4815
to
09bc299
Compare
robscott
force-pushed
the
ingressv1-pathtype
branch
3 times, most recently
from
eab504d
to
ff8d5a6
Compare
|
@cmluciano @thockin Thanks for the initial reviews! This is ready for another round of reviews when you have time. |
| if err != nil { | ||
| allErrs = append(allErrs, field.Invalid(fldPath.Child("paths").Index(i).Child("path"), rule.Path, "must be a valid regex")) | ||
|
|
||
| if *path.PathType == networking.PathTypeExact || *path.PathType == networking.PathTypePrefix { |
There was a problem hiding this comment.
Is there more we can or should validate here?
There was a problem hiding this comment.
Potentially yes. The KEP does not specify any additional validation, but there could be value in at least validating that the characters provided for these path types are URL safe. Happy to add that here and in the KEP if you think it would be valuable. It doesn't seem like there is any universally safe limit we could apply to the length here, but open to ideas on what else would make sense to validate here.
robscott
force-pushed
the
ingressv1-pathtype
branch
from
ff8d5a6
to
1f0e18c
Compare
robscott
force-pushed
the
ingressv1-pathtype
branch
from
1f0e18c
to
c1c2009
Compare
|
Thanks for the reviews @cmluciano and @thockin! This is ready for another look whenever you have time. |
robscott
force-pushed
the
ingressv1-pathtype
branch
from
c1c2009
to
fb3a1eb
Compare
Update cri-tools to v1.18.0 (#2)
This adds an ingress PathType as specified in the KEP (https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190125-ingress-api-group.md).