-
Notifications
You must be signed in to change notification settings - Fork 23
Adding regex validation for SSM parameters in nodeadm init command #595
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
Conversation
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.
Just a thought, not suggesting one way or another, currently the debug command deos require the config, but we do not necessarily need all of it. like for example if the ssm stuff isnt in there, debug doesnt really care since its intended to be run after the init process where the ssm will either register or fail.
im not opposed to adding these validations on the config in the debug command, just wanted to call out that some (maybe most) of the specifics in the config do not matter to the debug command.
Unit test is failing. Please fix them. |
The sample you show in the PR description for invalid ACTIVATION_CODE seems not correct. It showed invalid ACTIVATION_ID. |
ef4a757
to
4089719
Compare
Ack, String match error in unit test. Fixed in #2. |
Agree, debug command, looks designed to run after Init. However, the debug command can also run without the init command or with a failed init configuration.
|
25a1462
to
66aef5a
Compare
66aef5a
to
ebff073
Compare
cmd/nodeadm/debug/debug.go
Outdated
@@ -92,6 +93,8 @@ func (c *debug) Run(log *zap.Logger, opts *cli.GlobalOptions) error { | |||
runner := validation.NewRunner[*api.NodeConfig](printer) | |||
apiServerValidator := node.NewAPIServerValidator(kubelet.New()) | |||
clusterProvider := kubernetes.NewClusterProvider(awsConfig) | |||
|
|||
runner.Register(hybrid.NewHybridNodeConfigValidator(nodeConfig, log)) |
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.
lets just remove this from debug for now, per previous discussions with @g-gaston. debug is not meant to be run until init has succeeded. If a user does run debug before getting a successful init, most of the checks are going to return back errors because its expecting the node to be setup and working. Validating the config adds little value in those cases.
Its a minor change either way and if we see customer usage of debug/init not matching our expectations and it makes sense to add this back, we always can 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.
Done. Removed debug validation.
ebff073
to
80d737f
Compare
Issue #, if available: N/A
Description of changes:
SSM Activation parameter validation in nodeadm init command:
Added Regex validation for SSM Activation parameters based on SSM documentation - https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_CreateActivation.html#systemsmanager-CreateActivation-response-ActivationId
Testing (if applicable):
Logs:
Documentation added/planned (if applicable): N/A