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

controlplane/authenticator: allow to use kubernetes.io/tls secrets #344

Merged
merged 7 commits into from
Feb 10, 2022
Merged

controlplane/authenticator: allow to use kubernetes.io/tls secrets #344

merged 7 commits into from
Feb 10, 2022

Conversation

roobre
Copy link
Contributor

@roobre roobre commented Jan 25, 2022

Fixes #342

paologallinaharbur
paologallinaharbur approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@paologallinaharbur paologallinaharbur left a comment

Choose a reason for hiding this comment

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

Some nits, and corner case to doublecheck, everything else looks good!

gsanchezgavier
gsanchezgavier approved these changes Feb 1, 2022
View reviewed changes
Copy link
Contributor

@gsanchezgavier gsanchezgavier left a comment

Choose a reason for hiding this comment

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

Looks good to me !
I totally miss this when refactored the CP.
The reason we are keeping the old names is to avoid the friction on the migration and save the renaming for existing users that have this configured ?

src/controlplane/client/authenticator/authenticator.go Outdated Show resolved Hide resolved
@roobre
Copy link
Contributor Author

roobre commented Feb 9, 2022 •
edited
Loading

The reason we are keeping the old names is to avoid the friction on the migration and save the renaming for existing users that have this configured ?

Yes, the idea is to avoid making a big breaking change if there is a clean way of providing backwards compat. Or, in the words of Helm maintainers, because we are nice people :P

gsanchezgavier
gsanchezgavier reviewed Feb 10, 2022
View reviewed changes
src/controlplane/client/authenticator/authenticator.go
Comment on lines +122 to +124
// Earlier versions of the integration allowed to define a secret of type corev1.SecretTypeOpaque, with the required
// certs and keys stored in keys named as the constants below.
// New versions can also consume a secret of type corev1.SecretTypeTLS, using standard names for certificate and key.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@gsanchezgavier
Copy link
Contributor

Yes, the idea is to avoid making a big breaking change if there is a clean way of providing backwards compat. Or, in the words of Helm maintainers, because we are nice people :P

Thanks for clarifying , make sense to me. And the PR LGTM thanks!

@roobre roobre merged commit 9b6fe22 into main Feb 10, 2022
@roobre roobre deleted the controlplane-tls-certs branch February 10, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsanchezgavier gsanchezgavier gsanchezgavier approved these changes

@paologallinaharbur paologallinaharbur paologallinaharbur approved these changes

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

Successfully merging this pull request may close these issues.

Control plane secret fetcher does not use type: kubernetes.io/tls secrets
3 participants
@roobre @gsanchezgavier @paologallinaharbur