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

Do not add username labels to Identities and Users #92

Closed
wants to merge 1 commit into from

Conversation

alexeykazakov
Copy link
Contributor

Currently ksctl generate admin-manifests generates Identities with a username label. Which doesn't work for non DNS compliant usernames. For example usernames with +. Such usernames are allowed in OpenShift but can't be used in labels.

This PR still keeps the label for Service Accounts though. Something we should/can fix later.

Copy link
Contributor

@fbm3307 fbm3307 left a comment

Choose a reason for hiding this comment

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

Thanks for PR,
Looks Good,
Just curious, if the /todo changes of not using usernames in SA would be required immediately(meaning it can cause any failure ) or its good to have

Copy link
Contributor

@xcoulon xcoulon left a comment

Choose a reason for hiding this comment

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

The reason for having these labels is that we need to run a PostSync job after synchronizing the auth component in order to create the UserIdentityMapping resources to link the User and Identity resources together.
The job will look for Identity resources labeled with the username and create the UserIdentityMapping resources accordingly.

See https://github.com/kubesaw/ksctl/blob/master/cmd/user-identity-mapper/user_identity_mapper.go#L28

@MatousJobanek
Copy link
Contributor

closing this PR as since we need the label values and because the command should fail if the provided user name is not k8s compliant. See this PR: #94

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.86%. Comparing base (9ef0ea0) to head (1914d58).

Files with missing lines Patch % Lines
pkg/cmd/generate/cluster.go 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
+ Coverage   67.78%   67.86%   +0.07%     
==========================================
  Files          44       44              
  Lines        3216     3224       +8     
==========================================
+ Hits         2180     2188       +8     
  Misses        841      841              
  Partials      195      195              
Files with missing lines Coverage Δ
pkg/cmd/generate/permissions.go 77.84% <100.00%> (+0.91%) ⬆️
pkg/cmd/generate/cluster.go 77.50% <80.00%> (+0.57%) ⬆️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants