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

[deckhouse-controller] Fix auth data handling in change-registry helper #7095

Merged
merged 1 commit into from Jan 17, 2024

Conversation

mvasl
Copy link
Member

@mvasl mvasl commented Jan 10, 2024

Description

Fixed a bug where helper would not properly marshal provided credentials for registry into the secret. Unit tests of deckhouse-controller are now enabled as part of CI pipeline.

Why do we need it, and what problem does it solve?

This fixes a reported bug introduced with a go-containerregistry update.

Why do we need it in the patch release (if we do)?

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

Changelog entries

section: deckhouse-controller
type: fix
summary: fix for `change-registry` helper's handling of registry credentials.
impact_level: default

@mvasl mvasl added this to the v1.57.0 milestone Jan 10, 2024
@mvasl mvasl self-assigned this Jan 10, 2024
@github-actions github-actions bot added area/core Pull requests that update core modules go Pull requests that update Go code labels Jan 10, 2024
@mvasl mvasl force-pushed the fix/registry-change-auth-data branch 2 times, most recently from 4db6f9e to c5db697 Compare January 11, 2024 10:18
@mvasl mvasl added the e2e/run/yandex-cloud Run e2e tests in Yandex Cloud label Jan 11, 2024
@github-actions github-actions bot removed the e2e/run/yandex-cloud Run e2e tests in Yandex Cloud label Jan 11, 2024
@mvasl mvasl modified the milestones: v1.57.0, v1.56.5 Jan 11, 2024
@mvasl mvasl marked this pull request as ready for review January 11, 2024 12:34
Copy link
Member

@yalosev yalosev left a comment

Choose a reason for hiding this comment

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

Seems very risky, honestly. Because authn.AuthConfig has some attributes (at least custom json.Marshaller) which is used in a certain pieces of the Deckhouse.
I don't think that it's a good idea to replace it in a such way

@mvasl
Copy link
Member Author

mvasl commented Jan 15, 2024

Filed a bug to the go-containerregistry team about problems with their JSON marshalling implementation for authn.AuthConfig
google/go-containerregistry#1864

Until it's fixed we can use this PR as a workaround.

@yalosev cc

@mvasl mvasl requested a review from yalosev January 15, 2024 11:58
Signed-off-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
@mvasl mvasl force-pushed the fix/registry-change-auth-data branch from c5db697 to 276c99a Compare January 15, 2024 11:59
@mvasl mvasl added the e2e/run/yandex-cloud Run e2e tests in Yandex Cloud label Jan 15, 2024
@deckhouse-BOaTswain
Copy link
Collaborator

deckhouse-BOaTswain commented Jan 15, 2024

🟢 e2e: Yandex.Cloud for deckhouse:fix/registry-change-auth-data succeeded in 38m28s.

Workflow details

Yandex.Cloud-WithoutNAT-Containerd-1.25 - Connection string: ssh cloud-user@158.160.103.183

🟢 e2e: Yandex.Cloud, Containerd, Kubernetes 1.25 succeeded in 37m49s.

@github-actions github-actions bot removed the e2e/run/yandex-cloud Run e2e tests in Yandex Cloud label Jan 15, 2024
@deckhouse deckhouse deleted a comment from deckhouse-BOaTswain Jan 15, 2024
@name212 name212 merged commit 13e413a into main Jan 17, 2024
36 checks passed
@name212 name212 deleted the fix/registry-change-auth-data branch January 17, 2024 08:08
@mvasl mvasl removed this from the v1.57.0 milestone Jan 18, 2024
@mvasl mvasl added this to the v1.56.7 milestone Jan 18, 2024
@mvasl mvasl added the status/backport Backport pr label Jan 18, 2024
@deckhouse-BOaTswain
Copy link
Collaborator

Backport failed. See Job for details.

@mvasl mvasl removed the status/backport Backport pr label Jan 18, 2024
@mvasl mvasl modified the milestones: v1.56.7, v1.57.0 Jan 18, 2024
ghostinsoba pushed a commit that referenced this pull request Jan 31, 2024
…er (#7095)

Signed-off-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Co-authored-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Signed-off-by: Timur Kamaev <timur.kamaev@flant.com>
pashcovich pushed a commit that referenced this pull request Mar 6, 2024
…er (#7095)

Signed-off-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Co-authored-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Horiodino pushed a commit to Horiodino/deckhouse that referenced this pull request Apr 21, 2024
…er (deckhouse#7095)

Signed-off-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Co-authored-by: Maxim Vasilenko <maksim.vasilenko@flant.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core Pull requests that update core modules go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants