Add first warning to validation #27445
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds some initial pattern to the validation logic to support passing up errors and warnings without massive plumbing changes, as well as adding the first warning message
howardjohn
added
the
release-notes-none
googlebot
added
the
cla: yes
istio-testing
added
the
size/L
shamsher31
reviewed
Sep 22, 2020
| // wrapper around multierror.Append that enforces the invariant that if all input errors are nil, the output | ||
| // error is nil (allowing validation without branching). | ||
| func appendValidation(v Validation, vs ...error) Validation { | ||
| appendError := func(err, err2 error) error { |
There was a problem hiding this comment.
nit: can we have a better naming? As it can accept both err and warning.
err, err2 doesn't look good.
shamsher31
reviewed
Sep 22, 2020
| err = appendError(err, err2) | ||
| switch t := err.(type) { | ||
| case Validation: | ||
| err = appendError(err, t.Err) |
There was a problem hiding this comment.
will this case also return t.Warning?
There was a problem hiding this comment.
No, if you use appendError it will drop the warning. The alternative is it will be broken if someone calls appendError on a Validation (its really easy to do this and hard to debug) or appendValidation cannot accept both types. We cannot make appendValidation accept error|Validation without making Validation an error as well, which impacts appendError
nmittler
reviewed
Sep 24, 2020
| @@ -2347,6 +2381,30 @@ func ValidateProtocol(protocolStr string) error { | |||
| return nil | |||
| } | |||
|
|
|||
| // wrapper around multierror.Append that enforces the invariant that if all input errors are nil, the output | |||
| // error is nil (allowing validation without branching). | |||
| func appendValidation(v Validation, vs ...error) Validation { | |||
There was a problem hiding this comment.
should this just be a member of Validation?
There was a problem hiding this comment.
I considered this but didn't do it because appendErrors doesn't and append builtin also does not
nmittler
approved these changes
Sep 24, 2020
gargnupur
added a commit
to gargnupur/istio
that referenced
this pull request
Oct 1, 2020
Include data plane debug (istio#27492) * Include data plane debug * Comments Change zap fields to WithLabels (istio#27477) * Change zap fields to WithLabels * Missing bracket * Go mod tidy Add include_eds to config_dump (istio#27495) 'istioctl pc' regression fix, plus unit tests (istio#27467) remote namespace in ClusterRoleBinding's metadata (istio#27496) ClusterRoleBinding is not namespace scoped. Use common warning for experimental commands (istio#27504) support specify --type for create-remote-secret command (istio#27430) * support specify --secret-name for create-remote-secret command * add release notes * refine helper message * fix error * fix help message * use --type instead of --secret-name * update release notes Fix alignment for Istio logo (istio#27314) * Fix alignment for Istio logo * resize logo and move title * move title Add first warning to validation (istio#27445) * Add first warning to validation This adds some initial pattern to the validation logic to support passing up errors and warnings without massive plumbing changes, as well as adding the first warning message * Fix error Automator: update proxy@master in istio/istio@master (istio#27512) add pod securitycontext in the k8s settings. (istio#26456) * add securitycontext for pod. * add releasenote. * update test case. bad feature labels only fail analyzer (istio#26890) * continue to read feats if one fails * imports Automator: update istio/api@master dependency in istio/istio@master (istio#27514) [Test Framework] Adding debug logs for KUBECONFIG var (istio#27517) TestRenderManifest should verify if the value is really set (istio#27506) [Test Framework] Fix default for --istio.test.kube.config. (istio#27525) We were unnecessarily setting a default. We later on use the values from the environment as a default if this value is unset. fix permission issue for operator deleting istio. (istio#27502) test framework: default to single network (istio#27530) skip endpointslice tests for multicluster (istio#27493) * use fresh context in forwarder * make skips more visible and skip serverfirst mc * reduce calls per cluster * add nds debug logging * log all cluster vips * always read cluster vips * make multicluster controller respect endpointmode * revert debug changes - skip ep slice * revert ep slice change * revert context background * fix names Add new tests for gateway using XDS traffic flow simulation (istio#27482) * Initial simulation testing * initial * Add more tests * cleanup * cleanup * Add one more case * clean things up a bit * Add more tests * wip * Add Ingress support * Add ingress conflict * Fix race Replace FileAccessLog_LogFormat(Deprecated) and FileAccessLog_JsonFormat(Deprecated) with FileAccessLog_LogFormat (istio#27488) * replace Replace FileAccessLog_LogFormat(Deprecated) and FileAccessLog_JsonFormat(Deprecated) with FileAccessLog_LogFormat * fix the failing tests read GKE_CLUSTER_URL if set (istio#27551) Print cmd for istio/proxy update latest sha to istio/istio (istio#27554) Automator: update proxy@master in istio/istio@master (istio#27557) Limit concurrency and request rate (istio#27555) * Limit concurrency and request rate * Comments * Lint multicluster registries respect endpointmode (istio#27531) * multicluster registries respect endpointmode * give istio-reader endpointslice permissions * gen * use correct apigroup * re-skip Automator: update istio/api@master dependency in istio/istio@master (istio#27561) disable host header feedback on inbound stackdriver metrics (istio#27560) getSidecarScope cleanup (istio#27452) * getSidecarScope cleanup * address comment remove expliclity setting generate requestid (istio#27579) Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Automator: update common-files@master in istio/istio@master (istio#27584) Automator: update istio/api@master dependency in istio/istio@master (istio#27586) Automator: update proxy@master in istio/istio@master (istio#27587) default min TLS version and configure cipher suites (istio#27500) * default min tls version and cipher suites Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix ut Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * lint Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * order cipher suites Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix unit tests Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * change to positive predicate and remove unnecessary word Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix ut Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Automator: update istio/api@master dependency in istio/istio@master (istio#27592) Update istio logo for grafana istio-mesh-dashboard (istio#27573) * Update istio logo for grafana istio-mesh-dashboard * use latest in url * update grafana addon dashboard Add envoy ratelimiting test (istio#27217) * ratelimit test add root get policy add header Add Ratelimit test using Envoy Ratelimit Service Signed-off-by: gargnupur <gargnupur@google.com> Fix lint fix lint error Signed-off-by: gargnupur <gargnupur@google.com> Use envoyproxy docker file and configmap * Made it work for 1.8+ * Fix lint Signed-off-by: gargnupur <gargnupur@google.com> * Use echo app Signed-off-by: gargnupur <gargnupur@google.com> * Add workload selector Signed-off-by: gargnupur <gargnupur@google.com> * fix licensing note Signed-off-by: gargnupur <gargnupur@google.com> * Move within telemetry for now Signed-off-by: gargnupur <gargnupur@google.com> * Change timeout to 45s instead of 60s Signed-off-by: gargnupur <gargnupur@google.com> * Change test name Signed-off-by: gargnupur <gargnupur@google.com> * Add features tag Signed-off-by: gargnupur <gargnupur@google.com> * Add features tag Signed-off-by: gargnupur <gargnupur@google.com> * Try avoid flakiness Signed-off-by: gargnupur <gargnupur@google.com> Support for HTTP_ROUTE array insert in EnvoyFilter (istio#27084) * add support for INSERT_FIRST, INSERT_BEFORE, INSERT_AFTER array operations for HTTP_ROUTE objects in EnvoyFilter * add tests for array insert behavior for HTTP_ROUTE EnvoyFilters * release note for added EnvoyFilter features for HTTP_ROUTE istioctl: add command to manage logging levels for istiod (istio#25276) Currently there is `istioctl pc log` command for Envoy proxy. This changes adds similar functionalities for `istiod` through `controlz` API. Implements istio#24922 Fix race in ResyncEDS (istio#27589) Fixes errors like https://prow.istio.io/view/gs/istio-prow/logs/unit-tests_istio_postsubmit/3807 Automator: update common-files@master in istio/istio@master (istio#27590) multicluster dns debugging and fix (istio#27343) * dump ndsz debug info for failed tests * format * fail to make sure things work * get proxy id * revert test failure Add analyze output to bug-report (istio#27559) * Add analyze output to bug-report * logandprinf update Remove multicluster flag (istio#27593) This is handled by RequireSingleCluster. Adding this causes our k8s version tests to skip almost all versions Automator: update proxy@master in istio/istio@master (istio#27594) include integ build tag in go list for presubmit target (istio#27597) * presubmit all target includes build tag for go list * fix ordering Minor UX fixes to bug-report (istio#27607) * Minor UX fixes to bug-report * Add kubectl version Propagate IstioOperator CR name to reconciler (istio#27601) * Propagate IstioOperator CR name to reconciler * Fix name for saved state * Lint, fix tests * Name should not end with dash Build on s390x platform (istio#27319) Revert accidental merge (istio#27623) Fix ingress status update bug (istio#27523) (istio#27624) Don't stop processing ingress status updates just because a record is found that doesn't need to be updated, as there may be records later in the list that do need to be updated. Automator: update istio/api@master dependency in istio/istio@master (istio#27625) fix cluster vip resolution for multi-cluster (istio#27608) * debug loggging * track cluster vips and instanceByPort for publicly exported services * always init cluster vips map * updateContext copies old cluster vip map * cleanup extraneous changes Fix and test missing integ build tags (istio#27600) Update licenses for new mirror tool (istio#27626) This is expected to remove many licenses which where unintentionally added, and is regenerated following istio/tools#1216 change to the tool. These licenses were *not* used by Istio, and were unintentionally included. The tooling change has TOC approval already. Automator: update common-files@master in istio/istio@master (istio#27632) Minor wording changes for operator errors (istio#27631) * Minor wording changes for operator errors * Lint Delete endpointslice (istio#27612) Support TargetPort for DNS Service Entry (istio#27629) Aside from consistency/completeness, this is really useful for TLS origination. Rather than modifying apps (to switch port from 80 to 443, but stay plaintext, which is weird), we can transparently add tls origination like: ```yaml apiVersion: networking.istio.io/v1beta1 kind: ServiceEntry metadata: name: httpbin spec: hosts: - httpbin.org ports: - number: 80 name: http protocol: HTTP targetPort: 443 resolution: DNS --- apiVersion: networking.istio.io/v1beta1 kind: DestinationRule metadata: name: originate-tls spec: host: httpbin.org trafficPolicy: tls: mode: SIMPLE ``` Automator: update istio/api@master dependency in istio/istio@master (istio#27633) test loadbalancing across VMs in more than one cluster per-network (istio#27634) * set ISTIO_META_NETWORK on echo vm * deploy vms to all clusters * fix whitespace chars Revert "test loadbalancing across VMs in more than one cluster per-network (istio#27634)" (istio#27649) This reverts commit c36a602. Fix Egress Tests for multicluster topology
istio-testing
pushed a commit
that referenced
this pull request
Oct 2, 2020
Include data plane debug (#27492) * Include data plane debug * Comments Change zap fields to WithLabels (#27477) * Change zap fields to WithLabels * Missing bracket * Go mod tidy Add include_eds to config_dump (#27495) 'istioctl pc' regression fix, plus unit tests (#27467) remote namespace in ClusterRoleBinding's metadata (#27496) ClusterRoleBinding is not namespace scoped. Use common warning for experimental commands (#27504) support specify --type for create-remote-secret command (#27430) * support specify --secret-name for create-remote-secret command * add release notes * refine helper message * fix error * fix help message * use --type instead of --secret-name * update release notes Fix alignment for Istio logo (#27314) * Fix alignment for Istio logo * resize logo and move title * move title Add first warning to validation (#27445) * Add first warning to validation This adds some initial pattern to the validation logic to support passing up errors and warnings without massive plumbing changes, as well as adding the first warning message * Fix error Automator: update proxy@master in istio/istio@master (#27512) add pod securitycontext in the k8s settings. (#26456) * add securitycontext for pod. * add releasenote. * update test case. bad feature labels only fail analyzer (#26890) * continue to read feats if one fails * imports Automator: update istio/api@master dependency in istio/istio@master (#27514) [Test Framework] Adding debug logs for KUBECONFIG var (#27517) TestRenderManifest should verify if the value is really set (#27506) [Test Framework] Fix default for --istio.test.kube.config. (#27525) We were unnecessarily setting a default. We later on use the values from the environment as a default if this value is unset. fix permission issue for operator deleting istio. (#27502) test framework: default to single network (#27530) skip endpointslice tests for multicluster (#27493) * use fresh context in forwarder * make skips more visible and skip serverfirst mc * reduce calls per cluster * add nds debug logging * log all cluster vips * always read cluster vips * make multicluster controller respect endpointmode * revert debug changes - skip ep slice * revert ep slice change * revert context background * fix names Add new tests for gateway using XDS traffic flow simulation (#27482) * Initial simulation testing * initial * Add more tests * cleanup * cleanup * Add one more case * clean things up a bit * Add more tests * wip * Add Ingress support * Add ingress conflict * Fix race Replace FileAccessLog_LogFormat(Deprecated) and FileAccessLog_JsonFormat(Deprecated) with FileAccessLog_LogFormat (#27488) * replace Replace FileAccessLog_LogFormat(Deprecated) and FileAccessLog_JsonFormat(Deprecated) with FileAccessLog_LogFormat * fix the failing tests read GKE_CLUSTER_URL if set (#27551) Print cmd for istio/proxy update latest sha to istio/istio (#27554) Automator: update proxy@master in istio/istio@master (#27557) Limit concurrency and request rate (#27555) * Limit concurrency and request rate * Comments * Lint multicluster registries respect endpointmode (#27531) * multicluster registries respect endpointmode * give istio-reader endpointslice permissions * gen * use correct apigroup * re-skip Automator: update istio/api@master dependency in istio/istio@master (#27561) disable host header feedback on inbound stackdriver metrics (#27560) getSidecarScope cleanup (#27452) * getSidecarScope cleanup * address comment remove expliclity setting generate requestid (#27579) Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Automator: update common-files@master in istio/istio@master (#27584) Automator: update istio/api@master dependency in istio/istio@master (#27586) Automator: update proxy@master in istio/istio@master (#27587) default min TLS version and configure cipher suites (#27500) * default min tls version and cipher suites Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix ut Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * lint Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * add release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix release notes Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * order cipher suites Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix unit tests Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * change to positive predicate and remove unnecessary word Signed-off-by: Rama Chavali <rama.rao@salesforce.com> * fix ut Signed-off-by: Rama Chavali <rama.rao@salesforce.com> Automator: update istio/api@master dependency in istio/istio@master (#27592) Update istio logo for grafana istio-mesh-dashboard (#27573) * Update istio logo for grafana istio-mesh-dashboard * use latest in url * update grafana addon dashboard Add envoy ratelimiting test (#27217) * ratelimit test add root get policy add header Add Ratelimit test using Envoy Ratelimit Service Signed-off-by: gargnupur <gargnupur@google.com> Fix lint fix lint error Signed-off-by: gargnupur <gargnupur@google.com> Use envoyproxy docker file and configmap * Made it work for 1.8+ * Fix lint Signed-off-by: gargnupur <gargnupur@google.com> * Use echo app Signed-off-by: gargnupur <gargnupur@google.com> * Add workload selector Signed-off-by: gargnupur <gargnupur@google.com> * fix licensing note Signed-off-by: gargnupur <gargnupur@google.com> * Move within telemetry for now Signed-off-by: gargnupur <gargnupur@google.com> * Change timeout to 45s instead of 60s Signed-off-by: gargnupur <gargnupur@google.com> * Change test name Signed-off-by: gargnupur <gargnupur@google.com> * Add features tag Signed-off-by: gargnupur <gargnupur@google.com> * Add features tag Signed-off-by: gargnupur <gargnupur@google.com> * Try avoid flakiness Signed-off-by: gargnupur <gargnupur@google.com> Support for HTTP_ROUTE array insert in EnvoyFilter (#27084) * add support for INSERT_FIRST, INSERT_BEFORE, INSERT_AFTER array operations for HTTP_ROUTE objects in EnvoyFilter * add tests for array insert behavior for HTTP_ROUTE EnvoyFilters * release note for added EnvoyFilter features for HTTP_ROUTE istioctl: add command to manage logging levels for istiod (#25276) Currently there is `istioctl pc log` command for Envoy proxy. This changes adds similar functionalities for `istiod` through `controlz` API. Implements #24922 Fix race in ResyncEDS (#27589) Fixes errors like https://prow.istio.io/view/gs/istio-prow/logs/unit-tests_istio_postsubmit/3807 Automator: update common-files@master in istio/istio@master (#27590) multicluster dns debugging and fix (#27343) * dump ndsz debug info for failed tests * format * fail to make sure things work * get proxy id * revert test failure Add analyze output to bug-report (#27559) * Add analyze output to bug-report * logandprinf update Remove multicluster flag (#27593) This is handled by RequireSingleCluster. Adding this causes our k8s version tests to skip almost all versions Automator: update proxy@master in istio/istio@master (#27594) include integ build tag in go list for presubmit target (#27597) * presubmit all target includes build tag for go list * fix ordering Minor UX fixes to bug-report (#27607) * Minor UX fixes to bug-report * Add kubectl version Propagate IstioOperator CR name to reconciler (#27601) * Propagate IstioOperator CR name to reconciler * Fix name for saved state * Lint, fix tests * Name should not end with dash Build on s390x platform (#27319) Revert accidental merge (#27623) Fix ingress status update bug (#27523) (#27624) Don't stop processing ingress status updates just because a record is found that doesn't need to be updated, as there may be records later in the list that do need to be updated. Automator: update istio/api@master dependency in istio/istio@master (#27625) fix cluster vip resolution for multi-cluster (#27608) * debug loggging * track cluster vips and instanceByPort for publicly exported services * always init cluster vips map * updateContext copies old cluster vip map * cleanup extraneous changes Fix and test missing integ build tags (#27600) Update licenses for new mirror tool (#27626) This is expected to remove many licenses which where unintentionally added, and is regenerated following istio/tools#1216 change to the tool. These licenses were *not* used by Istio, and were unintentionally included. The tooling change has TOC approval already. Automator: update common-files@master in istio/istio@master (#27632) Minor wording changes for operator errors (#27631) * Minor wording changes for operator errors * Lint Delete endpointslice (#27612) Support TargetPort for DNS Service Entry (#27629) Aside from consistency/completeness, this is really useful for TLS origination. Rather than modifying apps (to switch port from 80 to 443, but stay plaintext, which is weird), we can transparently add tls origination like: ```yaml apiVersion: networking.istio.io/v1beta1 kind: ServiceEntry metadata: name: httpbin spec: hosts: - httpbin.org ports: - number: 80 name: http protocol: HTTP targetPort: 443 resolution: DNS --- apiVersion: networking.istio.io/v1beta1 kind: DestinationRule metadata: name: originate-tls spec: host: httpbin.org trafficPolicy: tls: mode: SIMPLE ``` Automator: update istio/api@master dependency in istio/istio@master (#27633) test loadbalancing across VMs in more than one cluster per-network (#27634) * set ISTIO_META_NETWORK on echo vm * deploy vms to all clusters * fix whitespace chars Revert "test loadbalancing across VMs in more than one cluster per-network (#27634)" (#27649) This reverts commit c36a602. Fix Egress Tests for multicluster topology
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds some initial pattern to the validation logic to support
passing up errors and warnings without massive plumbing changes, as well
as adding the first warning message
[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure
Pull Request Attributes
Please check any characteristics that apply to this pull request.
[ ] Does not have any changes that may affect Istio users.