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

Expose most_specific_header_mutations_wins #8692

Merged
merged 14 commits into from
Sep 21, 2023

Conversation

inFocus7
Copy link
Contributor

@inFocus7 inFocus7 commented Sep 15, 2023

Description

API/Code changes

  • Exposed the most_specific_header_mutation_wins in RouteConfigurations.
  • Add RouteTableOptions to TestContext's TestClients
  • Add tests for most_specific_header_mutations_wins

Docs changes

  • Add section in header manipulation docs bringing up this flag and its behavior

Context

Testing steps

./ci/kind/setup-kind.sh
helm install gloo _test/gloo-1.0.0-ci.tgz
kubectl apply -f - << 'EOF' 
apiVersion: v1
kind: ServiceAccount
metadata:
  name: httpbin
---
apiVersion: v1
kind: Service
metadata:
  name: httpbin
  labels:
    app: httpbin
spec:
  type: ClusterIP
  ports:
  - name: http
    port: 80
    targetPort: 80
  selector:
    app: httpbin
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: httpbin
spec:
  replicas: 1
  selector:
    matchLabels:
      app: httpbin
      version: v1
  template:
    metadata:
      labels:
        app: httpbin
        version: v1
    spec:
      serviceAccountName: httpbin
      containers:
      - image: docker.io/kennethreitz/httpbin
        imagePullPolicy: IfNotPresent
        name: httpbin
        ports:
        - containerPort: 80
EOF
kubectl apply -f - << 'EOF'
apiVersion: gateway.solo.io/v1
kind: VirtualService
metadata:
  name: default
  namespace: default
spec:
  virtualHost:
    domains:
    - '*'
    options:
      headerManipulation:
        responseHeadersToAdd:
        - header:
           key: X-Frame-Options
           value: VSORIGIN
          append: false
    routes:
    - delegateAction:
        selector:
          labels:
            delegate: 'true'
      matchers:
      - prefix: /
EOF
kubectl apply -f - << 'EOF'
apiVersion: gateway.solo.io/v1
kind: RouteTable
metadata:
  labels:
    delegate: 'true'
  name: child
  namespace: default
spec:
  routes:
  - matchers:
    - prefix: /get
    options:
      headerManipulation:
        responseHeadersToAdd:
        - header:
            key: X-Frame-Options
            value: RTORIGIN
          append: false
    routeAction:
      single:
        upstream:
          name: default-httpbin-80
          namespace: default
EOF
kubectl port-forward -n gloo-system deploy/gateway-proxy 8080:8080 & proxynormalPortFwdPid=$!
curl http://localhost:8080/get -v

Notice only the VS header (VSORIGIN) is in the header x-frame-options. This is because we have append: false on the header, therefore it is set to overwrite any headers that came before.

Edit the gateway resource to include spec.routeOptions.mostSpecificHeaderMutationsWins: true field.

curl http://localhost:8080/get -v

Notice that now, only the Route header (RTORIGIN) is in the header x-frame-options. This is because our route config is now set to prioritize the most specific header mutation.

Notes for reviewers

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@inFocus7 inFocus7 added the work in progress signals bulldozer to keep pr open (don't auto-merge) label Sep 15, 2023
@inFocus7 inFocus7 requested a review from a team as a code owner September 15, 2023 18:59
@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Sep 15, 2023
@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Visit the preview URL for this PR (updated for commit 263d9c2):

https://gloo-edge--pr8692-feat-expose-most-spe-ju7o68iu.web.app

(expires Wed, 27 Sep 2023 18:42:14 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 77c2b86e287749579b7ff9cadb81e099042ef677

@solo-changelog-bot
Copy link

Issues linked to changelog:
#8690

Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great use case for a set of e2e tests which show how configuration at the route/vs level bheavior differently based on this setting (ie when are heads actually appended vs overwritten). Since this functionality depends on multiple APIs in the data plane, unit tests alone won't be able to verify this.
The test could then be used as the source of truth for our docs.

@inFocus7 inFocus7 removed the work in progress signals bulldozer to keep pr open (don't auto-merge) label Sep 19, 2023
Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

projects/gloo/api/v1/options.proto Outdated Show resolved Hide resolved
test/e2e/headers_test.go Outdated Show resolved Hide resolved
test/e2e/headers_test.go Outdated Show resolved Hide resolved
test/services/gloo.go Show resolved Hide resolved
test/e2e/headers_test.go Show resolved Hide resolved
Co-authored-by: Art <artberger@users.noreply.github.com>
Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

projects/gloo/api/v1/options.proto Outdated Show resolved Hide resolved
projects/gloo/pkg/translator/route_config.go Outdated Show resolved Hide resolved
test/e2e/headers_test.go Show resolved Hide resolved
test/gomega/matchers/contain_headers.go Show resolved Hide resolved
Copy link
Contributor

@sam-heilbron sam-heilbron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I confirmed that the tests fail when I remove the translation code, and also when I adjust the value of this field

@soloio-bulldozer soloio-bulldozer bot merged commit 6de8d8f into main Sep 21, 2023
14 checks passed
@soloio-bulldozer soloio-bulldozer bot deleted the feat/expose-most-specific-header-mutations-wins branch September 21, 2023 14:34
inFocus7 added a commit that referenced this pull request Sep 21, 2023
* add most_specific_header_mutations_wins route config option

* translate and add MostSpecificHeaderMutationsWins to translator_test

* add changelog

* add skipCI-kube-tests:true directive

* add header manipulation order e2e tests and expose routeTable in testContext's testClients

* codegen

* Add section in docs to include mostSpecificHeaderMutationsWins

* remove skipCI-kube-tests:true directive

* review comment updates

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* review comment updates pt 2

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>
inFocus7 added a commit that referenced this pull request Sep 21, 2023
* add most_specific_header_mutations_wins route config option

* translate and add MostSpecificHeaderMutationsWins to translator_test

* add changelog

* add skipCI-kube-tests:true directive

* add header manipulation order e2e tests and expose routeTable in testContext's testClients

* codegen

* Add section in docs to include mostSpecificHeaderMutationsWins

* remove skipCI-kube-tests:true directive

* review comment updates

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* review comment updates pt 2

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>
soloio-bulldozer bot added a commit that referenced this pull request Sep 21, 2023
…on (#8704)

* Use appendAction for translated HeaderValueOptions (#8678)

* update header value options to convert append to envoy v3's append action

* update tests to expect appendAction

* remove focused test

* Update append description

* add changelog

* add extra informatnion on header append/removal docs

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md

Co-authored-by: Art <artberger@users.noreply.github.com>

* Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md

Co-authored-by: Art <artberger@users.noreply.github.com>

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>

* update changelog

* Expose most_specific_header_mutations_wins (#8692)

* add most_specific_header_mutations_wins route config option

* translate and add MostSpecificHeaderMutationsWins to translator_test

* add changelog

* add skipCI-kube-tests:true directive

* add header manipulation order e2e tests and expose routeTable in testContext's testClients

* codegen

* Add section in docs to include mostSpecificHeaderMutationsWins

* remove skipCI-kube-tests:true directive

* review comment updates

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* review comment updates pt 2

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>

* codegen

* do not skip kube tests

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>
soloio-bulldozer bot added a commit that referenced this pull request Sep 21, 2023
…on (#8705)

* Use appendAction for translated HeaderValueOptions (#8678)

* update header value options to convert append to envoy v3's append action

* update tests to expect appendAction

* remove focused test

* Update append description

* add changelog

* add extra informatnion on header append/removal docs

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md

Co-authored-by: Art <artberger@users.noreply.github.com>

* Update docs/content/guides/traffic_management/request_processing/append_remove_headers/_index.md

Co-authored-by: Art <artberger@users.noreply.github.com>

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>

* update changelog

* Expose most_specific_header_mutations_wins (#8692)

* add most_specific_header_mutations_wins route config option

* translate and add MostSpecificHeaderMutationsWins to translator_test

* add changelog

* add skipCI-kube-tests:true directive

* add header manipulation order e2e tests and expose routeTable in testContext's testClients

* codegen

* Add section in docs to include mostSpecificHeaderMutationsWins

* remove skipCI-kube-tests:true directive

* review comment updates

* Apply suggestions from code review

Co-authored-by: Art <artberger@users.noreply.github.com>

* review comment updates pt 2

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>

* codegen

* remove ContainHeaderKeys from header tests

* do not skip kube tests

* add matcher suite test

* add route table client to gateway

---------

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Art <artberger@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants