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

Rename Pinniped Proxy TLS values in chart #5192

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

castelblanque
Copy link
Collaborator

Signed-off-by: Rafa Castelblanque rcastelblanq@vmware.com

Description of the change

Renames a couple of chart values regarding TLS for Pinniped proxy.

pinnipedProxy.TLSSecret --> pinnipedProxy.tls.existingSecret
pinnipedProxy.CACert    --> pinnipedProxy.tls.caCertificate

Benefits

Kubeapps chart is in line with TLS parameters used by Bitnami.

Possible drawbacks

N/A

Applicable issues

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@netlify
Copy link

netlify bot commented Aug 10, 2022

Deploy Preview for kubeapps-dev canceled.

Name Link
🔨 Latest commit 056acee
🔍 Latest deploy log https://app.netlify.com/sites/kubeapps-dev/deploys/62f3c792cebc240008b749d6

Copy link
Contributor

@antgamdia antgamdia left a comment

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 1589 to 1601
## TLS settings for Pinniped Proxy
## @param pinnipedProxy.tls.existingSecret TLS secret with which to proxy requests
## @param pinnipedProxy.tls.caCertificate TLS CA cert config map which clients of pinniped proxy should use with TLS requests
## This config map must contain a ca.crt key with the CA cert content as the value.
##
TLSSecret: ""
## @param pinnipedProxy.CACert Specify the TLS CA cert config map which
## clients of pinniped proxy should use with tls requests. This config map
## must contain a ca.crt key with the CA cert content as the value.
##
CACert: ""
## @param pinnipedProxy.lifecycleHooks for the Pinniped Proxy container(s) to automate configuration before or after startup
tls:
## Optional TLS secret with which to proxy requests
existingSecret: ""
## TLS CA cert config map which clients of pinniped proxy should use with tls requests.
## This config map must contain a ca.crt key with the CA cert content as the value.
caCertificate: ""
## @param pinnipedProxy.lifecycleHooks For the Pinniped Proxy container(s) to automate configuration before or after startup
##
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this format? This way we avoid repeating the explanation, no?

...
  defaultPinnipedAPISuffix: pinniped.dev
  ## TLS settings for Pinniped Proxy
  ## ref: https://kubeapps.dev/docs/latest/howto/oidc/using-an-oidc-provider-with-pinniped/#running-the-pinniped-proxy-service-over-tls
  tls:
    ## @param pinnipedProxy.tls.existingSecret TLS secret with which to proxy requests
    ##
    existingSecret: ""
    ## @param pinnipedProxy.tls.caCertificate TLS CA cert config map which clients of pinniped proxy should use with TLS requests
    ## This config map must contain a ca.crt key with the CA cert content as the value.
    ##
    caCertificate: ""
  ## @param pinnipedProxy.lifecycleHooks For the Pinniped Proxy container(s) to automate configuration before or after startup
  ##
  lifecycleHooks: {}
...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, done. I thought of doing this way, but saw the blocks above in the values.yaml and all params where aggregated in the parent one. Didn't know it could be done in this way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recall the support for these non-aggregated parms was added after we built the values.yaml, but now that it's supported, we better use it. In fact, I've double-checked it running the readmenator manually because I wasn'ts sure haha

Signed-off-by: Rafa Castelblanque <rcastelblanq@vmware.com>
@castelblanque castelblanque merged commit 3c2c26e into main Aug 10, 2022
@castelblanque castelblanque deleted the 5191-pinniped-proxy-tls-params branch August 10, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename pinniped tls-related values to comply with standards
3 participants