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

Allow configuration of an external webhook and its associated certs in the Porch API server #40

Merged
merged 5 commits into from
Jul 1, 2024

Conversation

Catalin-Stratulat-Ericsson
Copy link
Contributor

This change is related to nephio-project/nephio#554 and previous PR nephio-project/porch#53

This PR implements an optional porch package which can be used to allow web-hooks to be externally defined in yaml files, have cert manager automatically create and update the certificates which these web-hooks will utilize.

This optional package Requires cert manager to be installed in the system and as such to not make porch dependent on cert manager has been made optional.

Note: The version used for the porch-server image is set to v3.0.0 which does not exist yet as it requires the code written in the previously mentioned porch PR to function and as such v2.0.0 would not work.

@Catalin-Stratulat-Ericsson
Copy link
Contributor Author

Note: the only changes in this package compared to the default porch package are found in issuer-cert, validating-webhook and porch-server yaml files. all other files have stayed the same

@liamfallon
Copy link
Member

To aid reviewers:

% diff -qr nephio/core/porch  nephio/optional/porch-cert-manager-webhook
Only in nephio/optional/porch-cert-manager-webhook: 2-2-issuer-cert.yaml
Only in nephio/optional/porch-cert-manager-webhook: 2-3-validating-webhook.yaml
Files nephio/core/porch/3-porch-server.yaml and nephio/optional/porch-cert-manager-webhook/3-porch-server.yaml differ
Files nephio/core/porch/9-porch-controller-packagevariants-clusterrole.yaml and nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariants-clusterrole.yaml differ
Files nephio/core/porch/9-porch-controller-packagevariantsets-clusterrole.yaml and nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariantsets-clusterrole.yaml differ

% diff -w nephio/core/porch/3-porch-server.yaml nephio/optional/porch-cert-manager-webhook/3-porch-server.yaml

40c40,41
<           emptyDir: {}
---
>           secret:
>             secretName: porch-system-server-tls
46c47
<           image: docker.io/nephio/porch-server:v2.0.0
---
>           image: docker.io/nephio/porch-server:v3.0.0
68a70,71
>             - name: USE_CERT_MAN_FOR_WEBHOOK
>               value: "true"

% diff -w nephio/core/porch/9-porch-controller-packagevariants-clusterrole.yaml nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariants-clusterrole.yaml

34,41d33
<   - config.porch.kpt.dev
<   resources:
<   - repositories
<   verbs:
<   - get
<   - list
<   - watch
< - apiGroups:

%  diff -w nephio/core/porch/9-porch-controller-packagevariantsets-clusterrole.yaml nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariantsets-clusterrole.yaml

51,58d50
< - apiGroups:
<   - config.porch.kpt.dev
<   resources:
<   - repositories
<   verbs:
<   - get
<   - list
<   - watch

Copy link
Member

@liamfallon liamfallon left a comment

Choose a reason for hiding this comment

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

/approve

@nephio-prow nephio-prow bot added the approved label Jun 27, 2024
@Catalin-Stratulat-Ericsson
Copy link
Contributor Author

Catalin-Stratulat-Ericsson commented Jun 27, 2024

To aid reviewers:

% diff -qr nephio/core/porch  nephio/optional/porch-cert-manager-webhook
Only in nephio/optional/porch-cert-manager-webhook: 2-2-issuer-cert.yaml
Only in nephio/optional/porch-cert-manager-webhook: 2-3-validating-webhook.yaml
Files nephio/core/porch/3-porch-server.yaml and nephio/optional/porch-cert-manager-webhook/3-porch-server.yaml differ
Files nephio/core/porch/9-porch-controller-packagevariants-clusterrole.yaml and nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariants-clusterrole.yaml differ
Files nephio/core/porch/9-porch-controller-packagevariantsets-clusterrole.yaml and nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariantsets-clusterrole.yaml differ

% diff -w nephio/core/porch/3-porch-server.yaml nephio/optional/porch-cert-manager-webhook/3-porch-server.yaml

40c40,41
<           emptyDir: {}
---
>           secret:
>             secretName: porch-system-server-tls
46c47
<           image: docker.io/nephio/porch-server:v2.0.0
---
>           image: docker.io/nephio/porch-server:v3.0.0
68a70,71
>             - name: USE_CERT_MAN_FOR_WEBHOOK
>               value: "true"

% diff -w nephio/core/porch/9-porch-controller-packagevariants-clusterrole.yaml nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariants-clusterrole.yaml

34,41d33
<   - config.porch.kpt.dev
<   resources:
<   - repositories
<   verbs:
<   - get
<   - list
<   - watch
< - apiGroups:

%  diff -w nephio/core/porch/9-porch-controller-packagevariantsets-clusterrole.yaml nephio/optional/porch-cert-manager-webhook/9-porch-controller-packagevariantsets-clusterrole.yaml

51,58d50
< - apiGroups:
<   - config.porch.kpt.dev
<   resources:
<   - repositories
<   verbs:
<   - get
<   - list
<   - watch

thanks for the diff for clarity. there should be no changes made to the cluster-roles for pkg variants and pkg variant sets it must have been due to myself making this copy of the nephio/core/porch pkg to use as a baseline for the nephio/optional/porch-cert-manager-webhook pkg before PR#37 was pulled in. this has been addressed in latest update.

Here is the redone git diff after the changes

diff -w nephio/core/porch  nephio/optional/porch-cert-manager-webhook/
Only in nephio/optional/porch-cert-manager-webhook/: 2-2-issuer-cert.yaml
Only in nephio/optional/porch-cert-manager-webhook/: 2-3-validating-webhook.yaml
diff -w nephio/core/porch/3-porch-server.yaml nephio/optional/porch-cert-manager-webhook/3-porch-server.yaml
40c40,41
<           emptyDir: {}
---
>           secret:
>             secretName: porch-system-server-tls
46c47
<           image: docker.io/nephio/porch-server:v2.0.0
---
>           image: docker.io/nephio/porch-server:v3.0.0
68a70,71
>             - name: USE_CERT_MAN_FOR_WEBHOOK
>               value: "true"

@liamfallon
Copy link
Member

Thanks @Catalin-Stratulat-Ericsson
/approve

@kushnaidu
Copy link
Contributor

/approve

@liamfallon
Copy link
Member

/approve
/lgtm

Copy link
Contributor

nephio-prow bot commented Jul 1, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Catalin-Stratulat-Ericsson, kushnaidu, liamfallon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nephio-prow nephio-prow bot merged commit 580d4cf into nephio-project:main Jul 1, 2024
2 checks passed
@efiacor efiacor deleted the porch-cert-manager-webhook branch July 10, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants