-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat: New istio config format #1249
feat: New istio config format #1249
Conversation
Hi @pastequo. Thanks for your PR. I'm waiting for a knative-extensions member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
30722ac
to
22c45ce
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1249 +/- ##
==========================================
+ Coverage 81.54% 82.10% +0.56%
==========================================
Files 19 19
Lines 1669 1738 +69
==========================================
+ Hits 1361 1427 +66
- Misses 220 222 +2
- Partials 88 89 +1 ☔ View full report in Codecov by Sentry. |
4befabe
to
c29c707
Compare
/ok-to-test |
/retest |
// New format is partially defined | ||
if (!hasGateway && hasLocalGateway) || (hasGateway && !hasLocalGateway) { | ||
return nil, fmt.Errorf("config is not fully defined") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically just specifying external-gateways
is a valid config and we would default the local gateway to what's defined here:
net-istio/pkg/reconciler/ingress/config/istio.go
Lines 69 to 75 in c29c707
func defaultLocalGateways() []Gateway { | |
return []Gateway{{ | |
Namespace: system.Namespace(), | |
Name: KnativeLocalGateway, | |
ServiceURL: network.GetServiceHostname(KnativeLocalGateway, IstioNamespace), | |
}} | |
} |
Same applies to vice-versa where the user defines only the local-gateways
key
I think it would be simpler to just look at the presence of the new keys in NewIstioFromConfigMap
and if they're present assume it's the new config map format. Otherwise fallback onto the older parsing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I implemented the following
- If both format are defined (even partially), the code will error
- default values are used either by the new or old format
I let you resolve the thread if this new version is ok for you
c29c707
to
fc6e93f
Compare
/retest |
fc6e93f
to
5ead169
Compare
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dprotaso, pastequo 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 |
Changes
/kind enhancement
Fixes #1247
Release Note
Docs