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

add TLS fix #278

Merged
merged 1 commit into from
Dec 2, 2024
Merged

add TLS fix #278

merged 1 commit into from
Dec 2, 2024

Conversation

wejdross
Copy link
Member

@wejdross wejdross commented Dec 2, 2024

Summary

  • Fixing TLS settings
  • For context: there is a difference if we initialize nested map inside some struct and single field. I had to add properties entry to generator to make it work.
  • Currently our XVSHNPostgreSQL receives new struct inside spec.parameters.service and has a form:
        repackEnabled: true
        serviceLevel: besteffort
 ++  tls:
 ++      enabled: true
        vacuumEnabled: false
  • test cases I did:
    ** new instance
    ** existing instance during migration from master branch
    ** existing instances with previous fixes and partially applied fields

@wejdross wejdross added bug Something isn't working minor labels Dec 2, 2024
@wejdross wejdross self-assigned this Dec 2, 2024
@zugao
Copy link
Collaborator

zugao commented Dec 2, 2024

I don't understand what is the problem.

@wejdross
Copy link
Member Author

wejdross commented Dec 2, 2024

Our Appcat's comp-function is reading field spec.parameters.service.tls.enabled and based on that it sets correct value for SGcluster's .spec.ssl.enabled. But this field must be present, otherwise it'll default to false inside comp function because of nil.

This PR ensures that after I deploy this change to clusters every xrd of XVSHNPostgreSQL would update it's spec and set new field .tls.enabled: true. Without this change after deployment of my previous PR, instances that were not created with tls settings in claim resulted in ssl.enabled: false because of nil value for TLS settings in xrd.

EDIT:
the most important thing is to initialize nested map: https://github.com/vshn/appcat/pull/278/files#diff-7fa21cdde94769399ac72b9c4e56d402f582b75d502f8b81769f276e529eb303R17

afterwards, I defaults settings of TLS to default={} and enabled is by default=true. This ensures that our PostgreSQL by default encrypts connections.

Copy link
Collaborator

@zugao zugao left a comment

Choose a reason for hiding this comment

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

LGTM

@zugao
Copy link
Collaborator

zugao commented Dec 2, 2024

Our Appcat's comp-function is reading field spec.parameters.service.tls.enabled and based on that it sets correct value for SGcluster's .spec.ssl.enabled. But this field must be present, otherwise it'll default to false inside comp function because of nil.

I guess if we had used disabled: false instead of enabled: true as default we would not have this problem now.

@zugao
Copy link
Collaborator

zugao commented Dec 2, 2024

Don't forget to squash your commits!

@wejdross wejdross merged commit bcfea90 into master Dec 2, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working minor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants