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

ingress: Improve coverage with unit tests #24684

Merged
merged 7 commits into from Apr 3, 2023
Merged

Conversation

sayboras
Copy link
Member

@sayboras sayboras commented Apr 2, 2023

Description

This PR is to perform minor clean-up and to improve unit test coverage with test fixtures same as upstream Ingress Conformance tests. The goal is that we don't need to even run Conformance Test to debug any failure/discrepancy.

Testing

Just to make sure there is no flake in unit tests.

$ go test -count=10000 -cover ./operator/pkg/model/...
ok      github.com/cilium/cilium/operator/pkg/model     1.495s  coverage: 37.7% of statements
ok      github.com/cilium/cilium/operator/pkg/model/ingestion   8.095s  coverage: 85.1% of statements
ok      github.com/cilium/cilium/operator/pkg/model/translation 39.045s coverage: 73.6% of statements
ok      github.com/cilium/cilium/operator/pkg/model/translation/gateway-api     17.741s coverage: 84.2% of statements
ok      github.com/cilium/cilium/operator/pkg/model/translation/ingress 6.682s  coverage: 84.0% of statements

This attribute is deprecated and derived based on sum of individual weights.

```
2023-04-01T06:08:08.708391385Z level=warning msg="[Deprecated field: type envoy.config.route.v3.WeightedCluster Using deprecated option 'envoy.config.route.v3.WeightedCluster.total_weight' from file route_components.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override." subsys=envoy-misc threadID=79
```

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
There is validation of unique domain names in envoy v1.25.x, which causes
the below error in conformance test. This commit is to make sure that we
don't generate two virtual hosts with same domain names if enforce https
is enabled.

```
2023-04-01T06:08:08.710574289Z level=warning msg="NACK received for versions after  and up to 4; waiting for a version update before sending again" subsys=xds xdsAckedVersion= xdsClientNode="host~127.0.0.1~no-id~localdomain" xdsDetail="Only unique values for domains are permitted. Duplicate entry of domain foo.bar.com in route default/cilium-ingress-default-host-rules/listener-insecure" xdsNonce=4 xdsStreamID=9 xdsTypeURL=type.googleapis.com/envoy.config.route.v3.RouteConfiguration
```

Before

```json
  - '@type': type.googleapis.com/envoy.config.route.v3.RouteConfiguration
    name: listener-insecure
    virtualHosts:
    - domains:
      - foo.bar.com
      - foo.bar.com:*
      name: foo.bar.com
      routes:
      - match:
          safeRegex:
            regex: (/.*)?$
        redirect:
          httpsRedirect: true
    - domains:
      - '*.foo.com'
      - '*.foo.com:*'
      name: '*.foo.com'
      routes:
      - match:
          headers:
          - name: :authority
            stringMatch:
              safeRegex:
                regex: ^[^.]+[.]foo[.]com$
          safeRegex:
            regex: (/.*)?$
        route:
          cluster: default/wildcard-foo-com:8080
          maxStreamDuration:
            maxStreamDuration: 0s
    - domains:
      - foo.bar.com
      - foo.bar.com:*
      name: foo.bar.com
      routes:
      - match:
          safeRegex:
            regex: (/.*)?$
        route:
          maxStreamDuration:
            maxStreamDuration: 0s
          weightedClusters:
            clusters:
            - name: default/foo-bar-com:http
              weight: 1
            - name: default/foo-bar-com:http
              weight: 1
 ```

 After

 ```json
   - '@type': type.googleapis.com/envoy.config.route.v3.RouteConfiguration
     name: listener-insecure
     virtualHosts:
     - domains:
       - foo.bar.com
       - foo.bar.com:*
       name: foo.bar.com
       routes:
       - match:
           safeRegex:
             regex: (/.*)?$
         redirect:
           httpsRedirect: true
     - domains:
       - '*.foo.com'
       - '*.foo.com:*'
       name: '*.foo.com'
       routes:
       - match:
           headers:
           - name: :authority
             stringMatch:
               safeRegex:
                 regex: ^[^.]+[.]foo[.]com$
           safeRegex:
             regex: (/.*)?$
         route:
           cluster: default/wildcard-foo-com:8080
           maxStreamDuration:
             maxStreamDuration: 0s
 ```

Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
Signed-off-by: Tam Mach <tam.mach@cilium.io>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 2, 2023
@sayboras sayboras added area/CI Continuous Integration testing issue or flake release-note/misc This PR makes changes that have no direct user impact. release-note/ci This PR makes changes to the CI. area/servicemesh GH issues or PRs regarding servicemesh labels Apr 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 2, 2023
@sayboras sayboras marked this pull request as ready for review April 2, 2023 06:31
@sayboras sayboras requested a review from a team as a code owner April 2, 2023 06:31
@sayboras sayboras requested a review from youngnick April 2, 2023 06:31
@sayboras
Copy link
Member Author

sayboras commented Apr 2, 2023

/test

@sayboras sayboras merged commit 6f48ae8 into cilium:master Apr 3, 2023
44 checks passed
@sayboras sayboras deleted the tam/cleanup branch April 3, 2023 07:55
@youngnick
Copy link
Contributor

Beat me to it @mhofstetter, nice work both of you!

@sayboras sayboras added needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Sep 29, 2023
@julianwiedmann julianwiedmann added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. release-note/ci This PR makes changes to the CI. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants