fix(nodes): reviewed kubernetes.nodes implementation #399
Conversation
- Changed node_log tool name to nodes_log for consistency - Added access control check for nodes and configured denied resources - Migrated tests to testify - Added complete test coverage for nodes_log https://kubernetes.io/docs/concepts/cluster-administration/system-logs/#log-query Signed-off-by: Marc Nuri <[email protected]>
|
Thanks @manusa, One other thing that I have noticed is that there is no default value for the tailLines. Should we use something like 500? |
At the moment there's a default value of 100 (you set this in the initial impl): Clients should preset this value. However, the value is not enforced. Maybe the minimum is not checked either (I need to verify this). |
Signed-off-by: Marc Nuri <[email protected]>
|
I added an additional test to verify the current behavior with negative (unsupported) tail arguments. In this case, we're not validating and simply defaulting to show the entire log. |
| switch v := tail.(type) { | ||
| case float64: | ||
| tailInt = int64(v) | ||
| case int: case int64: | ||
| tailInt = int64(v) | ||
| case int: | ||
| case int64: | ||
| tailInt = v |
There was a problem hiding this comment.
@manusa I feel like I've seen this code/similar code in other places in the codebase - is it worth extracting to a util?
Or with the sdk migration I guess if we define the paramaters to a tool using a struct with json tags, this would be handled by the sdk
There was a problem hiding this comment.
It's in the same function for pod logs (although it's not exactly the same),
With our approach towards the go-sdk and decoupled toolsets, it might not be the case that the SDK takes care of this, but I need to evaluate first.
If it's not the case, and we do want to support reading number values with diverse input format or types, we definitely need to extract this to some util.
Similarly to extract strings, etc.
3 participants
Follows up on #384
/cc @blublinsky
It also seems that the tailLines param is not placed in the expected location. Will look into it as a subsequent fix: (might be an issue with the cluster itself)
https://kubernetes.io/docs/concepts/cluster-administration/system-logs/#log-query