Webhook: validate certificateRefs when tls is set and mode is Terminate.

[gyohuangxin]

What type of PR is this?

/kind feature

What this PR does / why we need it:

When the TLSModeType is set to terminate, there is always the need for a valid certificate to terminate the TLS connection.
This PR validates the certificateRefs must be set and not empty when tls config is set and tls mode is terminate.

Which issue(s) this PR fixes:

Fixes #1441

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

Hi @gyohuangxin. Thanks for your PR.

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

[gyohuangxin]

@mlavacca Since you created the issue, please review this PR, thanks.

[mlavacca]

This validation should be put into the v1alpha2 package as well.

[gyohuangxin]

@mlavacca Thanks for your comments, please review again.

[mlavacca]

/ok-to-test

[mlavacca]

Apart from a tiny nit related to the comment, it looks good. Thank you!

[gyohuangxin]

@mlavacca Thank you for your comments!

[shaneutt]

Everything looks good, the only thing is in other PRs we're trying to consolidate copied code between v1alpha2 and v1beta1. I would ask that since it appears there are no differences, just keep the v1beta1 copy of the code and tests, and include that in the v1alpha2 validation for the time being.

[gyohuangxin]

@shaneutt Sounds good, could you give the example PR I can refer to?

[shaneutt]

Sure, #1412 is the most recent example.

[gyohuangxin]

@shaneutt I keep the v1beta1 validation code and tests, and include that in the v1alpha2 validation. Please review again, thanks.

[gyohuangxin]

A request not related to this PR, I'd like to join the kubernetes-sigs org to continuously contribute to this project in the future. Would you like to be my sponsors @mlavacca @shaneutt ?

[mlavacca]

A request not related to this PR, I'd like to join the kubernetes-sigs org to continuously contribute to this project in the future. Would you like to be my sponsors @mlavacca @shaneutt ?

I'd like to, but unfortunately, I'm not a reviewer. Hence, I cannot sponsor you, as stated on the community page.

[michaelbeaumont]

TLS is the TLS configuration for the Listener. This field is required if the Protocol field is “HTTPS” or “TLS”.

From reading the docs, i.e. the field comments, I would say we can validate that tls must be set when HTTPS is the protocol. Terminate is the default mode and when Terminate is set, certificateRefs must be set, right?

[gyohuangxin]

TLS is the TLS configuration for the Listener. This field is required if the Protocol field is “HTTPS” or “TLS”.

From reading the docs, i.e. the field comments, I would say we can validate that tls must be set when HTTPS is the protocol. Terminate is the default mode and when Terminate is set, certificateRefs must be set, right?

Correct, but the validation to validate that tls must be set when HTTPS/TLS is the protocol is not implemented yet. We can implement it quickly in this function ValidateTLSCertificateRefs or create another PR for another independent validation function. What do you think? @mlavacca @michaelbeaumont

[gyohuangxin]

A request not related to this PR, I'd like to join the kubernetes-sigs org to continuously contribute to this project in the future. Would you like to be my sponsors @mlavacca @shaneutt ?

I'd like to, but unfortunately, I'm not a reviewer. Hence, I cannot sponsor you, as stated on the community page.

It's OK, sorry for missing this, thanks anyway.

[mlavacca]

I think we should add this validation here, where we already check that TLS must not be set when the protocol is HTTP, TCP, or UDP. If you want to implement this check here, @gyohuangxin, I suggest updating the issue fixed by this PR by adding this check, or creating a new issue, since this looks like a new webhook feature.

[gyohuangxin]

@mlavacca Yes, it's a better place. I prefer create another issue to implement it, I will create one later.

[shaneutt]

/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gyohuangxin, mlavacca, shaneutt

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 [shaneutt]

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