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

feat: unified approach for secrets referencing (and disabling) #189

Merged

Conversation

mgorsk1
Copy link
Contributor

@mgorsk1 mgorsk1 commented Oct 19, 2023

Describe your changes :

I worked on unifying the way secret values are pulled to OM deployment. Currently this chart works under assumption that every secret value (cert, password, etc.) is pulled from k8s secrets but that doesn't have to be the case. In some deployments (such as ours) values are populated using external secrets provider (like hashicorp vault) using init containers. Since we don't want to reference k8s secrets this refactor provides unified approach towards disabling referencing secrets - if secretRef is empty then env variable won't be included in the deployment.

I also made couple of variables to leverage tpl (like cluster name) as users might want to inject namespace or release details into them.

Type of change :

  • Bug fix
  • Improvement
  • New feature
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation

Frontend Preview (Screenshots) :

For frontend related change, please link screenshots of your changes preview! Optional for backend related changes.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have performed a self-review of my own.
  • I have tagged my reviewers below.
  • I have commented on my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed.

Reviewers

@shahsank3t @ayush-shah

@mgorsk1 mgorsk1 marked this pull request as ready for review October 19, 2023 13:46
@tutte
Copy link
Contributor

tutte commented Oct 19, 2023

Thanks for your contribution @mgorsk1.
Although technically LGTM and I'm sure this will cover your use case, I have to suggest to rely instead on envFrom. Specially if you are using ESO.
My point is this file is constantly growing and we are adding complexity with every new parameter.
ESO + independent files for each section is the cleanest approach.


As an example.

In helm chart:

envFrom:
 - secretRef:
    name: om-db-config
 - secretRef:
    name: om-saml-config

In secrets:

apiVersion: v1
kind: Secret
metadata:
  name: om-saml-config
type: Opaque
data:
  SAML_IDP_CERTIFICATE: c29tZXRoaW5nCg==
  SAML_SP_CERTIFICATE: c29tZXRoaW5nCg==

It gives more flexibility (as long as you manage the ESO definitions) and allows you to store the application configuration in Vault. You could have also something like

apiVersion: external-secrets.io/v1beta1
kind: ExternalSecret
metadata:
  name: om-app-config
  namespace: openmetadata
spec:
  data:
  - remoteRef:
      key: /openmetadata/app_config.json
    secretKey: data
  refreshInterval: 1h
  secretStoreRef:
    kind: SecretStore
    name: openmetadata-app-config
  target:
    creationPolicy: Owner
    deletionPolicy: Retain
    name: om-app-config
    template:
      engineVersion: v2
      mergePolicy: Replace
      templateFrom:
      - literal: '{{ .data }}'
        target: Data

and store the config as JSON directly in vault

@mgorsk1
Copy link
Contributor Author

mgorsk1 commented Oct 19, 2023

Hey @tutte thanks for prompt feedback! I agree that external secrets could be an option and indeed a very handy one. However, they don't come out of the box in k8s and deploying ESO requires more work on infra side. I would say it's quite invasive, especially that eso is in beta and might not meet all companies security policies. There are multiple ways to facilitate external secret providers in k8s (vault agent injector is an alternative approach) and I don't think forcing one way is 100% ok.

I agree it's several new lines here and there in _helpers but I think it actually reduces complexity by replacing several different approaches to secrets (in some cases you can disable populating envs from secrets using feature flags, in some not, etc.) with a single, unified one.

@mgorsk1 mgorsk1 force-pushed the feat/unify-collecting-from-secrets branch from 1e733a2 to 330269f Compare November 2, 2023 10:07
@akash-jain-10 akash-jain-10 merged commit b872778 into open-metadata:main Dec 5, 2023
1 check failed
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.

3 participants