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 some documentation about the webhook #2158

Merged
merged 17 commits into from
Dec 13, 2019

Conversation

barkbay
Copy link
Contributor

@barkbay barkbay commented Nov 22, 2019

Supersedes #2100

I did not keep the cert-manager recipe as I'm not sure about how to maintain the versions, also I'm not sure it adds some benefits to duplicate it from the documentation.

@barkbay barkbay added >docs Documentation v1.0.0 labels Nov 22, 2019
Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this doc! 👍 I think I like the cert-manager section best as it is clear and immediately useful for folks who want to run ECK next to cert-manager. I think the introduction is maybe a bit too technical. But maybe others can chime in and also help with wording and stylistic issues: I am thinking of @anyasabo or @charith-elastic and @alaudazzi

[id="{p}-webhook"]
== Validating webhook

A validating webhook provides additional validation of Elasticsearch resources beyond what can be specified in OpenAPI specs.
Copy link
Collaborator

@pebrc pebrc Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very technical and assumes a lot of knowledge about the structure and features of k8s CRDs

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, do you think it would be useful to just strike everything from "beyond" and on?

docs/index.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@alaudazzi alaudazzi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions, otherwise LGTM.

docs/operator-config.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
docs/webhook.asciidoc Outdated Show resolved Hide resolved
@anyasabo
Copy link
Contributor

Just two more nitpicks and a question about the webhook mgmt flag, but overall I am good with merging this in. Nice work!

@barkbay
Copy link
Contributor Author

barkbay commented Dec 12, 2019

Thanks everyone for your help and your review.
@anyasabo could I have a last review on the recent changes ? 🙏
Thank you

docs/webhook.asciidoc Outdated Show resolved Hide resolved
rollingUpdate:
partition: 0
type: RollingUpdate
EOF
Copy link
Contributor

@sebgl sebgl Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:(
I hate that we have to duplicate and maintain up to date the operator manifest here. But I don't see an easy way out. Could we still try to display a minimal subset of the operator manifest instead with only non-default fields?

Copy link
Contributor

@anyasabo anyasabo Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed on the duplication. When I first wrote this we did not have the main operator yaml configured for webhooks, so it seemed useful since there were a lot of changes necessary. Now we have most of them in the all-in-one though. So maybe we can just omit the sset entirely here and direct people to disable cert management in the operator? But keep the cert-manager resource examples

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes me realize that the sample is wrong here, we should include --manage-webhook-certs=false in the args of the container. I will remove the sset since it is clearly stated that "you first must ensure that the automatic certificate management feature is disabled"

docs/webhook.asciidoc Outdated Show resolved Hide resolved
Copy link
Contributor

@anyasabo anyasabo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding that! I'm good as is, but I am interested in Seb's suggestion about not duplicating the operator sset. It doesn't necessarily need to be in this PR if we do decide to do it though.

Edit: I saw your comment right after posting this. I'm good with removing the operator sset and merging this in then 🥇

@barkbay barkbay merged commit 2eaa89b into elastic:master Dec 13, 2019
@barkbay barkbay deleted the webhook-cert-manager branch December 13, 2019 09:06
mjmbischoff pushed a commit to mjmbischoff/cloud-on-k8s that referenced this pull request Jan 13, 2020
* Add some documentation about the webhook
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>docs Documentation v1.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants