search

1Password / connect-helm-charts

Official 1Password Helm Charts
https://developer.1password.com
MIT License
93 stars 74 forks source link

1Password Connect Improved Networking #107

Closed klaus385 closed 2 years ago

klaus385 commented 2 years ago

1Password Connect - (Improved Networking)

The changes I made here will allow for a multitude of things for users trying to interact with the 1Password Connect and Sync applications deployed.

Changes

Below is a list of changes with detailed descriptions of the changes I made and why I made them.

Documentation

  1. Updated values.yaml to have the individual comments to allow for an inline view
  2. Re-ordered values.yaml to have a flow that tries to follow a purpose for each section.

Syntax

  1. Removed trailing whitespace.
  2. Added some templated for the improvements section that will allow ingress definitions and load balancer definitions.

Explanation of Changes

  1. Updated the values.yaml to have an example to use the ingress.yaml template, additional functionality for the service.yaml to use loadBalancerIP which would satisfy this GitHub issue, and loadBalancerSourceRanges to further extend on the service configuration for Load Balancer type.
  2. Created the ingress.yaml template to allow for the ingress values to be deployed.
  3. Created the service.yaml changes to allow for what is mentioned above.
  4. Updated _helpers.tpl to allow for easy means of templating out loadBalancerIP and loadBalancerSourceRanges.

Future Improvements

This section is kinda a QA in regards to this helm chart. In which I had other changes in mind but decided to NOT at this time. This is related to how the service.yaml and connect-deployment.yaml is written.

Potential Future Changes Questions

  1. Within service.yaml the API and Sync service are defined. Was there a particular reason why these were or are NOT separate templates? I ask this question due to the nature of needing to make a change to service.yaml in which by doing so you don't affect just API or Sync you affect change essentially to both.
  2. Within connect-deployment.yaml similar question in regards to splitting these deployments out. Because, if let's say one wants to affect how API or Sync have deployed any changes to one effect the code which indirectly affects the other.
  3. For ingress.yaml leverage if/else to discern the port when .Values.connect.tls.enabled is true or false.
klaus385 commented 2 years ago

Just curious if there is anything else needed for this to get merged?

klaus385 commented 2 years ago

@edif2008 maybe when I made this PR I missed something. Anyway, we can get some traction on this?

klaus385 commented 2 years ago

Thought to bump this again and see what can be done to look into merging this into the helm chart?

zifeo commented 2 years ago

@florisvdg Is there anything blocking this to get reviewed/merged?

edif2008 commented 2 years ago

Hey folks. Thank you so much for your contribution in making the helm charts better. It's so cool to see this big enhancement made by the community. ❤️

Apologies for keeping this PR hanging for a while. I've brought this PR on my team's radar to review it as soon as possible.

volodymyrZotov commented 2 years ago

@klaus385 Hey. I looked at the PR. Looks very good! Thank you! 👍
I left several small comments, please take a look.

jpcoenen commented 2 years ago

Thanks for your contribution, @klaus385! 🙌