Introduce admission webhook settings page #590

flavio · 2018-06-19T14:46:51Z

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

To work this requires the following PR merged into the salt repo: SUSE/caasp-salt#555

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

The workflow is the following one:

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

Notable things missing:

Validate input (required)
Fix UI
Add tests
Fix coverage

vitoravelino · 2018-06-19T20:50:59Z

Current state with last commit:

vitoravelino · 2018-06-21T14:55:41Z

app/assets/javascripts/settings/index.js

@@ -11,7 +11,7 @@ $(function() {
    new RegistryForm($registryForm);
  }

-  if ($systemCertificateForm.length) {
-    new SystemCertificateForm($systemCertificateForm);


There's no SystemCertificateForm. I let this pass on a previous PR. Removed it since it was causing a js error.

vitoravelino · 2018-06-21T14:56:32Z

app/forms/admission_webhook_form.rb

@@ -0,0 +1,21 @@
+# AdmissionWebhookForm represents a form for AdmissionWebhookController
+class AdmissionWebhookForm


The idea is to simulate a virtual model and take advantage of some nice things of a model like validation.

This will be useful when we introduce the validation of the certificates itself.

I am wondering if parts of what this virtual model is doing should maybe be part of the certificate class, as that handles storage of uploaded certificates?

Then we might have another class to handle the keys - as not all views need both.

We could include Certificate. Would that be enough?

Sorry for the late reply: I wanted to try out the implementation in #593 first.

I guess yes - including certificate and putting the validation logic for certificates in the certificate.rb model would be enough, then we don't duplicate it.

As far as I can see the only thing that would be reused, and it would in the controller, is the Certificate.get_certificate_text method. Validation for this page has a condition and that differs from the Certificate model.

The reason for that is because the user is not forced to update both certificate and key at once. They can update one or the other or both. So I guess it's fine leaving this way, right?

We can reuse get_certificate_text after we merge #593.

Right now you are right - so let's leave it like this.

I assume later on the verification that the certificate is actually a certificate (X509 probably) will be part of certificate.rb, then it might be useful - but I guess we can do that once we work on that feature.

vitoravelino · 2018-06-21T17:34:30Z

@flavio This is good to be reviewed.

flavio · 2018-06-22T15:56:42Z

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 · 2018-06-22T16:00:56Z

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

vitoravelino · 2018-06-22T18:34:17Z

Fixed both cases, @flavio. Thanks!

vitoravelino · 2018-06-22T18:36:51Z

MaximilianMeister · 2018-06-25T12:08:37Z

app/views/settings/admission_webhook/index.html.slim

+h2 Admission Webhook
+
+p
+  | Enable Kubernetes ValidatingAdmissionWebhook admission controller. Once enabled


you need to add a space at $ or at the beginning of the next line, otherwise it reads Once enabledit is possible to...

MaximilianMeister · 2018-06-25T12:20:05Z

app/controllers/settings/admission_webhook_controller.rb

+    @key = Pillar.value(pillar: :api_admission_webhook_key)
+  end
+
+  def admission_webhook_params


can you still make sure to filter the cert/key from the logs in config/initializers/filter_parameter_logging.rb ?

I thought about that but for file inputs the content doesn't appear as for text inputs. I can paste the content here if you want.

If we still think that it's necessary to filter it, I can add it.

hmm in my case it appears, but it's the devenv. if it doesnt appear in production it's totally fine ;)

Oh. You mean the current_cert and current_key. Yeah, you are right. I thought that it was the key and certificate inputs. 😅

flavio · 2018-06-25T13:03:18Z

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

MaximilianMeister · 2018-06-26T08:06:25Z

app/controllers/setup_controller.rb

@@ -246,7 +246,7 @@ def proxy_enabled
  end

  def redirect_to_dashboard
-    redirect_to root_path if setup_done?
+    # redirect_to root_path if setup_done?


Yeah... Reverted. Thanks again! 😅

MaximilianMeister · 2018-06-26T11:05:19Z

app/controllers/settings/admission_webhook_controller.rb

+        notice: "Admission webhook settings successfully saved."
+    else
+      set_instance_variables
+      render action: :index, status: :unprocessable_entity


what about possible @errors? we should show them in an alert, would be a waste to throw them away

MaximilianMeister · 2018-06-26T11:20:40Z

app/forms/admission_webhook_form.rb

+  validates :key, presence: true, if: :current_key_blank?
+  validates :certificate, presence: true, if: :current_certificate_blank?
+
+  validate :cant_be_empty_file


should we at least validate the filetype as well?

def must_be_key `file #{path}`.include? "private key" end def must_be_cert `file #{path}`.include? "certificate" end

otherwise i'm afraid we'll have an uncaught exception

as discussed on irc we can address this in a follow up, as we have other cert handling as well, and it should be done for all parts where we upload certs

vitoravelino · 2018-06-28T11:39:34Z

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 · 2018-07-05T17:07:49Z

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?

bergmannf · 2018-07-16T11:40:52Z

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

vitoravelino · 2018-07-16T11:53:40Z

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?

bergmannf · 2018-07-16T11:55:39Z

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 · 2018-07-17T09:44:41Z

app/validators/pem_der_key_validator.rb

+require "openssl"
+
+# Verifies that an attribute is a valid PEM/DER encoded key
+class PemDerKeyValidator < ActiveModel::EachValidator


This validator seems to fail, when trying to upload a password protected key.

I think it is not supposed to be password-protected as a mutual TLS key, but maybe we should put this in the UI.

Allow user to enable AdmissionWebhook by using a new dedicated page. feature#ValidatingAdmissionWebhook Signed-off-by: Flavio Castelli <fcastelli@suse.com>

vitoravelino · 2018-08-09T13:33:11Z

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

bergmannf · 2018-08-09T13:42:37Z

spec/forms/admission_webhook_form_spec.rb

+describe AdmissionWebhookForm do
+  describe "validations" do
+    let(:cert_file) do
+      Rack::Test::UploadedFile.new(Rails.root.join("spec", "fixtures", "admin.crt"))


I think this code can just use fixture_file_upload instead of Rack::Test::UploadedFile.

bergmannf · 2018-08-09T13:44:10Z

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.

This is a follow-up from the previous commit improving the UI elements, adding more validation and adding tests for all the new components added with this new feature. feature#ValidatingAdmissionWebhook Signed-off-by: Vítor Avelino <vavelino@suse.com>

MalloZup · 2019-03-18T18:52:48Z

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

flavio requested a review from vitoravelino June 19, 2018 14:47

vitoravelino force-pushed the admission_webhook branch 2 times, most recently from c1b7c8d to b09e87f Compare June 21, 2018 14:52

vitoravelino reviewed Jun 21, 2018

View reviewed changes

vitoravelino force-pushed the admission_webhook branch from b09e87f to 3567e2b Compare June 21, 2018 14:59

vitoravelino changed the title ~~WIP - Introduce admission webhook settings page~~ Introduce admission webhook settings page Jun 21, 2018

vitoravelino force-pushed the admission_webhook branch 3 times, most recently from b752b14 to 1a365f8 Compare June 21, 2018 17:31

vitoravelino force-pushed the admission_webhook branch from 1a365f8 to fa43397 Compare June 21, 2018 17:49

vitoravelino force-pushed the admission_webhook branch 2 times, most recently from abd4fc6 to cd70225 Compare June 22, 2018 18:33

MaximilianMeister reviewed Jun 25, 2018

View reviewed changes

vitoravelino force-pushed the admission_webhook branch from cd70225 to 12f3f15 Compare June 25, 2018 12:43

MaximilianMeister mentioned this pull request Jun 25, 2018

Enable the ValidatingAdmissionWebhook SUSE/caasp-salt#555

Closed

vitoravelino force-pushed the admission_webhook branch from 12f3f15 to 26054b7 Compare June 25, 2018 13:58

MaximilianMeister reviewed Jun 26, 2018

View reviewed changes

vitoravelino force-pushed the admission_webhook branch from 26054b7 to dd87143 Compare June 26, 2018 09:22

vitoravelino mentioned this pull request Jun 26, 2018

Allow uploading certificates via file #593

Merged

MaximilianMeister reviewed Jun 26, 2018

View reviewed changes

vitoravelino force-pushed the admission_webhook branch from dd87143 to 550c421 Compare June 26, 2018 12:47

vitoravelino previously approved these changes Jun 28, 2018

View reviewed changes

vitoravelino mentioned this pull request Jul 2, 2018

Certificate validation #600

Merged

vitoravelino dismissed their stale review via 7bd68ca July 5, 2018 15:33

vitoravelino force-pushed the admission_webhook branch 2 times, most recently from 7bd68ca to 115add5 Compare July 5, 2018 17:04

vitoravelino force-pushed the admission_webhook branch from 115add5 to cd96dd8 Compare July 5, 2018 17:35

bergmannf force-pushed the admission_webhook branch from cd96dd8 to eb22226 Compare July 16, 2018 11:30

vitoravelino mentioned this pull request Jul 16, 2018

Add ability to add, modify, delete and list LDAP connections in the ui #607

Merged

bergmannf reviewed Jul 17, 2018

View reviewed changes

Introduce admission webhook settings page

7a77499

Allow user to enable AdmissionWebhook by using a new dedicated page. feature#ValidatingAdmissionWebhook Signed-off-by: Flavio Castelli <fcastelli@suse.com>

vitoravelino force-pushed the admission_webhook branch from eb22226 to c8066a1 Compare August 9, 2018 13:15

bergmannf reviewed Aug 9, 2018

View reviewed changes

vitoravelino force-pushed the admission_webhook branch 2 times, most recently from 6d55095 to cf73dd0 Compare August 9, 2018 14:36

vitoravelino force-pushed the admission_webhook branch from cf73dd0 to 5c93ade Compare August 9, 2018 17:42

ereslibre added the on hold label Nov 5, 2018

		@@ -0,0 +1,21 @@
		# AdmissionWebhookForm represents a form for AdmissionWebhookController
		class AdmissionWebhookForm

Introduce admission webhook settings page #590

Are you sure you want to change the base?

Introduce admission webhook settings page #590

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!