Skip to content
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

Add support for String Matcher in Header Matching #6226

Closed
costinm opened this issue Apr 26, 2023 · 9 comments · Fixed by #6313
Closed

Add support for String Matcher in Header Matching #6226

costinm opened this issue Apr 26, 2023 · 9 comments · Fixed by #6313
Assignees
Labels
Type: Feature New features or improvements in behavior

Comments

@costinm
Copy link

costinm commented Apr 26, 2023

core] [Channel #7] ccResolverWrapper: reporting error to cc: received route is invalid: route name:"istio-safe-stability.v1--testecho.0"  match:{prefix:"/"  case_sensitive:{value:true}  headers:{name:"x-istio-cluster"  string_match:{exact:"outbound|8080||v1--testecho.istio-safe-stability.svc.cluster.local"}}}  route:{cluster:"outbound|8080||v1--testecho.istio-safe-stability.svc.cluster.local"  cluster_not_found_response_code:INTERNAL_SERVER_ERROR  timeout:{}  retry_policy:{retry_on:"connect-failure,refused-stream,unavailable,cancelled,retriable-status-codes"  num_retries:{value:2}  retry_host_predicate:{name:"envoy.retry_host_predicates.previous_hosts"  typed_config:{type_url:"type.googleapis.com/envoy.extensions.retry.host.previous_hosts.v3.PreviousHostsPredicate"}}  host_selection_retry_max_attempts:5  retriable_status_codes:503}  max_stream_duration:{max_stream_duration:{}}}  metadata:{filter_metadata:{key:"istio"  value:{fields:{key:"config"  value:{string_value:"/apis/networking.istio.io/v1alpha3/namespaces/istio-safe-stability/virtual-service/v1--testecho-0-istio-autogenerated-k8s-gateway"}}}}}  decorator:{operation:"v1--testecho.istio-safe-stability.svc.cluster.local:8080/*"} has an unrecognized header matcher: &{StringMatch:exact:"outbound|8080||v1--testecho.istio-safe-stability.svc.cluster.local"}

This is produced by a standard HttpRoute -

  parentRefs:
  - group: gateway.networking.k8s.io
    kind: Gateway
    name: gateway
  - group: gateway.networking.k8s.io
    kind: Service
    name: echo
  rules:
  - backendRefs:
    - group: ""
      kind: Service
      name: v1--testecho
      port: 8080
      weight: 1
    matches:
    - headers:
      - name: x-istio-cluster
        type: Exact
        value: outbound|8080||v1--testecho.istio-safe-stability.svc.cluster.local
      path:
        type: PathPrefix
        value: /

NOTE: if you are reporting is a potential security vulnerability or a crash,
please follow our CVE process at
https://github.com/grpc/proposal/blob/master/P4-grpc-cve-process.md instead of
filing an issue here.

Please see the FAQ in our main README.md, then answer the questions below
before submitting your issue.

What version of gRPC are you using?

v1.56.0-dev ( tried on older versions too )

What version of Go are you using (go version)?

What operating system (Linux, Windows, …) and version?

What did you do?

If possible, provide a recipe for reproducing the error.

What did you expect to see?

What did you see instead?

@costinm
Copy link
Author

costinm commented Apr 26, 2023

This may impact the other languages - in K8S Gateway/GAMMA 'exact' is the only supported header matcher in core ( plus a regex with unspecified syntax ). It is pretty bad as it prevents the use of the new API ( at least in Istio - but I suspect in any other XDS implementation )

@easwars easwars self-assigned this May 2, 2023
@easwars
Copy link
Contributor

easwars commented May 2, 2023

We have had a couple of requests in the past to add support for StringMatch, and we ended up closing them saying this needs to be done across all languages. See #5917 and #5649.

We are having a cross-language discussion right now to see what it would take to add support for StringMatch and how to prioritize it compared to our other ongoing efforts. I'll update this issue once a decision is made. Thanks.

@easwars easwars added Type: Feature New features or improvements in behavior and removed Type: Bug labels May 2, 2023
@easwars
Copy link
Contributor

easwars commented May 3, 2023

We will add support for this soon. Please see grpc/proposal#359.

@costinm
Copy link
Author

costinm commented May 4, 2023 via email

@zasweq zasweq assigned zasweq and unassigned easwars May 9, 2023
@zasweq zasweq changed the title XDS with Istiod - rejected routes Add support for String Matcher in Header Matching May 16, 2023
@zasweq
Copy link
Contributor

zasweq commented May 16, 2023

The PR is still in flight. We can kick off once proposal finally merged.

@wulianglongrd
Copy link

Do we need to implement StringMatcher for RBAC?

func newHeaderMatcher(headerMatcherConfig *v3route_componentspb.HeaderMatcher) (*headerMatcher, error) {

@zasweq
Copy link
Contributor

zasweq commented Jun 13, 2023

Ah, yes we do. I spoke to the other languages and they have this for RBAC. Thanks for bringing this up. I'll file an issue.

@zasweq
Copy link
Contributor

zasweq commented Jun 13, 2023

Filed #6369.

@zasweq
Copy link
Contributor

zasweq commented Jun 27, 2023

Sent #6419 to add support for string matcher in RBAC header matching. Thanks for bringing this up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants