Add doc on how to use Istio Authorization with Knative and Istio mesh mode by nak3 · Pull Request #2583 · knative/docs

nak3 · 2020-06-15T06:27:16Z

Proposed Changes

This patch adds doc on how to use Istio Authorization with Knative.

As described in #2569, the sample configuration of Authorizationpolicy is helpful for our users.

Part of #2569

abrennan89

@nak3 some comments 🙂 a few things could maybe do with some additional explanation. This is good information though!
cc @mpetason

abrennan89 · 2020-06-15T17:00:08Z

docs/serving/istio-authorization.md

@@ -0,0 +1,114 @@
+---
+title: "Knative application under the strict authorization policy"


It's not clear to me from this title

What "strict authorization policy" is and why someone would need it for a Knative app?

Whether this section is conceptual information or contains steps to complete a task?

I'd try to make this clearer. Maybe something like:
"Enabling requests to Knative services when additional authorization policies are enabled"

I'm also wondering whether this belongs in a more Istio specific place, @mattmoor wdyt?

What "strict authorization policy" is and why someone would need it for a Knative app?

"strict authorization policy" is an Istio's feature. It can control access to allow/deny, which Knative does not have. So someone would like to use it.

Whether this section is conceptual information or contains steps to complete a task?

It is a "contains steps to complete a task"?

I'm also wondering whether this belongs in a more Istio specific place

Yes, we definitely should belong in an Istio specific place. But I think I should open a new request to create the new hierarchy of the doc like:
https://knative.dev/docs/serving/istio/some-new-feature (istio)
https://knative.dev/docs/serving/kourier/some-new-feature (kourier)

docs/serving/istio-authorization.md

abrennan89 · 2020-06-15T17:06:09Z

docs/serving/istio-authorization.md

+When you deployed app to Knative Serving, serving system pods such as activator and autoscaler access to your app.
+Hence, you have to allow the requests to your app when you configure security features such as istio authorization policy.
+
+> Tip: This example assumes that your application enabled istio sidecar injection.


This should be a prerequisite instead, and I think this whole doc should be somewhere specific to Istio.
@evankanderson @mattmoor I think we should probably have some sort of "Post-installation tasks" section that includes things like this but I'm not sure, wdyt?

abrennan89 · 2020-06-15T17:07:21Z

docs/serving/istio-authorization.md

+> $ kubectl create namespace serving-tests
+> $ kubectl label namespace serving-tests istio-injection=enabled


Explain what these commands actually do. It looks like it's creating a namepace and labelling it for istio-injection, but it's not clear what this accomplishes. Is this how a user enables access to Knative services as mentioned earlier?

docs/serving/istio-authorization.md

Shashankft9 · 2020-06-16T11:35:22Z

Hey @nak3, I am also working on this right now, testing Istio Authorization Policies with knative-serving.
I noticed that when I install istio with sidecar using this doc: https://knative.dev/v0.14-docs/install/installing-istio/ , I further had to edit the configmap in namespace istio-system as (by default it was disabled and hence the sidecars were not being injected to the pods upon labeling the namespace):

...
data:
  config: |-
    policy: enabled
...

so maybe the flags in the helm template are wrong or this is an additional step, I haven't tested with the way istio is installed here: https://knative.dev/docs/install/installing-istio/?

nak3 · 2020-06-16T11:58:57Z

Hi @Shashankft9 Yes, it was discussed in #2073 whether it should be fixed or not. (Although it was not fixed on Istio 1.4, Istio 1.5+ can inject the sidecar with namespace label.)

Shashankft9 · 2020-06-16T12:04:00Z

ahh I see, mine is v1.4.6, that explains. So I should Instead follow the installation steps from here: https://knative.dev/docs/install/installing-istio/#installing-istio-with-sidecar-injection? Will it conflict with my knative-serving v0.14.0 because I dont see any crd kind: IstioOperator right now.

nak3 · 2020-06-16T12:08:40Z

Yes, the new Istio installation needs istioctl
command v1.5+ and it is totally different from helm's installation.

abrennan89

This looks good, thanks Kenjiro! 🙂
/lgtm

abrennan89 · 2020-06-16T13:50:47Z

/hold

abrennan89 · 2020-06-16T13:51:42Z

@nak3 do we need any other reviews from technical / eng folks here or is this OK to merge? I've put it on hold for now but it looks fine to me.

nak3 · 2020-06-16T14:24:51Z

Thank you @abrennan89
Yes, I think we should get the lgtm from one more folks.

/cc @tcnghia @josueetcom @ZhiminXiang @itsmurugappan
Would you mind taking a look?

Shashankft9 · 2020-06-16T15:07:20Z

maybe add what Using net-istio for your Knative Ingress signifies and what to do in order to make sure its being used - in the Before you begin section?

docs/serving/istio-authorization.md

itsmurugappan · 2020-06-16T15:33:24Z

docs/serving/istio-authorization.md

+        paths:
+        - /     # The path for your application.
+EOF
+```


Its worth mentioning that, If you use principal and namespace in the policies, Mutual TLS need to be enabled. If you are using jwt claims, mtls is not a prereq.

Yes, we should. I updated the doc with the link to Istio doc.

itsmurugappan · 2020-06-16T15:36:59Z

overall looks good. Just a few nits from my side.

docs/serving/istio-authorization.md

josueetcom · 2020-06-18T21:16:52Z

docs/serving/istio-authorization.md

@@ -0,0 +1,90 @@
+---
+title: "Enabling requests to Knative services when additional authorization policies are enabled"


Is this a doc for specifically the case when additional authorization policies are enabled or is it a general doc for using Istio authorization with Knative and Istio mesh mode (per the PR title)?

Yes, It is a general doc for using Istio authorization with Knative and Istio mesh mode.

Hmm, maybe I'm lacking context but when I read the title it does immediately suggest this is specifically for Istio (do other networking layers have authorization policies?).

docs/serving/istio-authorization.md

josueetcom

Just some minor nits, otherwise LGTM

docs/serving/istio-authorization.md

josueetcom · 2020-06-22T19:00:23Z

docs/serving/istio-authorization.md

@@ -0,0 +1,90 @@
+---
+title: "Enabling requests to Knative services when additional authorization policies are enabled"


Hmm, maybe I'm lacking context but when I read the title it does immediately suggest this is specifically for Istio (do other networking layers have authorization policies?).

docs/serving/istio-authorization.md

knative-prow-robot · 2020-06-22T20:38:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, josueetcom, nak3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abrennan89]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nak3 · 2020-06-23T12:39:19Z

Thank you so much for the reviews!

Just one note, In addition to reflecting the review comments, I removed the "allowing list by namespace" example by e725e97. Because it allows all traffic via activator and it is very confusable and easy to create a security hall.
I already got the two feedback in #1130.

nak3 · 2020-06-29T23:12:38Z

/unhold

nak3 · 2020-06-29T23:16:34Z

@abrennan89 This is ready to merge, I think. Could you please add /lgtm again?

abrennan89 · 2020-06-30T13:44:47Z

/lgtm
Thanks, Kenjiro! 🙂

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 15, 2020

knative-prow-robot requested review from markusthoemmes and mattmoor June 15, 2020 06:27

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 15, 2020

nak3 mentioned this pull request Jun 15, 2020

Documentation on how to use Istio Authorization with Knative #2569

Closed

abrennan89 suggested changes Jun 15, 2020

View reviewed changes

abrennan89 linked an issue Jun 15, 2020 that may be closed by this pull request

Documentation on how to use Istio Authorization with Knative #2569

Closed

abrennan89 added status/in-progress kind/serving lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. priority/medium labels Jun 15, 2020

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 16, 2020

abrennan89 reviewed Jun 16, 2020

View reviewed changes

knative-prow-robot assigned abrennan89 Jun 16, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020

abrennan89 approved these changes Jun 16, 2020

View reviewed changes

knative-prow-robot added the approved label Jun 16, 2020

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 16, 2020

knative-prow-robot requested review from itsmurugappan and josueetcom June 16, 2020 14:24

knative-prow-robot requested review from ZhiminXiang and tcnghia June 16, 2020 14:24

itsmurugappan reviewed Jun 16, 2020

View reviewed changes

docs/serving/istio-authorization.md Outdated Show resolved Hide resolved

docs/serving/istio-authorization.md Outdated Show resolved Hide resolved

docs/serving/istio-authorization.md Outdated Show resolved Hide resolved

itsmurugappan reviewed Jun 16, 2020

View reviewed changes

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2020

mattmoor reviewed Jun 17, 2020

View reviewed changes

josueetcom reviewed Jun 18, 2020

View reviewed changes

nak3 mentioned this pull request Jun 22, 2020

Need instructions about how to configure mTLS within Knative cluster #1130

Closed

josueetcom approved these changes Jun 22, 2020

View reviewed changes

nak3 added 10 commits June 23, 2020 19:37

Add doc on how to use Istio Authorization with Knative

a464b67

Fix nit

d12e364

Fix nit 2

9d72b92

Fix nit 3

9e1488c

Fix review comments

cad71d6

Replace health with healthz

8d93bdb

Add comment for mTLS requirment

2b89f5d

Fix review comment

453400a

Fix review comments

581b45c

Remove allowing list by namespace

e725e97

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2020

knative-prow-robot merged commit 8bb7a41 into knative:master Jun 30, 2020

		@@ -0,0 +1,114 @@
		---
		title: "Knative application under the strict authorization policy"

		> $ kubectl create namespace serving-tests
		> $ kubectl label namespace serving-tests istio-injection=enabled

		@@ -0,0 +1,90 @@
		---
		title: "Enabling requests to Knative services when additional authorization policies are enabled"

Conversation

nak3 commented Jun 15, 2020

Proposed Changes

Uh oh!

abrennan89 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Shashankft9 commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nak3 commented Jun 16, 2020

Uh oh!

Shashankft9 commented Jun 16, 2020

Uh oh!

nak3 commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abrennan89 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abrennan89 commented Jun 16, 2020

Uh oh!

abrennan89 commented Jun 16, 2020

Uh oh!

nak3 commented Jun 16, 2020

Uh oh!

Shashankft9 commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

itsmurugappan commented Jun 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

josueetcom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

knative-prow-robot commented Jun 22, 2020

Uh oh!

Shashankft9 commented Jun 16, 2020 •

edited

Loading

nak3 commented Jun 16, 2020 •

edited

Loading

abrennan89 left a comment •

edited

Loading