Skip to content

Conversation

@tpiperatgod
Copy link
Contributor

@tpiperatgod tpiperatgod commented Jun 2, 2022

(If this PR fixes a github issue, please add Fixes #<xyz>.)

Fixes #319

(or if this PR is one task of a github issue, please add Master Issue: #<xyz> to link to the master issue.)

Master Issue: #

Motivation

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

  • add a subchart named "admission-webhook"
  • move CRDs to the "templates" dir
  • remove the "crds" dir
  • we currently use Cert Manager to manage certificates when communicating with the webhook

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Documentation

Check the box below.

Need to update docs?

@github-actions github-actions bot added the doc-required This pr needs a document label Jun 2, 2022
@tpiperatgod tpiperatgod changed the title [WIP] Add webhook to chart Add webhook to chart Jun 6, 2022
Copy link
Member

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

lgtm, but left some comments as questions, ptal, thanks.
also, could you please check if the changes affect function-mesh worker service https://github.com/streamnative/function-mesh-worker-service/blob/master/.ci/helm.sh#L118-L136, thanks.

@freeznet freeznet requested a review from jiangpengcheng June 6, 2022 08:22
@tpiperatgod tpiperatgod changed the title Add webhook to chart [WIP] Add webhook to chart Jun 10, 2022
@tpiperatgod tpiperatgod changed the title [WIP] Add webhook to chart Add webhook to chart Jun 10, 2022
@tpiperatgod
Copy link
Contributor Author

lgtm, but left some comments as questions, ptal, thanks. also, could you please check if the changes affect function-mesh worker service https://github.com/streamnative/function-mesh-worker-service/blob/master/.ci/helm.sh#L118-L136, thanks.

This PR does not change the usage when using cert-manager to manage webhook certificates.

I checked the ci workflows for the function-mesh and function-mesh-worker-service repositories and they both manage certificates through the cert-manager, so this PR does not affect either workflow.

@freeznet
Copy link
Member

freeznet commented Jun 10, 2022

lgtm, but left some comments as questions, ptal, thanks. also, could you please check if the changes affect function-mesh worker service https://github.com/streamnative/function-mesh-worker-service/blob/master/.ci/helm.sh#L118-L136, thanks.

This PR does not change the usage when using cert-manager to manage webhook certificates.

I checked the ci workflows for the function-mesh and function-mesh-worker-service repositories and they both manage certificates through the cert-manager, so this PR does not affect either workflow.

thanks @tpiperatgod, is it possible to enable webhook to the ci workflows after this PR merged?

@tpiperatgod
Copy link
Contributor Author

thanks @tpiperatgod, is it possible to enable webhook to the ci workflows after this PR merged?

Yes, it will be enabled by default.

But the current ci workflow Precommit - Integration Tests seems to have some problems. I'm not sure if this is due to insufficient resources for the github action. What do you think? @freeznet

Signed-off-by: laminar <tpiperatgod@gmail.com>
add the "go-get-tool" function to get the go library.

Signed-off-by: laminar <tpiperatgod@gmail.com>
- add a subchart named "admission-webhook"
- move CRDs to the "templates" dir
- remove the "crds" dir
- we currently use Cert Manager to manage certificates when communicating with the webhook

Signed-off-by: laminar <tpiperatgod@gmail.com>
- use "function-mesh-admission-webhook-service" as the name of the webhook service
- change the template name from "function-mesh-operator.service.webhook" to "function-mesh-operator.webhook.service"
- change the template name from "function-mesh-operator.certificate.secretName" to "function-mesh-operator.certificate.secret"

Signed-off-by: laminar <tpiperatgod@gmail.com>
- add the "function-mesh-secrets-webhook" chart, which contains two Secret resources:
  - CA Secret ( automatically generated by the Helm functions )
  - certificate Secret  ( automatically generated by the Helm functions )

Signed-off-by: laminar <tpiperatgod@gmail.com>
add `custom` certificates provider

Signed-off-by: laminar <tpiperatgod@gmail.com>
Signed-off-by: laminar <tpiperatgod@gmail.com>
use the fixed name `function-mesh-admission-webhook-server-cert` as the of Certificate and Secret

Signed-off-by: laminar <tpiperatgod@gmail.com>
@freeznet freeznet merged commit 0332a9d into streamnative:master Jun 13, 2022
@tpiperatgod tpiperatgod deleted the add_webhook_to_chart branch June 13, 2022 08:34
@Huanli-Meng Huanli-Meng added doc-added and removed doc-required This pr needs a document labels Jun 22, 2022
@Huanli-Meng
Copy link
Contributor

The doc is added through this PR: streamnative/function-mesh-website#134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

support deploy webhook with helm charts

4 participants