velum icon indicating copy to clipboard operation
velum copied to clipboard

Published 20 hours ago • verified publisher icon SUSE

Introduce admission webhook settings page

Open flavio opened this issue 6 years ago • 15 comments

Allow user to enable AdmissionWebhook by using a new dedicated page.

To work this requires the following PR merged into the salt repo: https://github.com/kubic-project/salt/pull/555

The new UI has to be improved by @vitoravelino, right now it looks something like that:

image

The workflow is the following one:

  • go this new page
  • upload key and certificate
  • enable the feature
  • deploy the cluster

Notable things missing:

  • [X] Validate input (required)
  • [X] Fix UI
  • [X] Add tests
  • [x] Fix coverage

flavio avatar Jun 19 '18 14:06 flavio

Current state with last commit:

screenshot-20180619174743-431x277 screenshot-20180619174814-670x444 screenshot-20180619174851-673x755

vitoravelino avatar Jun 19 '18 20:06 vitoravelino

@flavio This is good to be reviewed.

vitoravelino avatar Jun 21 '18 17:06 vitoravelino

It looks to me, I like the workflow. However there's a bug:

  • upload key and cert
  • save
  • disable the feature
  • save -> accept the warning message from the popup window
  • reload the page
  • enable the feature -> the old key and cert are still shown

Basically we don't actually delete them from the database.

flavio avatar Jun 22 '18 15:06 flavio

Also another bug: it's possible to upload empty files. The validation in place doesn't look for that

flavio avatar Jun 22 '18 16:06 flavio

Fixed both cases, @flavio. Thanks!

vitoravelino avatar Jun 22 '18 18:06 vitoravelino

screenshot-20180622141838-295x229

vitoravelino avatar Jun 22 '18 18:06 vitoravelino

I cannot be the one approving the PR because I'm the one who created it. But it LGTM

flavio avatar Jun 25 '18 13:06 flavio

I guess we can proceed with the merge, right? I'll give my approval on behalf of @flavio since he created the PR and can't do that.

vitoravelino avatar Jun 28 '18 11:06 vitoravelino

I updated the code with with the x509 validation and added pem/der key format validation. Suggestion for wording are welcome. I also made a minor refactoring to make things more readable.

Please review it again. Thanks!

@flavio, could you play with this again?

vitoravelino avatar Jul 05 '18 17:07 vitoravelino

Just a short update to fix the merge conflicts. I still intend to setup an admission webhook and actually try out the code.

bergmannf avatar Jul 16 '18 11:07 bergmannf

Just a short update to fix the merge conflicts. I still intend to setup an admission webhook and actually try out the code.

I don't see any conflicts. Is something going to change?

vitoravelino avatar Jul 16 '18 11:07 vitoravelino

Sorry - I just meant I repushed the changes after a rebase due to conflicts in app/controllers/internal_api/v1/pillars_controller.rb - the dex connector apparently added some more pillars.

bergmannf avatar Jul 16 '18 11:07 bergmannf

I've rebased this and should be good to be reviewed again.

vitoravelino avatar Aug 09 '18 13:08 vitoravelino

LGTM with one small change (use fixture_file_upload).

However given that the salt PR is on hold I don't think we can merge this right now.

bergmannf avatar Aug 09 '18 13:08 bergmannf

This GitHub PR is unactive since 30. Is this GitHub PR still needed? Please close or update it accordingly. This reminder is autogenerated by https://github.com/MalloZup/blacktango

MalloZup avatar Mar 18 '19 18:03 MalloZup