-
Notifications
You must be signed in to change notification settings - Fork 28
Update Serverless + Service Mesh integration documentation #97
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
Conversation
✅ Deploy Preview for jazzy-shortbread-5f62b7 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Add Eventing diagram for Istio authorization policies
Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments but LGTM overall.
modules/serverless-common/pages/service-mesh/common-service-mesh-network-isolation.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-network-isolation.adoc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, this a is major step forward and a great improvements for the docs. I did an in-depth review with some proposals that might be difficult to imlplement. Let's discuss them on the topic.
One overall comment that might also affects other parts of the documentation: I would prefer to use kn service create
instead of adding the plain yaml files because its much shorter and focus on the important parts. See the example that I have added when verifying the mt setup.
A part that might be a bit controversial is how we organise the content in the ToC. Combining Serving and Eventing right now might be the right thing, but will become later difficult in case we want to split Serving and Eventing apart into separatre products. But see my comments above.
@@ -0,0 +1,4 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the graphic, but I wonder whether this will work in the OpenShift docs with having a lot of blurb around (navigation, headers), so screen real estate might be an issue. I wonder whether we could shorten the text and maybe add some callouts to just the resource type ? Details can then be in the flow text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not even sure if we even can add visuals to the official docs as they would need resources to re-draw the visuals themselves (in the OCP docs style). Lets discuss that point with docs first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, but I'm pretty sure that one of the feedback will be that we need to move out the text from the graphics. So we could already do this improvement to ease the discussion with docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pierDipi for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I will work on it
...s/serverless-common/assets/images/service-mesh/multi-tenancy-service-mesh-Serving.drawio.svg
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-network-isolation.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-network-isolation.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Many review comments were already provided. I'm adding a few more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the blue arrows.
- Arrow 1) seems correct to me
- Arrow 2) - I'm missing the number "2)" next to the arrow
- Arrow 3) - the arrow has "3)" next to it but the description for "3)" at the bottom of the picture seems to belong to the blue arrow that doesn't have any number next to it. Just because it speaks about "tenant-2" namespace but the arrow with "3)" comes from "knative-eventing". Not sure if we can make it more clear...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was previously asking @pierDipi whether it is possible for the sender to sneak-in the Kn-Namespace header to act like it can communicate with the tenants. The answer was something like "it is only added by Eventing core" so it's not possible to sneak it in.
But here I see "ksvc" in tenant-2 and it looks like this service needs to provide "Kn-Namespace" by itself. Is it true? Is it possible to abuse communication by somebody who is not supposed to belong to the tenants?
Note: We still need to write tests for this. That's TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pierDipi can you answer those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mgencur I think to make 3 more clear I can "slice" sender and receiver with "tenant-1" and "tenant-2"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I will try to answer it myself. The ksvc is NOT supposed to set Kn-Namespace header and the AuthorizatioPolicy we have is defining the "source" namespace as "knative-eventing" so the Kn-Namespace header is taken into account only when sent by a component from knative-eventing ns:
- from:
- source:
namespaces:
- "knative-eventing"
when:
- key: request.headers[Kn-Namespace]
values:
- "tenant-1"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, @mgencur for the other comment, that's the idea, only when coming from our own namespace it's expected to match the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For 2 and 3, I've updated the diagram and the legend in ReToCode#3
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Outdated
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-setup.adoc
Show resolved
Hide resolved
modules/serverless-common/pages/service-mesh/common-service-mesh-network-isolation.adoc
Outdated
Show resolved
Hide resolved
I have reviewed the new commit: https://deploy-preview-97--jazzy-shortbread-5f62b7.netlify.app/docs/latest/serverless/serving/serving-with-ingress-sharding LGTM 🎉 |
@ReToCode thanks a lot for the refactoring, looks good to me. |
@rhuss I added the |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ReToCode, rhuss 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 |
modules/serverless/pages/service-mesh/common-service-mesh-network-isolation.adoc
Show resolved
Hide resolved
…sion of the diagram Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Update eventing diagram to use textual legend and a tenant-sliced version of the diagram
@mgencur PTAL: #97 (comment) . If you are ok with that, I think we can merge this. |
/unhold |
It looks good to me. Thanks! |
For JIRAs:
Changes