-
Notifications
You must be signed in to change notification settings - Fork 571
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
WIP implement stub for /v1/actions/execute-command #8064
WIP implement stub for /v1/actions/execute-command #8064
Conversation
…teCommand message for ExecuteCommand Endpoint
--
|
@lippserd Damn! The actions handler doesn't allow to do something for each object (trigger execution) and then do something else for each object (wait). Shall we just omit waiting? |
Ok!
In this case, for relaying the
Yes, when collecting the results, the actions handler defaults to 500 whenever Thanks for the feedback for now! We'll get to the implementation asap. |
Are you going to let the agent execute the command? See here. |
Ah, right. Feel free to check not for 200, but for 200-299. |
Thanks! Currently, we check for the range, but left the Do you think it should be propagated from the results? |
200 is OK. |
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.
Don't forget to say like we're done, please review.
Ok. We just drop the @benjamingroeber The idea of the |
Hi @Al2Klimov We noticed that the API does not forward cluster messages to itself (which makes sense), such that we need a separate branch for the handling of Thanks! |
Interesting, please share the code location. |
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.
Now this stub implementation...
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.
These two stub implementations are (more than?) enough for a single PR.
Write a test protocol (as comment here) for each of them:
- API action: demonstrate the parameter validation, the checkable property update (locally) and the correctly sent cluster messages with curl, the debug log and the Icinga console
- Cluster message handler: demonstrate the checkable property update with the same tools
Write it replicably so one of us can verify the tests
This is the configuration that can be used to run the tests: Master configurationconstants.conf
hosts.conf
services.conf
zones.conf
api-users.conf
Satellite configurationconstants.conf
hosts.conf
services.conf
zones.conf
TestsBefore the tests
Test correct request
result:
log output master:
log output satellite:
cli:
Test wrong command_type
result:
log output master:
log output satellite: cli:
Test ttl is required
result:
log output master:
log output satellite: cli:
Test ttl is negative
result:
log output master:
log output satellite: cli:
Test macros is not a dictionary
result:
log output master:
log output satellite: cli:
Test invalid endpoint
result:
log output master:
log output satellite: cli:
Test invalid CheckCommand
result:
log output master:
log output satellite: cli:
Tests on satellite
Test correct request
result:
log output master:
log output satellite:
cli:
Tests with a limited userTest allowed endpoint
result:
log output master:
log output satellite: cli:
Test denied endpoint
result:
log output master:
log output satellite: cli:
|
And the logs with the cluster messages? And the debug console outputs with the changed checkable executions attribute? |
You're right, we have updated the comment. |
You seem to have ordered it right in the code. |
... so you don't need to care about it for now. |
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.
Danm. {"PHXL0262!ping":"329aa56e-93c5-447f-8099-f8cdcb045d38","code":202.0,"status":"Accepted"}
is not optimal as "PHXL0262!ping"
may also be "code"
or "status"
(host named like this).
Change it to this: {"checkable":"PHXL0262!ping","execution":"329aa56e-93c5-447f-8099-f8cdcb045d38","code":202.0,"status":"Accepted"}
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.
And one last thing for now:
The checkable is already there. Don't just GetByName()
other objects like the endpoint. Instead use this function with this permission.
Rationale: This new action shall not be a workaround for querying forbidden objects.
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.
LGTM. And now please a test protocol for allow and denial of an existing object for one of the resolved types.
We added the tests in the comment above |
@Yonas-net Check out this PR on master + sat and verify the test protocol. |
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 have copied everything one by one and works great 👍 .
Feel free to implement the remaining stuff now. (New branch, PR into the same branch as here.) |
Hi guys!
This is the first step for the implementation of #8040 / #8034 . We started the implementation this week with the first stub, and will continue working on this branch until all stubs are implemented.
We would like to have some feedback from your side before proceeding, to verify that we're on the right path.
During the implementation the following questions arose, which would need some quick feedback from you:
ttl
parameter have a default value?new MessageOrigin()
the correct way to obtain the object for the cluster messages?wait=false
we expect the HTTP return code to be 202, however currently the API action handler will always return a 500 error code. What is the desired behavior here? Currently OK results are favored over everything else.Regarding the implementation of the next stubs:
wait=true
parameter? In specific how would you handle the necessary "downtime" while waiting for the result of the Command?For sure there is a lot of space fore improvement, and we're excited to hear from you! If you prefer, we're also available for a short call if you prefer @lippserd .
Cheers!