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.

Sign up for GitHub

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

Jump to bottom

fix(xds): backwards compatibility on access logs paths #7662

Merged
merged 1 commit into from
Sep 7, 2023
Merged

fix(xds): backwards compatibility on access logs paths #7662

merged 1 commit into from
Sep 7, 2023

Conversation

jakubdyszkiewicz
Copy link
Contributor

@jakubdyszkiewicz jakubdyszkiewicz commented Sep 6, 2023 •
edited
Loading

Checklist prior to review

The problem was else if metadata.Resource != nil {. In case of indirect lifecycle (Kubernetes), we don't have resource in the metadata therefore AccessLogSocketPath was always empty which results in

2023-09-05T23:47:22.049Z	ERROR	xds-server.dataplane-sync-watchdog	OnTick() failed	{"dataplaneKey": {"Mesh":"xxx","Name":"xxx"}, "error": "invalid resource \"kuma:metrics:hijacker\": invalid Cluster.LoadAssignment: embedded message failed validation | caused by: invalid ClusterLoadAssignment.Endpoints[0]: embedded message failed validation | caused by: invalid LocalityLbEndpoints.LbEndpoints[0]: embedded message failed validation | caused by: invalid LbEndpoint.Endpoint: embedded message failed validation | caused by: invalid Endpoint.Address: embedded message failed validation | caused by: invalid Address.SocketAddress: embedded message failed validation | caused by: invalid SocketAddress.Address: value length must be at least 1 runes", "errorVerbose": "invalid Cluster.LoadAssignment: embedded message failed validation | caused by: invalid ClusterLoadAssignment.Endpoints[0]: embedded message failed validation | caused by: invalid LocalityLbEndpoints.LbEndpoints[0]: embedded message failed validation | caused by: invalid LbEndpoint.Endpoint: embedded message failed validation | caused by: invalid Endpoint.Address: embedded message failed validation | caused by: invalid Address.SocketAddress: embedded message failed validation | caused by: invalid SocketAddress.Address: value length must be at least 1 runes

The problem goes away on redeploy of DPPs, but we should have a seamless upgrade.

This was not caught by E2E test, because our upgrade tests are somewhat basic. Here is the action plan how to fix it for the future #7663

The issue only affects 2.4 version.

  • Link to relevant issue as well as docs and UI issues --
  • This will not break child repos: it doesn't hardcode values (.e.g "kumahq" as a image registry) and it will work on Windows, system specific functions like syscall.Mkfifo have equivalent implementation on the other OS --
  • Tests (Unit test, E2E tests, manual test on universal and k8s) --
  • Do you need to update UPGRADE.md? --
  • Does it need to be backported according to the backporting policy? (this GH action will add "backport" label based on these file globs, if you want to prevent it from adding the "backport" label use no-backport-autolabel label) --
  • Do you need to explicitly set a > Changelog: entry here or add a ci/ label to run fewer/more tests?

@jakubdyszkiewicz jakubdyszkiewicz marked this pull request as ready for review September 6, 2023 15:40
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner September 6, 2023 15:40
@jakubdyszkiewicz jakubdyszkiewicz requested review from Automaat and bartsmykla and removed request for a team September 6, 2023 15:40
@jakubdyszkiewicz jakubdyszkiewicz changed the base branch from master to release-2.4 September 6, 2023 15:40
@jakubdyszkiewicz jakubdyszkiewicz changed the base branch from release-2.4 to master September 6, 2023 15:41
lukidzi
lukidzi approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@lukidzi lukidzi left a comment

Choose a reason for hiding this comment

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

Nice catch 👍

lobkovilya
lobkovilya approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge without fixing the nit comment, it's super minor

pkg/core/xds/metadata.go Show resolved Hide resolved
lahabana
lahabana approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

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

Great catch!

Feels like the api to xdsMetadata should be a bit safer for these kind of cases (Or at least have good error handling).

@jakubdyszkiewicz jakubdyszkiewicz merged commit 27ea0f0 into kumahq:master Sep 7, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023 •
edited
Loading

backporting to release-2.0 with action

backporting to release-2.2 with action
backporting to release-2.3 with action
backporting to release-2.4 with action

@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

backporting to release-2.1 with action

kumahq bot pushed a commit that referenced this pull request Sep 7, 2023
@jakubdyszkiewicz @kumahq
fix(xds): backwards compatibility on access logs paths (#7662)
6d75962 
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
This was referenced Sep 7, 2023
Closed
Closed
kumahq bot pushed a commit that referenced this pull request Sep 7, 2023
@jakubdyszkiewicz @kumahq
fix(xds): backwards compatibility on access logs paths (#7662)
a59e06f 
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
@kumahq kumahq bot mentioned this pull request Sep 7, 2023
Closed
kumahq bot pushed a commit that referenced this pull request Sep 7, 2023
@jakubdyszkiewicz @kumahq
fix(xds): backwards compatibility on access logs paths (#7662)
32eebcc 
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
kumahq bot pushed a commit that referenced this pull request Sep 7, 2023
@jakubdyszkiewicz @kumahq
fix(xds): backwards compatibility on access logs paths (#7662)
f332be1 
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
This was referenced Sep 7, 2023
Closed
Merged
kumahq bot pushed a commit that referenced this pull request Sep 7, 2023
@jakubdyszkiewicz @kumahq
fix(xds): backwards compatibility on access logs paths (#7662)
8d7513c 
Signed-off-by: Jakub Dyszkiewicz <[email protected]>
@jakubdyszkiewicz jakubdyszkiewicz deleted the fix/backwards-compat branch September 7, 2023 06:46
jakubdyszkiewicz added a commit that referenced this pull request Sep 7, 2023
@kumahq @jakubdyszkiewicz
fix(xds): backwards compatibility on access logs paths (backport of #…
0daca1a 
…7662) (#7694)

Signed-off-by: Jakub Dyszkiewicz <[email protected]>
Co-authored-by: Jakub Dyszkiewicz <[email protected]>
@BrewTestBot BrewTestBot mentioned this pull request Nov 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukidzi lukidzi lukidzi approved these changes

@lobkovilya lobkovilya lobkovilya approved these changes

@lahabana lahabana lahabana approved these changes

@Automaat Automaat Awaiting requested review from Automaat Automaat is a code owner automatically assigned from kumahq/kuma-maintainers

@bartsmykla bartsmykla Awaiting requested review from bartsmykla bartsmykla is a code owner automatically assigned from kumahq/kuma-maintainers

Assignees
No one assigned
Labels
backport
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jakubdyszkiewicz @lahabana @lobkovilya @lukidzi