Skip to content
This repository was archived by the owner on Jul 11, 2023. It is now read-only.

Conversation

@nshankar13
Copy link
Contributor

@nshankar13 nshankar13 commented Aug 31, 2022

Fix ingress backend SAN's, which were getting the trust domain appended to the provided SAN.
This adds an e2e test to catch that going forward.
This also switches the internal builders to use the principal (trust domain appended) vs the identity (no trust domain)

Description: Cherry pick commit 15e46da from PR #4914 to release-v1.2 branch to fix ingressBackend bug.

This will fix #5042, which is a bug in release-v1.2.

Testing done:

Affected area:

Functional Area
New Functionality [ ]
CI System [ ]
CLI Tool [ ]
Certificate Management [ ]
Control Plane [ ]
Demo [ ]
Documentation [ ]
Egress [ ]
Ingress [ ]
Install [ ]
Networking [ ]
Observability [ ]
Performance [ ]
SMI Policy [ ]
Security [ ]
Sidecar Injection [ ]
Tests [ ]
Upgrade [ ]
Other [ ]

Please answer the following questions with yes/no.

  1. Does this change contain code from or inspired by another project?

    • Did you notify the maintainers and provide attribution?
  2. Is this a breaking change?

  3. Has documentation corresponding to this change been updated in the osm-docs repo (if applicable)?

@SanyaKochhar
Copy link
Contributor

Could you format the commit/PR title/description specifying the commit that is being backported? Some examples:
openservicemesh/osm-docs#408
openservicemesh/osm-docs#151

keithmattix
keithmattix previously approved these changes Aug 31, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #5063 (1493cfa) into release-v1.2 (893ff87) will increase coverage by 0.01%.
The diff coverage is 93.93%.

❗ Current head 1493cfa differs from pull request most recent head cf37976. Consider uploading reports for the commit cf37976 to get more accurate results

@@               Coverage Diff                @@
##           release-v1.2    #5063      +/-   ##
================================================
+ Coverage         68.64%   68.65%   +0.01%     
================================================
  Files               220      220              
  Lines             15944    15941       -3     
================================================
  Hits              10944    10944              
+ Misses             4948     4945       -3     
  Partials             52       52              
Flag Coverage Δ
unittests 68.65% <93.93%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/identity/types.go 74.07% <0.00%> (-5.93%) ⬇️
pkg/catalog/catalog.go 91.30% <100.00%> (+1.30%) ⬆️
pkg/catalog/inbound_traffic_policies.go 94.90% <100.00%> (ø)
pkg/catalog/ingress.go 96.26% <100.00%> (-0.04%) ⬇️
pkg/envoy/lds/rbac.go 80.35% <100.00%> (-0.68%) ⬇️
pkg/envoy/rbac/policy.go 100.00% <100.00%> (ø)
pkg/envoy/rds/route/rbac.go 92.85% <100.00%> (-0.70%) ⬇️
pkg/trafficpolicy/trafficpolicy.go 96.15% <100.00%> (ø)
pkg/certificate/manager.go 77.51% <0.00%> (+0.80%) ⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jaellio
jaellio previously approved these changes Aug 31, 2022
@shalier
Copy link
Contributor

shalier commented Aug 31, 2022

@nshankar13 should be able to rebase with main to get rid of the G114 and revive lint errors

@nshankar13 nshankar13 changed the title Cherry-pick release-v1.2: Fix ingress backend broken SAN (#4914) [backport] cherry-pick commit 15e46da to release-v1.2 Aug 31, 2022
@nshankar13 nshankar13 dismissed stale reviews from jaellio and keithmattix via af95f3e August 31, 2022 19:10
@nshankar13 nshankar13 force-pushed the release-v1.2 branch 8 times, most recently from 2dbc3a9 to 8ccbe7f Compare August 31, 2022 21:49
@shashankram
Copy link
Member

@nshankar13 could you confirm if this is the root cause of the issue described in #5042? I can see how that issue could be related to the fix here.

steeling and others added 2 commits August 31, 2022 21:36
Fix ingress backend broken SAN (openservicemesh#4914)

Fix ingress backend SAN's, which were getting the trust domain appended to the provided SAN.
This adds an e2e test to catch that going forward.
This also switches the internal builders to use the principal (trust domain appended) vs the identity (no trust domain)

Signed-off-by: nshankar13 <[email protected]>
@nshankar13
Copy link
Contributor Author

nshankar13 commented Sep 1, 2022

@shashankram yes this is the PR that will fix the authenticated principal and trustdomain problem causing the issue in #5042. I believe this fix was made even before release-v1.2 but the commit was missed for cherry-picking.

@shashankram
Copy link
Member

@shashankram yes this is the PR that will fix the authenticated principal and trustdomain problem causing the issue in #5042. I believe this fix was made even before release-v1.2 but the commit was missed for cherry-picking.

Thanks for confirming. Could you reference issue #5042 in your PR description so that it's clear this backport is necessary to fix a bug in release-v1.2.

Copy link
Contributor

@keithmattix keithmattix left a comment

Choose a reason for hiding this comment

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

Approving pending the description change requested by Shashank

@nshankar13
Copy link
Contributor Author

@shashankram @keithmattix updated PR description.

@keithmattix keithmattix merged commit d2175d3 into openservicemesh:release-v1.2 Sep 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants