Skip to content
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

Merged
merged 4 commits into from
May 23, 2023
Merged

Conversation

pjuarezd
Copy link
Member

@pjuarezd pjuarezd commented May 9, 2023

What does this do?

  • Enables by default the admin policy for releases of KES v0.22.0 and newer
  • Fixes genetared config errors, that were preventing start of KES pods because of an invalid config.
  • Enhances KES Configuration UI in Operator Console with explanations of each config field
  • Fixes bug: UI fails to load values in form fields when the KES config in the secret has the kestore field, instead of keys.

How does it looks?

During the tenant creation Operator console page, Encryption is disabled by default

new-tenant-KES-1

When the switch is turned on, Encryption section shows as in the image below:

new-tenant-KES-2

  • A subtle, but substantial change is that now every field in the Encryption config page have a tooltip exaplaining what the field is about.

new-tenant-KES-3-tooltips

  • When apply, also the tooltip explains what is the expected value, event what will the default be if no value is specified

new-tenant-KES-5-tooltips-extended

  • Also an small enhancement is that now the Credentials section is enclosed in a groupbox, to differenciate it from config settings.

Screenshot 2023-05-18 at 11 24 06 PM

  • Tenant creation and tenant edition Encryption sections now also matches in look-and-feel

edit-tenant-KES-1

  • Added a warning message on the "Save" action: "The content of the KES config secret will be overwritten."

edit-tenant-KES-3-warning

KES 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.

  • For versions prior to v0.22.0 the previous configuration will keep being stored in the config secret as was before, we will call it KES Config version 1 for differentiation purposes, that configuration is as show below:

edit-tenant-KES-4-old-config

  • For KES versions v0.22.0 and newer, KES config will enable admin by default, change the section keys name to kestore and remove the policy section, we will call this config KES Config version 2. And looks as shown below:

edit-tenant-KES-2

  • The config variations are identified on base to the KES container images. On semantic versioning and datetime versioning, as follows:
    • Anything with date time versioning is consider KES Config version 2
    • date time container image name could include a "architecture" suffix, ie: minio/kes:2023-05-02T22-48-10Z-arm64 this include arm64 as version suffix.
    • KES v0.22.0 is considered KES Config version 2
    • Any semantic version prior to v0.22.0 is considered KES Config version 1

@pjuarezd pjuarezd changed the title [WIP] Add KES toolbox helps and v2 config Add KES toolbox helps and v2 config May 19, 2023
@pjuarezd pjuarezd changed the title Add KES toolbox helps and v2 config Add KES v2 config and toolbox helps May 19, 2023
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 {
Copy link
Contributor

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)

Copy link
Member Author

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)(-.*)`

Copy link
Contributor

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)
Copy link
Contributor

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.

Copy link
Member Author

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.

@cesnietor
Copy link
Contributor

@pjuarezd I found something while setting up encryption.
If I write something on the Encryption fields, the Create button is disabled until all fields are set but if I started setting up some fields and I change between encryption methods, the Create button gets enabled and allows us to create a Tenant. Which then causes an error while trying to initialize it. I think we probably need to refresh the button state every time we change encryption methods.

Screen.Recording.2023-05-22.at.10.24.14.AM.mov

Screenshot 2023-05-22 at 10 20 31 AM

@pjuarezd
Copy link
Member Author

@pjuarezd I found something while setting up encryption. If I write something on the Encryption fields, the Create button is disabled until all fields are set but if I started setting up some fields and I change between encryption methods, the Create button gets enabled and allows us to create a Tenant. Which then causes an error while trying to initialize it. I think we probably need to refresh the button state every time we change encryption methods.

Screen.Recording.2023-05-22.at.10.24.14.AM.mov
Screenshot 2023-05-22 at 10 20 31 AM

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.

@cesnietor
Copy link
Contributor

@pjuarezd I found something while setting up encryption. If I write something on the Encryption fields, the Create button is disabled until all fields are set but if I started setting up some fields and I change between encryption methods, the Create button gets enabled and allows us to create a Tenant. Which then causes an error while trying to initialize it. I think we probably need to refresh the button state every time we change encryption methods.
Screen.Recording.2023-05-22.at.10.24.14.AM.mov
Screenshot 2023-05-22 at 10 20 31 AM

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

@dvaldivia dvaldivia merged commit 5f0bbde into minio:master May 23, 2023
24 checks passed
@pjuarezd pjuarezd deleted the kes-config-update branch May 23, 2023 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants