-
Notifications
You must be signed in to change notification settings - Fork 5
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
Calling bucket_spec_valid in a k8s validating webhook #5
base: main
Are you sure you want to change the base?
Conversation
b7745e4
to
21e102a
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.
A little explanation in the README is also missing.
README.md
Outdated
@@ -223,7 +223,7 @@ The operator is implemented in Python using the [Kopf](https://github.com/nolar/ | |||
To run it locally follow these steps: | |||
|
|||
1. Create and activate a local python virtualenv | |||
2. Install dependencies: `pip install -r requirements.txt` | |||
2. Install dependencies: `pip install -r requirements.txt`. During development the additional command `pip install 'kopf[dev]'` is required (but not for production, so it is not part of requirements.txt). |
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.
Did not test it yet, but if I read the kopf docs right, isn't dev
also needed if we want automatically-generated certs for the webhook in production?
Size is not a problem, the azure sdk is quite big already so a bit more doesn't really matter.
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.
Got it, including it in the requirements.txt
hybridcloud/operator.py
Outdated
@@ -33,6 +34,10 @@ def configure(settings: kopf.OperatorSettings, **_): | |||
settings.watching.connect_timeout = 60 | |||
settings.watching.client_timeout = 120 | |||
settings.networking.request_timeout = 120 | |||
settings.admission.managed = 'auto.kopf.dev' | |||
|
|||
if os.environ.get('ENVIRONMENT') is None: |
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.
The else branch to configure admission when not developing is missing.
Also for detecting if we are running in a cluster, the existence of the KUBERNETES_POD_NAME
envvar is a good test as we set that in the helm chart for the dpeloyment.
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.
As discussed, using kopf.WebhookAutoServer()
for now
hybridcloud/operator.py
Outdated
@@ -33,6 +34,10 @@ def configure(settings: kopf.OperatorSettings, **_): | |||
settings.watching.connect_timeout = 60 | |||
settings.watching.client_timeout = 120 | |||
settings.networking.request_timeout = 120 | |||
settings.admission.managed = 'auto.kopf.dev' |
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.
To show the validation is coming from the operator it would be better to use something like hybridcloud.maibornwolff.de
here.
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.
Got it 👍
f7e3da6
to
e4177d4
Compare
e4177d4
to
8f83598
Compare
This PR leverages the K8s native dynamic admission controllers to run the function
bucket_spec_valid
in a validating webhook. This causes invalid specs to actually fail whereas until now invalid specs would only cause an error in the status.In this example, the spec was invalid due to a name that was longer than 24 characters. The screenshot shows that the info generated by the
bucket_spec_valid
method is passed along to the user in the the form of a kopf.AdmissionErrorOnce this PR is approved, I will do the same thing for the other
hybrid-cloud
-operators