-
Notifications
You must be signed in to change notification settings - Fork 461
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 KES v2 config and toolbox helps #1602
Conversation
c448936
to
591df1c
Compare
535fcc3
to
6eb57fe
Compare
api/encryption-handlers.go
Outdated
releaseTagNoArch := imageTag | ||
fields := strings.Split(imageTag, "-") | ||
// here we will remove the arch suffix if present, ie: 2023-05-02T22-48-10Z-arm64 | ||
if len(fields) > 5 { |
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.
could you please remove this magic number and make it a constant? to know better what that means (it will self document)
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.
what about using a regex to identify if a tag matches the format with architecture in it? I think it is way more self describing that split by dashes
kesImageTagWithArchRegexPattern = `(\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z)(-.*)`
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.
That would be a much more robust implementation I agree!
@@ -435,8 +552,37 @@ func (suite *TenantTestSuite) TestTenantEncryptionInfoWithoutError() { | |||
suite.assert.Nil(err) |
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.
should we add here an assert of the expected response? cause otherwise we would just be testing that there were no errors. Similar in other related tests that don't return errors.
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.
you got it!, added assertion for expected errors and response model.
@pjuarezd I found something while setting up encryption. Screen.Recording.2023-05-22.at.10.24.14.AM.mov |
that is a neat finding!, but I think belongs in a new bug ticket, in the scope of this PR I am not changing the behavior of the form. |
Sounds good, I've created this issue #1610 |
What does this do?
admin
policy for releases of KESv0.22.0
and newerkestore
field, instead ofkeys
.How does it looks?
During the tenant creation Operator console page,
Encryption
is disabled by defaultWhen the switch is turned
on
,Encryption
section shows as in the image below:Encryption
config page have a tooltip exaplaining what the field is about.Credentials
section is enclosed in a groupbox, to differenciate it from config settings.Encryption
sections now also matches in look-and-feelKES Config updates
Starting KES v0.22.0 the
admin
section is required to be present, even if disabled, this was breaking fresh installs since the config generated from the Operator Console was not taking this into conscideration.KES Config version 1
for differentiation purposes, that configuration is as show below:admin
by default, change the sectionkeys
name tokestore
and remove thepolicy
section, we will call this configKES Config version 2
. And looks as shown below:KES Config version 2
minio/kes:2023-05-02T22-48-10Z-arm64
this includearm64
as version suffix.v0.22.0
is consideredKES Config version 2
v0.22.0
is consideredKES Config version 1