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

googleKms.rotationPeriod converted to int but it's expected to be a sting #36

Open
mrfelton opened this issue Feb 6, 2020 · 3 comments

Comments

@mrfelton
Copy link

mrfelton commented Feb 6, 2020

Describe the bug

Unable to successfully deploy when leveraging GoogleKms due to invalid value for rotationPeriod

If you leave rotationPeriod unset it complains that a nil value is not allowed. If you set it to a number like 30 it complains that int64 is incorrect datatype. If you set it to string "30" it still complains about int64 being incorrect datatype.

Versions used
Kamus (API images): 0.6.0.0
Kamus CLI: 0.3.0
Chart version: ~0.4.3
KMS provider: Google KMS
Kubernetes flavour and version: (e.g. OpenShift Origin 3.9) GKE

To Reproduce

  1. Create a chart that leverages google kms
  2. Attempt to deploy it
  3. Note errors relating to invalid value for rotationPeriod

Expected behavior
Should deploy without errors.

@mrfelton
Copy link
Author

mrfelton commented Feb 6, 2020

Also worth noting, that I was able to manually correct the issue by leveraging kustomize overlays to correct the invalid k8 config files that this chart creates:

eg:

kind: Kustomization
apiVersion: kustomize.config.k8s.io/v1beta1

resources:
- ./all.yaml

patches:
- overlays/kamus.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: kamus-controller
data:
  KeyManagement__GoogleKms__RotationPeriod: ""
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: kamus-encryptor
data:
  KeyManagement__GoogleKms__RotationPeriod: ""
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: kamus-decryptor
data:
  KeyManagement__GoogleKms__RotationPeriod: ""

@omerlh
Copy link
Contributor

omerlh commented Feb 10, 2020

Thanks for filing a bug! That looks like an issue in the chart. Happy to hear you find a workaround, if you'll have time to open a fix PR that will be great!

marinkovicpetar added a commit to marinkovicpetar/helm-charts that referenced this issue Nov 22, 2020
@marinkovicpetar
Copy link
Contributor

@omerlh I've created a PR instead of @mrfelton (my colleague 👍 ) to allow the installation of chart when this value is omitted, so default value of "" can be used

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

No branches or pull requests

3 participants