-
Notifications
You must be signed in to change notification settings - Fork 123
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: deployment experience integration allow list for cmd API #287
Conversation
375a267
to
46f4bc3
Compare
Pull Request Test Coverage Report for Build 498124943
💛 - Coveralls |
{"empty", "", false}, | ||
{"allowed", "nri-lsi-java", true}, | ||
{"not allowed", "", false}, | ||
{"extra suffix", "nri-lsi-java-foo", false}, |
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.
'empty' and 'not allowed' are the same case. And 'extra suffix' is not allowed as well right?
@@ -54,6 +56,10 @@ func NewHandler(definitionQ chan<- integration.Definition, il integration.Instan | |||
return | |||
} | |||
|
|||
if cmdapi.IsAllowedToRunStopFromCmdAPI(args.IntegrationName) { | |||
return ErrIntNotAllowed | |||
} |
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.
Make sense to move it to be the first we validate. Therefore we avoid to 'Unmarshal' if there is no need.
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.
only previous check is empty case, and I think that's correct, as it should provide its own error, otherwise this IsAllowedToRunStopFromCmdAPI
should return a 3rd case (not bool anymore) where it says empty case is not handled
@@ -26,7 +27,8 @@ const cmdName = "run_integration" | |||
|
|||
// Errors | |||
var ( | |||
ErrNoIntName = errors.New("missing required \"integration_name\"") | |||
ErrNoIntName = errors.New("missing required \"integration_name\"") | |||
ErrIntNotAllowed = errors.New("integration now allowed to run/stop from command channel") |
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.
small typo here? "now" -> "not"?
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
For security reasons, command channel won't allow arbitrary integration executions. These ones should be listed in the allow-list.