-
Notifications
You must be signed in to change notification settings - Fork 724
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 operator flag to define webhook port #6691
Conversation
chore: update hostNetwork description to nicer description. Co-authored-by: Thibault Richard <thbkrkr@users.noreply.github.com>
deploy/eck-operator/values.yaml
Outdated
@@ -130,6 +130,8 @@ webhook: | |||
# This is required to allow for communication with the kube API when using some alternate CNIs in conjunction with webhook enabled. | |||
# CAUTION: Proceed at your own risk. This setting has security concerns such as allowing malicious users to access workloads running on the host. | |||
hostNetwork: false |
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.
Not from this PR but would it make sense to move hostNetwork
as a "top level" field?
hostNetwork
is part of the Pod
specification, as resources
, podSecurityContext
, serviceAccount
... and is not directly related to the webhook configuration.
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.
There is also a typo in the comment:
--- a/deploy/eck-operator/values.yaml
+++ b/deploy/eck-operator/values.yaml
@@ -126,7 +126,7 @@ webhook:
# objectSelector corresponds to the objectSelector property of the webhook.
# Setting this restricts the webhook to act only on objects that match the selector.
objectSelector: {}
- # HostNetwork allows a Pod to use the Node network namespace.
+ # hostNetwork allows a Pod to use the Node network namespace.
# This is required to allow for communication with the kube API when using some alternate CNIs in conjunction with webhook enabled.
# CAUTION: Proceed at your own risk. This setting has security concerns such as allowing malicious users to access workloads running on the host.
hostNetwork: false
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.
Not from this PR but would it make sense to move hostNetwork as a "top level" field?
hostNetwork is part of the Pod specification, as resources, podSecurityContext, serviceAccount... and is not directly related to the webhook configuration.
You're absolutely right, I missed that in #6636.
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.
I had it at the root, but was asked to change. Will move it to the root :)
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.
Thanks. Also need to update:
diff --git a/deploy/eck-operator/templates/statefulset.yaml b/deploy/eck-operator/templates/statefulset.yaml
index e17c34a82..16d66e5e2 100644
--- a/deploy/eck-operator/templates/statefulset.yaml
+++ b/deploy/eck-operator/templates/statefulset.yaml
@@ -117,7 +117,7 @@ spec:
{{- with .Values.volumes }}
{{- toYaml . | nindent 8 }}
{{- end }}
- {{- if .Values.webhook.hostNetwork }}
+ {{- if .Values.hostNetwork }}
This comment was marked as resolved.
This comment was marked as resolved.
I'm fine not supporting that in this PR, but still, how would you deploy ECK with |
IIRC there is a scheduler predicate that prevents 2 Pods with Edit: to be confirmed, just a vague recollection. |
By default I believe this is the case yes. EDIT: Its undoubtedly much more work but one way that would also improve the security of the operator would be to separate the webhook validation from the operator. |
Could you also add these new config parameters in the following documents, please? docs/operating-eck/operator-config.asciidoc |
Done :) |
Co-authored-by: Michael Morello <michael.morello@gmail.com>
Co-authored-by: Michael Morello <michael.morello@gmail.com>
buildkite test this |
@elasticmachine run elasticsearch-ci/docs |
buildkite test this |
@elasticmachine run elasticsearch-ci/docs |
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.
LGTM
As discussed in #6655, changes have been made to support nominating the port to listen.
Couple of notes: