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

L7 intentions #352

Merged
merged 8 commits into from
Oct 7, 2020
Merged

L7 intentions #352

merged 8 commits into from
Oct 7, 2020

Conversation

thisisnotashwin
Copy link
Contributor

@thisisnotashwin thisisnotashwin commented Oct 7, 2020

Changes proposed in this PR:
Add support for L7 intentions

How I've tested this PR:
Local tests.
Pending: Testing via helm

How I expect reviewers to test this PR:
This PR is mostly just an update to the existing struct. At this point, there is not enough documentation for the fields but that will be updated when consul updates docs for service intentions as config entries.

@thisisnotashwin thisisnotashwin requested a review from lkysow October 7, 2020 17:42
@thisisnotashwin thisisnotashwin force-pushed the crd-controller-service-intentions branch 2 times, most recently from 37d7c9e to de6bdbc Compare October 7, 2020 17:51
Base automatically changed from crd-controller-service-intentions to crd-controller-base October 7, 2020 18:02
@thisisnotashwin thisisnotashwin requested review from a team and ishustava and removed request for a team October 7, 2020 18:22
@thisisnotashwin thisisnotashwin marked this pull request as ready for review October 7, 2020 18:22
api/v1alpha1/serviceintentions_types.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ishustava ishustava left a comment

Choose a reason for hiding this comment

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

Looks good! A couple of comments, but nothing blocking.

Comment on lines 191 to 193
if in == nil {
return nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably don't need this since a nil array is an empty array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!!

if invalidPathPrefix(in.PathExact) {
errs = append(errs, field.Invalid(path.Child("pathExact"), in.PathExact, `must begin with a '/'`))
}
return errs
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we need to validate that a valid HTTP method was provided? This is not a blocker by any means, could be added at a later time once we have more docs and stuff from consul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering the same thing. I wasn't sure if casing was a problem or if wild cards were supported which I why I punted on that for now. If the exhaustive list of methods is GET, PUT, POST, PATCH & DELETE (do you know if we support CONNECT, OPTIONS, TRACE and HEAD)??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: i had to look up MDN to get the exhaustive list of http methods 😛

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, fair enough! Definitely makes sense to punt! Haha, I don't even know what is MDN 🤯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review @ishustava 💥

@thisisnotashwin thisisnotashwin merged commit fc52045 into crd-controller-base Oct 7, 2020
@thisisnotashwin thisisnotashwin deleted the l7-intentions branch October 7, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants