fix: istio destionationrule subsets enforcement #3126

dnguy078 · 2023-10-25T18:02:56Z

Checklist:
Relates to issue in #3125

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional with a list of types and scopes found here, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed my commits with DCO
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

Signed-off-by: Dennis Nguyen <[email protected]>

github-actions · 2023-10-25T18:45:40Z

Go Published Test Results

2 085 tests 2 085 ✔️ 2m 49s ⏱️
  118 suites       0 💤
      1 files       0 ❌

Results for commit 8402330.

♻️ This comment has been updated with latest results.

github-actions · 2023-10-25T19:05:12Z

E2E Tests Published Test Results

    4 files     4 suites 4h 0m 12s ⏱️
103 tests   82 ✔️   6 💤 15 ❌
454 runs 388 ✔️ 24 💤 42 ❌

For more details on these failures, see this check.

Results for commit 8402330.

♻️ This comment has been updated with latest results.

Signed-off-by: Dennis Nguyen <[email protected]>

codecov · 2023-10-25T21:31:03Z

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (423be7b) 81.85% compared to head (9af7c90) 81.86%.

❗ Current head 9af7c90 differs from pull request most recent head 8402330. Consider uploading reports for the commit 8402330 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3126      +/-   ##
==========================================
+ Coverage   81.85%   81.86%   +0.01%     
==========================================
  Files         134      134              
  Lines       20556    20568      +12     
==========================================
+ Hits        16826    16839      +13     
+ Misses       2866     2865       -1     
  Partials      864      864

Files	Coverage Δ
rollout/trafficrouting/istio/istio.go	`76.11% <75.00%> (+0.32%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nforcement Signed-off-by: Dennis Nguyen <[email protected]>

agrawroh

Looks good to me overall. @zachaller Could you please take a look as well?

@dnguy078 Could you please also add some tests for the failure cases?

agrawroh · 2023-11-01T03:53:49Z

rollout/trafficrouting/istio/istio.go

+			return nil, false, err
+		}
+
+		if host != "" {


Is it possible to have an empty host? We should check if Istio allows it.

looks like the host is required for Istio DestinationRule. We can do this check on the DestinationRule if included in .spec.strategy.canary.trafficRouting.istio.destinationRule

ah that check already exist :)

argo-rollouts/rollout/trafficrouting/istio/istio.go

Line 795 in 423be7b

if r.rollout.Spec.Strategy.Canary.TrafficRouting.Istio.DestinationRule != nil {

dnguy078 · 2023-11-01T04:09:05Z

Thank you for the review @agrawroh !

Signed-off-by: Dennis Nguyen <[email protected]>

sonarcloud · 2023-11-01T04:31:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
0.0% Duplication

* istio destionationrule subsets enforcement Signed-off-by: Dennis Nguyen <[email protected]> * add users Signed-off-by: Dennis Nguyen <[email protected]> * add test case for failure cases Signed-off-by: Dennis Nguyen <[email protected]> --------- Signed-off-by: Dennis Nguyen <[email protected]>

zachaller · 2023-11-01T14:17:59Z

@agrawroh thanks for the review, I also picked this back to v1.6.1 which I am releasing now

zachaller · 2023-11-01T23:04:52Z

@dnguy078 This might have caused this: https://cloud-native.slack.com/archives/C01U781DW2E/p1698875164861259 you mind taking a peak?

zachaller · 2023-11-02T00:31:23Z

Here is the issue: #3141

dnguy078 · 2023-11-02T00:51:11Z

I'll take a look @zachaller

This reverts commit 04e1119.

This reverts commit 04e1119. Signed-off-by: zachaller <[email protected]>

…#3147) Revert "fix: istio destionationrule subsets enforcement (#3126)" This reverts commit 04e1119. Signed-off-by: zachaller <[email protected]>

…rgoproj#3126)" (argoproj#3147)" This reverts commit 0593d92.

…rgoproj#3126)" (argoproj#3147)" This reverts commit 0593d92. Signed-off-by: Dennis Nguyen <[email protected]>

dnguy078 changed the title ~~istio destionationrule subsets enforcement~~ fix: istio destionationrule subsets enforcement Oct 25, 2023

istio destionationrule subsets enforcement

23706d6

Signed-off-by: Dennis Nguyen <[email protected]>

dnguy078 force-pushed the destinationrule-enforcement branch from c354708 to 23706d6 Compare October 25, 2023 18:14

Merge branch 'master' into destinationrule-enforcement

7d0e85c

add users

b4603bd

Signed-off-by: Dennis Nguyen <[email protected]>

dnguy078 mentioned this pull request Oct 27, 2023

feat: Adding support for defining max destination weights in Istio VS #1420

Closed

6 tasks

Merge remote-tracking branch 'upstream/master' into destinationrule-e…

9af7c90

…nforcement Signed-off-by: Dennis Nguyen <[email protected]>

agrawroh self-assigned this Oct 30, 2023

zachaller added the cherry-pick/release-1.6 label Oct 30, 2023

Merge branch 'master' into destinationrule-enforcement

0f572a8

agrawroh approved these changes Nov 1, 2023

View reviewed changes

add test case for failure cases

8402330

Signed-off-by: Dennis Nguyen <[email protected]>

zachaller approved these changes Nov 1, 2023

View reviewed changes

zachaller enabled auto-merge (squash) November 1, 2023 13:28

zachaller merged commit 04e1119 into argoproj:master Nov 1, 2023
22 checks passed

zachaller added the cherry-pick-completed Used once we have cherry picked the PR to all requested releases label Nov 1, 2023

dnguy078 deleted the destinationrule-enforcement branch November 1, 2023 16:11

zachaller added a commit that referenced this pull request Nov 2, 2023

Revert "fix: istio destionationrule subsets enforcement (#3126)"

57227ff

This reverts commit 04e1119.

zachaller mentioned this pull request Nov 2, 2023

Revert "fix: istio destionationrule subsets enforcement" #3146

Closed

zachaller added a commit to zachaller/argo-rollouts that referenced this pull request Nov 2, 2023

Revert "fix: istio destionationrule subsets enforcement (argoproj#3126)"

e8e29ed

This reverts commit 04e1119. Signed-off-by: zachaller <[email protected]>

dnguy078 added a commit to dnguy078/argo-rollouts that referenced this pull request Nov 28, 2023

Revert "fix: Revert "fix: istio destionationrule subsets enforcement (a…

0f8d9aa

…rgoproj#3126)" (argoproj#3147)" This reverts commit 0593d92.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: istio destionationrule subsets enforcement #3126

fix: istio destionationrule subsets enforcement #3126

dnguy078 commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

agrawroh left a comment •

edited

Loading

agrawroh Nov 1, 2023

dnguy078 Nov 1, 2023 •

edited

Loading

dnguy078 Nov 1, 2023

dnguy078 commented Nov 1, 2023

sonarcloud bot commented Nov 1, 2023

zachaller commented Nov 1, 2023

zachaller commented Nov 1, 2023

zachaller commented Nov 2, 2023

dnguy078 commented Nov 2, 2023

fix: istio destionationrule subsets enforcement #3126

fix: istio destionationrule subsets enforcement #3126

Conversation

dnguy078 commented Oct 25, 2023 • edited Loading

github-actions bot commented Oct 25, 2023 • edited Loading

Go Published Test Results

github-actions bot commented Oct 25, 2023 • edited Loading

E2E Tests Published Test Results

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

agrawroh left a comment • edited Loading

Choose a reason for hiding this comment

agrawroh Nov 1, 2023

Choose a reason for hiding this comment

dnguy078 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

dnguy078 Nov 1, 2023

Choose a reason for hiding this comment

dnguy078 commented Nov 1, 2023

sonarcloud bot commented Nov 1, 2023

zachaller commented Nov 1, 2023

zachaller commented Nov 1, 2023

zachaller commented Nov 2, 2023

dnguy078 commented Nov 2, 2023

dnguy078 commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

github-actions bot commented Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

agrawroh left a comment •

edited

Loading

dnguy078 Nov 1, 2023 •

edited

Loading