Add new Kafka Role to the docs #3186

manalibhutiyani · 2018-03-16T23:02:49Z

Fixes: #3118
Signed-off-by: Manali Bhutiyani manali@covalent.io

Add new Kafka Role to the docs

manalibhutiyani · 2018-03-16T23:03:04Z

test-me-please

manalibhutiyani · 2018-03-17T06:26:21Z

test-me-please

manalibhutiyani · 2018-03-17T19:14:01Z

test-me-please

joestringer

Looks good, minor wording suggestions.

joestringer · 2018-03-18T00:09:27Z

Documentation/policy/language.rst

-  message. If set, it has to be a string representing a positive integer. If
-  omitted or empty, all versions are allowed.
+Role
+  Role is a case-insensitive string and describes a group of API keys


case-insensitive string which describes...

joestringer · 2018-03-18T00:11:14Z

Documentation/policy/language.rst

+  The following values are supported:
+	- "produce": Allow producing to the topics specified in the rule
+	- "consume": Allow consuming from the topics specified in the rule
+  This field is incompatible with the APIKey field, i.e either APIKey or Role


either APIKey or Role can be specified -> APIKey and Role cannot both be specified in the same rule

joestringer · 2018-03-18T00:12:21Z

pkg/policy/api/rule.go

-	//
-	// If omitted or empty, the field has no effect and the logic of the APIKey
-	// field applies.
+	// This field is incompatible with the APIKey field, i.e either APIKey or Role


If you take my suggestion above, this one needs updating too.

ianvernon · 2018-03-18T18:31:48Z

Documentation/policy/language.rst

+	- "consume": Allow consuming from the topics specified in the rule
+  This field is incompatible with the APIKey field, either APIKey or Role
+  may be specified. If omitted or empty, all keys are allowed, if APIKey is also
+  the empty


... is also empty

manalibhutiyani · 2018-03-19T20:32:13Z

test-me-please

manalibhutiyani · 2018-03-19T21:00:58Z

test-me-please

manalibhutiyani · 2018-03-19T21:02:29Z

test-me-please

manalibhutiyani · 2018-03-19T21:03:05Z

@ianvernon @joestringer : Addressed review comments. Need an approval if everything looks ok.

manalibhutiyani · 2018-03-19T22:14:14Z

@eloycoto : Still seeing this test flake unrelated to my docs changes

21:46:33 
21:46:33 • Failure [200.479 seconds]
21:46:33 K8sValidatedPolicyTestAcrossNamespaces
21:46:33 /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/k8sT/Policies.go:675
21:46:33   Policies Across Namespaces [It]
21:46:33   /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/k8sT/Policies.go:712
21:46:33 
21:46:33   Unable to connect between frontend-d9q97 and 10.97.83.36:80/
21:46:33   Expected
21:46:33       <string>: 000command terminated with exit code 7
21:46:33       
21:46:33   to equal
21:46:33       <string>: 200
21:46:33 
21:46:33   /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/src/github.com/cilium/cilium/test/k8sT/Policies.go:855
21:46:33 ------------------------------
21:46:33 SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSS```

manalibhutiyani · 2018-03-19T22:14:37Z

test-me-please

tgraf · 2018-03-19T21:49:40Z

Documentation/policy/language.rst

+
+  This field is incompatible with the APIKey field, i.e APIKey and Role
+  cannot both be specified in the same rule.
+  If omitted or empty, all keys are allowed.


This last sentence is not true. If omitted, APIKey can still be specified. This needs to be clear.

Changing this to : If omitted or empty, and if APIKey is not specified then all keys are allowed.

tgraf · 2018-03-19T23:02:52Z

Documentation/policy/language.rst

-~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+Only allow producing to topic empire-announce using Role
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+


This needs a sentence here that describes the motivation of role and when to use roles and when to use apiKeys.

Planning to add this:

PortRuleKafka is a list of Kafka protocol constraints. All fields are optional, if all fields are empty or missing, the rule will match all Kafka messages. There are two ways to specify the Kafka rules. We can choose to specify a high level "produce" or "consume" role to a topic or choose to specify more low level kafka protocol specific apiKeys. Thus the rule allows us to specify both high level role constructs as well as low level Kafka protocol constructs.

done, please let me know if you think this needs to be improved.

tgraf · 2018-03-19T23:03:18Z

examples/policies/l7/kafka/kafka-Role.yaml

@@ -0,0 +1,23 @@
+apiVersion: "cilium.io/v2"


Rename the filename to all lowercase

tgraf · 2018-03-19T23:03:55Z

pkg/policy/api/rule.go

 	//
+	// If omitted or empty, all keys are allowed.


This needs to be adjusted.

Changed to If omitted or empty, and if Role is not specified, then all keys are allowed.

manalibhutiyani · 2018-03-19T23:48:57Z

test-me-please

manalibhutiyani · 2018-03-20T02:19:38Z

I am seeing a lot of unrelated CI errors. Not sure what's the best route here? Trying CI again.

manalibhutiyani · 2018-03-20T02:19:47Z

test-me-please

tgraf · 2018-03-20T02:21:11Z

I am seeing a lot of unrelated CI errors. Not sure what's the best route here? Trying CI again.

All PRs are seeing these right now. Nee to investigate tomorrow.

ianvernon · 2018-03-20T04:52:10Z

Documentation/policy/language.rst

@@ -524,20 +524,38 @@ Kafka (Tech Preview)

 PortRuleKafka is a list of Kafka protocol constraints. All fields are optional,
 if all fields are empty or missing, the rule will match all Kafka messages.
+There are two ways to specify the Kafka rules. We can choose to specify a
+high level "produce" or "consume" role to a topic or choose to specify more


ianvernon · 2018-03-20T04:52:25Z

Documentation/policy/language.rst

@@ -524,20 +524,38 @@ Kafka (Tech Preview)

 PortRuleKafka is a list of Kafka protocol constraints. All fields are optional,
 if all fields are empty or missing, the rule will match all Kafka messages.
+There are two ways to specify the Kafka rules. We can choose to specify a
+high level "produce" or "consume" role to a topic or choose to specify more
+low level kafka protocol specific apiKeys. Thus the rule allows us to specify


low-level

kafka --> Kafka

ianvernon · 2018-03-20T04:52:33Z

Documentation/policy/language.rst

+There are two ways to specify the Kafka rules. We can choose to specify a
+high level "produce" or "consume" role to a topic or choose to specify more
+low level kafka protocol specific apiKeys. Thus the rule allows us to specify
+both high level role constructs as well as low level Kafka protocol constructs.


high-level ... low-level

These last two sentences appear to state the same thing. Perhaps replace the last sentence with something like:

"Writing rules based on Kafka roles is easier and covers most common use cases, however if more granularity is needed then users can alternatively write rules using specific apiKeys."

ianvernon · 2018-03-20T04:52:39Z

Documentation/policy/language.rst


 The following fields can be matched on:

+Role
+  Role is a case-insensitive string which describes a group of API keys
+  necessary to perform certain higher level Kafka operations such as "produce"


higher-level

ianvernon · 2018-03-20T04:52:44Z

Documentation/policy/language.rst

+  Role is a case-insensitive string which describes a group of API keys
+  necessary to perform certain higher level Kafka operations such as "produce"
+  or "consume". A Role automatically expands into all APIKeys required
+  to perform the specified higher level operation.


higher-level

manalibhutiyani · 2018-03-20T18:04:46Z

test-me-please

This change fixes the `Policy` docs to reflect the new `Kafka Role` Fixes: #3118 Signed-off-by: Manali Bhutiyani <manali@covalent.io>

…ents This change makes the `PortRuleKafka` fields consistent with the docs and also improves/corrects the comments. Fixes: #3118 Signed-off-by: Manali Bhutiyani <manali@covalent.io>

…presentation The yaml and json representation of kafka policy was inconsistent. This change fixes this inconsistency. Fixes: #3118 Signed-Off-By: Manali Bhutiyani <manali@covalent.io>

…role Fixes: #3118 Signed-Off-By: Manali Bhutiyani <manali@covalent

manalibhutiyani added wip release-note/bug This PR fixes an issue in a previous release of Cilium. labels Mar 16, 2018

manalibhutiyani requested review from a team as code owners March 16, 2018 23:02

manalibhutiyani force-pushed the kafka-docs branch 2 times, most recently from d942605 to 610d560 Compare March 17, 2018 06:26

manalibhutiyani added pending-review and removed wip labels Mar 17, 2018

manalibhutiyani force-pushed the kafka-docs branch from 610d560 to 1655cc1 Compare March 17, 2018 19:05

manalibhutiyani mentioned this pull request Mar 17, 2018

Docs: Add new Kafka Role to the docs #3118

Closed

joestringer reviewed Mar 18, 2018

View reviewed changes

ianvernon requested changes Mar 18, 2018

View reviewed changes

manalibhutiyani force-pushed the kafka-docs branch 2 times, most recently from 0f231d7 to d28ae7a Compare March 19, 2018 20:28

manalibhutiyani force-pushed the kafka-docs branch 2 times, most recently from fd9eaa0 to cdeead4 Compare March 19, 2018 21:00

manalibhutiyani force-pushed the kafka-docs branch from cdeead4 to 34f03ca Compare March 19, 2018 21:02

tgraf requested changes Mar 19, 2018

View reviewed changes

tgraf added priority/1.0-blocker and removed priority/1.0-blocker labels Mar 19, 2018

manalibhutiyani force-pushed the kafka-docs branch 2 times, most recently from 4820782 to 772cd7c Compare March 19, 2018 23:43

tgraf approved these changes Mar 19, 2018

View reviewed changes

ianvernon requested changes Mar 20, 2018

View reviewed changes

manalibhutiyani force-pushed the kafka-docs branch from 772cd7c to b62c833 Compare March 20, 2018 18:03

Manali Bhutiyani added 4 commits March 20, 2018 12:27

Docs: Add new Kafka Role to the docs

63087c2

This change fixes the `Policy` docs to reflect the new `Kafka Role` Fixes: #3118 Signed-off-by: Manali Bhutiyani <manali@covalent.io>

policy: Make the Kafka api consistent with the docs, improve the comm…

d4125be

…ents This change makes the `PortRuleKafka` fields consistent with the docs and also improves/corrects the comments. Fixes: #3118 Signed-off-by: Manali Bhutiyani <manali@covalent.io>

policy: Make the kafka policy in the docs consistent with the yaml re…

105be17

…presentation The yaml and json representation of kafka policy was inconsistent. This change fixes this inconsistency. Fixes: #3118 Signed-Off-By: Manali Bhutiyani <manali@covalent.io>

policy: Fix the L7 policy to have the correct yaml field i.e Role -> …

dd4e172

…role Fixes: #3118 Signed-Off-By: Manali Bhutiyani <manali@covalent

manalibhutiyani force-pushed the kafka-docs branch from b62c833 to dd4e172 Compare March 20, 2018 19:27

manalibhutiyani merged commit fbd2236 into master Mar 20, 2018

manalibhutiyani deleted the kafka-docs branch March 20, 2018 19:27

Add new Kafka Role to the docs #3186

Add new Kafka Role to the docs #3186

Conversation

manalibhutiyani commented Mar 16, 2018

manalibhutiyani commented Mar 16, 2018

manalibhutiyani commented Mar 17, 2018

manalibhutiyani commented Mar 17, 2018

joestringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manalibhutiyani commented Mar 19, 2018

manalibhutiyani commented Mar 19, 2018

manalibhutiyani commented Mar 19, 2018

manalibhutiyani commented Mar 19, 2018

manalibhutiyani commented Mar 19, 2018 • edited Loading

manalibhutiyani commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manalibhutiyani Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manalibhutiyani commented Mar 19, 2018

manalibhutiyani commented Mar 20, 2018

manalibhutiyani commented Mar 20, 2018

tgraf commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joestringer Mar 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manalibhutiyani commented Mar 20, 2018

manalibhutiyani commented Mar 19, 2018 •

edited

Loading

manalibhutiyani Mar 19, 2018 •

edited

Loading

joestringer Mar 20, 2018 •

edited

Loading