-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add the ability to customise the details of the CA #17309
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.
I have added some initial comments inline along with an internal note about a change I believe is required for this to work. It would also be nice if new tests used must
rather than require, so we continue moving towards this library.
db6f1da
to
47b7fa2
Compare
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.
Thanks for the changes @lhaig; I left some comments inline, mostly stylistic ones and some areas we could reduce the amount of code.
I feel we could make some changes to the validation logic as well after testing this locally. I would expect to be able to modify only the organizational-unit
for example, however, when running this I get an error:
./pkg/darwin_arm64/nomad tls ca create --organizational-unit="nomad engineering"
please provide the -common-name flag when customizing the CA
I therefore think we need some type of merge function, which merges default values onto a potentially partially populated opts struct.
3ca5aa6
to
4c202ef
Compare
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.
Some inline comments. It would probably also be nice to expand on the command description, given that defaults don't take effect if you modify certain parameters.
Add changelog entry
Co-authored-by: James Rasell <[email protected]>
Added the ability to provide custom data to create CAs improved testing.
Update Tests
Apply suggestion by @jrasell to compose the function off the struct.
d8fb7c5
to
4bc9d81
Compare
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, thanks so much @lhaig!
Allow operators to customise the CA created using
nomad tls ca create