images/kube-webhook-certgen/rootfs: add support for patching APIService objects

[invidian]

What this PR does / why we need it:

This PR adds ability to inject created CA bundle to APIService objects, in addition to the existing ValidatingWebhookConfiguration and MutatingWebhookConfiguration, which is helpful, e.g. when one wants to use extension API server without insecureSkipTLSVerify: true.

This mainly allows deploying APIService in a secure way without e.g. dependency on cert-manager, which in some environments might be undesirable.

Note, that right now this PR includes changes from PR #7630.

Do note that changes to the code includes some uncommon things, like wrappedError struct to 100% preserve existing behavior, I didn't know if it was actually required, but at least right now they should be easier to clean up.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

There is decent set of unit tests implemented and invidian/kube-webhook-certgen:controller-v1.0.0-61-g36229c197 Docker image has been used for testing https://github.com/newrelic/helm-charts/tree/gsanchez/metrics-adapter-chart.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

@invidian: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @invidian. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[rikatz]

/ok-to-test

[rikatz]

damn this is big.

Thanks @invidian I will put this on my review queue ASAP!

[invidian]

Thanks @invidian I will put this on my review queue ASAP!

Thanks @rikatz! I this is not super urgent or anything on my side, but it would be nice to not having it stale.

damn this is big.

Well, code required some creative work to keep it the same, add tests to existing functionality and make it extensible for new one. Hopefully another person adding features or fixing bugs in it will have a bit easier 😅

Also rebased after #7630 merge.

[rikatz]

nit: As you are unexporting the function (still wondering why!) you should change the comment as well

[invidian]

still wondering why

To reduce the surface of the package and to promote use of newly introduced and more flexible PatchObjects function. Though this is a breaking change on Go module level, so I can revert this change if you like.

you should change the comment as well

Probably, but then some of my linters yell, that comment is not capitalized, so I recently just started leaving those docs as they are.

Please suggest a desired change and I'll apply it, as I don't have a preference here.

[rikatz]

@invidian overall lgtm, cool to see this project been extended :)

Some comments:

• I have left two minor comments, one about if we want really to extend this to other clientsets (other than k8s), otherwise using I'm not sure about that interface
• One nit about unexported functions.

Also, I was discussing with sig-net to turn this into an independent project without success, so if you don't mind we can keep the development here. I would suggest to add you as an approver/reviewer of the code of this program (we can add you into OWNERS file after you get membership in k8s org).

One last thing, maybe we want in a future to add some e2e tests into this using github actions :D

Thanks

[invidian]

Thanks for a timely review @rikatz, highly appreciated. It would indeed be nice to plug this code into CI to ensure no regressions, if you plan to maintain it. However, I'll need to get familiar with existing setup for the project to adopt the standards also for this sub-directory.

Once we have that, we can perhaps add some e2e tests as well.

Also, I was discussing with sig-net to turn this into an independent project without success, so if you don't mind we can keep the development here. I would suggest to add you as an approver/reviewer of the code of this program (we can add you into OWNERS file after you get membership in k8s org).

I'm happy to apply for membership and become a reviewer here, I guess there shouldn't be too much work involved. Would you like to sponsor me in that?

[rikatz]

Thanks for a timely review @rikatz, highly appreciated. It would indeed be nice to plug this code into CI to ensure no regressions, if you plan to maintain it. However, I'll need to get familiar with existing setup for the project to adopt the standards also for this sub-directory.

Once we have that, we can perhaps add some e2e tests as well.

Also, I was discussing with sig-net to turn this into an independent project without success, so if you don't mind we can keep the development here. I would suggest to add you as an approver/reviewer of the code of this program (we can add you into OWNERS file after you get membership in k8s org).

I'm happy to apply for membership and become a reviewer here, I guess there shouldn't be too much work involved. Would you like to sponsor me in that?

Yes sure, I can sponsor you for that :)

[invidian]

I'm sorry @rikatz, but I think I won't be able to add CI or e2e tests for this code. I've been doing several attempts of trying to figure out how the existing CI setup works and I find it very hard to extend in a reasonable way. I think the following points are significant here:

1. I don't see how make test is triggered on CI, I assume this is done somehow via Prow.
2. All scripts and targets are closely tied to PKG variable, pointing to k8s.io/ingress-nginx by default, which because of 1. is hard to reasonably override or extend.
3. Existing setup for e2e tests is very complex. Here is the rough flow:

1. GitHub Actions calls 'make kind-e2e-test'.
2. 'make kind-e2e-test' ONLY calls 'test/e2e/run.sh'.
3. 'test/e2e/run.sh' runs:
  3a. Creates kind cluster.
  3b. Calls 'make clean-image'.
  3c. Calls 'make build':
    - Calls 'build/build.sh' via 'build/run-in-docker.sh':
      - Calls 'go build'
  3d. Calls 'make image':
    - Calls 'make clean-image again'
      - Calls 'docker build'.
  3e. Calls 'make image' on e2e-image:
    - Calls 'make e2e-test-binary' on root Makefile:
      - Calls 'ginkgo build' via 'build/run-in-docker.sh'.
      - Calls 'docker build'.
  3f. Calls 'make e2e-test'.
4. 'make e2e-test' ONLY calls 'build/run-e2e-suite.sh'.
5. 'build/run-e2e-suite.sh' creates a Pod with e2e image.
6. e2e image calls 'ginkgo' with some obscure /e2e.test binary.

Note, that this is only to entry the actual test logic written in Go.

I could just drop a completely independent file to .github/workflows directory with some simplified tests, but I don't know if this is something acceptable. Please let me know how we can move those things forward.

Regarding the membership in K8s org, I found another sponsor, so I will be applying for it today.

[rikatz]

Yeah, so about the CI we need to improve it. Components are really tied. What we CAN do right now is just verify if the image builds fine, and maybe create a kind cluster just to check if things gets patched. Anyway, we can look at this as a follow up.

Thanks for all the clarifications
/approve
/hold
Will just finish reviewing and merge this. We should probably want to promote the generated image for this one as well (and maybe announce in README.md the correct path, later!)

Thanks

[rikatz]

/lgtm
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invidian, rikatz

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:

• images/OWNERS [rikatz]

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

[invidian]

Thanks for merging!

Will just finish reviewing and merge this. We should probably want to promote the generated image for this one as well (and maybe announce in README.md the correct path, later!)

Yeah, having it "released" would be nice :)

Yeah, so about the CI we need to improve it. Components are really tied. What we CAN do right now is just verify if the image builds fine, and maybe create a kind cluster just to check if things gets patched. Anyway, we can look at this as a follow up.

Sure, I'll have a look into that.

[invidian]

Created #7717 for basic CI.