-
Notifications
You must be signed in to change notification settings - Fork 337
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
feat(kuma-cp): add support for inspect api for new kind Dataplane and section name for selecting single inbound #12644
Open
Automaat
wants to merge
3
commits into
kumahq:master
Choose a base branch
from
Automaat:feat/add-support-for-dataplane-kind-inspect-api
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+148
−1
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
85 changes: 85 additions & 0 deletions
85
...a/resources/inspect/dataplanes/_rules/dataplane_kind_meshtimeout_section_name.golden.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| { | ||
| "httpMatches": [], | ||
| "resource": { | ||
| "labels": { | ||
| "app": "demo-app" | ||
| }, | ||
| "mesh": "mesh-1", | ||
| "name": "dp-1", | ||
| "type": "Dataplane" | ||
| }, | ||
| "rules": [ | ||
| { | ||
| "fromRules": [ | ||
| { | ||
| "inbound": { | ||
| "name": "secondary-port", | ||
| "port": 9090, | ||
| "tags": { | ||
| "kuma.io/service": "bar" | ||
| } | ||
| }, | ||
| "rules": [ | ||
| { | ||
| "conf": { | ||
| "connectionTimeout": "7s", | ||
| "idleTimeout": "7s", | ||
| "http": { | ||
| "requestTimeout": "2s" | ||
| } | ||
| }, | ||
| "matchers": [], | ||
| "origin": [ | ||
| { | ||
| "labels": {}, | ||
| "mesh": "mesh-1", | ||
| "name": "select-whole-dpp", | ||
| "type": "MeshTimeout" | ||
| }, | ||
| { | ||
| "labels": {}, | ||
| "mesh": "mesh-1", | ||
| "name": "select-single-inbound", | ||
| "type": "MeshTimeout" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| }, | ||
| { | ||
| "inbound": { | ||
| "name": "main-port", | ||
| "port": 8080, | ||
| "tags": { | ||
| "kuma.io/service": "foo" | ||
| } | ||
| }, | ||
| "rules": [ | ||
| { | ||
| "conf": { | ||
| "connectionTimeout": "7s", | ||
| "idleTimeout": "7s", | ||
| "http": { | ||
| "requestTimeout": "7s" | ||
| } | ||
| }, | ||
| "matchers": [], | ||
| "origin": [ | ||
| { | ||
| "labels": {}, | ||
| "mesh": "mesh-1", | ||
| "name": "select-whole-dpp", | ||
| "type": "MeshTimeout" | ||
| } | ||
| ] | ||
| } | ||
| ] | ||
| } | ||
| ], | ||
| "toResourceRules": [], | ||
| "toRules": [], | ||
| "type": "MeshTimeout", | ||
| "warnings": [] | ||
| } | ||
| ] | ||
| } |
53 changes: 53 additions & 0 deletions
53
...ta/resources/inspect/dataplanes/_rules/dataplane_kind_meshtimeout_section_name.input.yaml
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| #/meshes/mesh-1/dataplanes/dp-1/_rules 200 | ||
| type: Mesh | ||
| name: mesh-1 | ||
| --- | ||
| type: Dataplane | ||
| name: dp-1 | ||
| mesh: mesh-1 | ||
| labels: | ||
| app: demo-app | ||
| networking: | ||
| address: 127.0.0.1 | ||
| inbound: | ||
| - port: 8080 | ||
| name: main-port | ||
| tags: | ||
| kuma.io/service: foo | ||
| - port: 9090 | ||
| name: secondary-port | ||
| tags: | ||
| kuma.io/service: bar | ||
| --- | ||
| type: MeshTimeout | ||
| name: select-whole-dpp | ||
| mesh: mesh-1 | ||
| spec: | ||
| targetRef: | ||
| kind: Dataplane | ||
| labels: | ||
| app: demo-app | ||
| from: | ||
| - targetRef: | ||
| kind: Mesh | ||
| default: | ||
| idleTimeout: 7s | ||
| connectionTimeout: 7s | ||
| http: | ||
| requestTimeout: 7s | ||
| --- | ||
| type: MeshTimeout | ||
| name: select-single-inbound | ||
| mesh: mesh-1 | ||
| spec: | ||
| targetRef: | ||
| kind: Dataplane | ||
| labels: | ||
| app: demo-app | ||
| sectionName: secondary-port | ||
| from: | ||
| - targetRef: | ||
| kind: Mesh | ||
| default: | ||
| http: | ||
| requestTimeout: 2s |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| "fromRules": [ | ||
| { | ||
| "inbound": { | ||
| "name": "", | ||
| "port": 8080, | ||
| "tags": { | ||
| "kuma.io/service": "foo" | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
why is the
namerequired?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've made it required, just so it is not pointer to. string but I can change this if you think it should not be required
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.
Technically it doesn't matter as an empty value
name: ""meansnameis not set. Could you please check with @johncowen or @schogges to understand what's the preferred way for such kind of situation?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 think it's better to omit
nameif it's not set. Theoretically it doesn't really matter because we'd catch that in our data-layer, but ifnameis an optional property, I'd treat it as such 🤔 from a TypeScript perspective I think it's better because we are forced to catch that case. An empty string is falsy, but we are not forced to handle that case. If we leave it as an empty string, we'd have to know that it could be an empty string and handle that.For consistency reasons, are there similar cases and how do we handle that in these cases?
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.
TLDR; I'd go with
name: ""if not set.I'd say a non-union type is always best. So one single type, i.e. empty string
""in the case of strings,0(or sometimes even-1) in the case of numbers, arrays/slices should be empty[], objects are slightly trickier but generally{}. No three way booleans, alwaystrueorfalse, never omitted/undefinedNo omitted's,
undefineds ornullsThis makes the properties of an API response stable and clients are then less prone to error when they expect a property to be there and it suddenly isn't.
We can fix these up in our data layer if necessary (and this is what we've been doing with older APIs), but moving forwards it would be great if the HTTP APIs follow this thinking also.
Theoretically the GUI is not the only HTTP API client, and not everyone will have a thin layer in between to smooth out any rough edges.
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 guess someone might be doing a bash thing with curl, so IMO I'd say we can't fully expect that.
I think the other thing, even if people are using the specs, they still have to add some sort of guard to check that
nameis there before doing something with it. This could be multiplied across a codebase (unless you have a centralised place like we do), and then you could multiply that by the amount of people using the HTTP API. That's a lot of unnecessary codeAlternatively, we could put whatever logic is needed to set
name: ""in the backend (i.e. at source) instead then folks using the HTTP API don't have to check for existence before usingnameThere 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.
Regardless of how the api might be consumed, there are plenty ways to check what is in the specs. Even when ignoring the specs and inspecting an API response and the
nameis not always returned one could infer that it's an optional property.I doubt that this is our problem 😅 also when I build something on top of the api and want to exclude the name when it's not set, it doesn't matter if it's returned or an empty string. In any case I have to check for that.
But they would have to check why it's an empty string and what it means. They could even think it's a bug. This is just not obvious from my point of view and I think this could cause some confusion.
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.
Well it looks like we have pretty strong opposite opinions, so kinda feel like we aren't helping here 😅
@Automaat @lobkovilya any thoughts?
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.
❓ Are Inbound
names unique?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'm leaning towards John opinion, to avoid nullable values in api. I think they should be unique, on Kubernetes they will be unique, on universal I think we should add validation for this. What do you think about it @lobkovilya?