Add feature flag reconcileExternalGateway to control gateway reconciliation #4718
Conversation
There was a problem hiding this comment.
@ZhiminXiang: 0 warnings.
In response to this:
Fixes #
Proposed Changes
- Add feature flag reconcileExternalGateway to control gateway reconciliation
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.
|
/cc @Joon-L |
|
@ZhiminXiang: GitHub didn't allow me to request PR reviews from the following users: Joon-L. Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
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. |
| @@ -112,9 +119,11 @@ func NewIstioFromConfigMap(configMap *corev1.ConfigMap) (*Istio, error) { | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| reconcileGateway := strings.ToLower(configMap.Data[ReconcileExternalGatewayKey]) == "enabled" | |||
There was a problem hiding this comment.
Just go with strings.EqualFold
| @@ -270,7 +270,7 @@ func (r *BaseIngressReconciler) reconcileIngress(ctx context.Context, ra Reconci | |||
| ia.GetStatus().MarkLoadBalancerReady(lbs, publicLbs, privateLbs) | |||
| ia.GetStatus().ObservedGeneration = ia.GetGeneration() | |||
|
|
|||
| if enablesAutoTLS(ctx) { | |||
| if enableReconcileGateway(ctx) { | |||
There was a problem hiding this comment.
Would it be a future PR to have a way to make use of this flag without AutoTLS enabled? As far as I understand, even with this flag, Gateway will be reconciled using tls information on Route which will only get populated when the AutoTLS flag is set.
There was a problem hiding this comment.
Is it also correct to assume that there will be some step in between to reference an existing cert instead of calling the MakeCertificates() function in future PRs?
Also allow to choose mode of TLS as MakeServers() only allow simple TLS for now?
There was a problem hiding this comment.
Yes. This PR is just a first step of the manual TLS feature request. There will be a future PR in the Route controller to consume manual provisioned Certificates.
config/config-istio.yaml
Outdated
| # When auto TLS feature is turned on, reconcileExternalGateway will be automatically enforced. | ||
| # 1. Enabled: enabling reconciling external gateways. | ||
| # 2. Disabled: disabling reconciling external gateways. | ||
| reconcileExternalGateway: "Disabled" |
There was a problem hiding this comment.
Just in case: perhaps true/false toggle? Then the parsing can be delegated to strconv?
|
@vagababov I addressed your comments. PTAL. |
|
The following is the coverage report on pkg/.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mattmoor, vagababov, ZhiminXiang 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 |
This is a part of work related to #4631 about manual TLS configuration.
Proposed Changes