Install APIGroup once for multiple DNS providers

[devholic]

What this PR does / why we need it:

If we register multiple DNS providers while running the webhook server, it will cause an unexpected exit with WebService with duplicate root path detected error.

This issue happens because the root path of each DNS provider is equal since they share the group name.

To resolve this issue, this commit installs APIGroup once for multiple DNS providers by extracting apiGroupInfo variable and InstallAPIGroup call from solver (DNS provider) loop in ChallengeServer constructor.

Which issue this PR fixes

None

Special notes for your reviewer:

None

Release note:

Fix unexpected exit when multiple DNS providers are passed to `RunWebhookServer` 

/kind bug

[jetstack-bot]

Hi @devholic. Thanks for your PR.

I'm waiting for a jetstack 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.

[irbekrm]

Hi, thanks for opening the PR!

I am not very familiar with this part of the code- would you be able to add some extra information to the PR? Are you writing a custom webhook and what config caused the error?

If we register multiple DNS providers while running the webhook server, it will cause an unexpected exit with WebService with duplicate root path detected error.

I see that this error comes from go-restful and the go-restful function that throws the error seems to be called from kubernetes endpoints package.

It makes sense to not re-install the same GroupVersion multiple times, but I am wondering if whether there can ever be different GroupVersions for different solvers that are being looped over here.

If you could add some extra information as to what setup you had that resulted in the error that would be very helpful.

[devholic]

@irbekrm

Sorry for the lack of context sharing 🙇

As you guessed, I'm trying to build a custom webhook for OpenStack Designate.
Our team operates a Kubernetes cluster that can share context with multiple OpenStack clusters. So, I tried to run multiple solvers with different environment variables within a single binary.

For example:

• v1alpha1.mycompany.com/openstack-cluster-a
  • Create challenge record in OpenStack cluster-a (which can register record for zone a.com)
• v1alpha1.mycompany.com/openstack-cluster-b
  • Create challenge record in OpenStack cluster-b (which can register record for zone b.com)

It makes sense to not re-install the same GroupVersion multiple times, but I am wondering if whether there can ever be different GroupVersions for different solvers that are being looped over here.

It seems RunWebhookServer, which is an entrypoint for building custom webhook, takes a single GroupName, so I think there cannot be different GroupVersions for now.

[irbekrm]

Thanks for that extra context!

It seems RunWebhookServer, which is an entrypoint for building custom webhook, takes a single GroupName, so I think there cannot be different GroupVersions for now.

You're right about this 👍🏼 I've looked over the code again- so it looks like the only solver-specific action that is done in that for-loop is adding a challenge handler for the specific resource to the versioned resources storage map here and this bit should keep functioning in the same way with either one or more solvers, since we only create a new versioned resources storage map if it doesn't exist, else the existing one will be updated (here). I guess arguably we could take the creation of the map out of the for-loop, however that would again need changing if a new version is added, so this should be ok as is 👍🏼

I'm a little concerned that we don't have any tests for this functionality. Have you tested that your webhook works with this change?

/ok-to-test

[devholic]

I tested running multiple solvers with the current branch, but I agree with having a test for this functionality.

However, completedConfig.New() assumes that it is called in the cluster only, so I had to add a restConfig field to inject dummy rest config for a test.

[irbekrm]

Thank you very much for adding the test!

However, completedConfig.New() assumes that it is called in the cluster only, so I had to add a restConfig field to inject dummy rest config for a test.

I cannot see any potential issue with that as it doesn't change the behaviour of how the kube config is obtained when a new webhook is created using RunWebhookServer 👍🏼
Perhaps at some point we'll want to refactor this to allow callers to pass a kube config (i.e via a CLI flag like it's currently done for cert-manager's webhook component).

I've also verified that the new test throws an error as in this PR's description when ran against current master 👍🏼

Thanks for confirming that this works for your webhook too.

Adding a hold for a while to allow others to take a look, but this seems non-controversial to me.

/hold
/lgtm
/approve
/milestone v1.7

[munnerz]

Thanks for reviewing this @irbekrm and thanks @devholic for the change (and corresponding test case 🙌)

This looks good to me 🎉

/approve
/hold cancel

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devholic, irbekrm, munnerz

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:

• OWNERS [irbekrm,munnerz]

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