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: add support for checksum annotations #415

Merged
merged 1 commit into from
Apr 20, 2022

Conversation

nritholtz
Copy link
Contributor

@nritholtz nritholtz commented Mar 15, 2022

Introduces two new boolean values secret.hashSumEnabled and configMap.hashSumEnabled, both true by default, which will auto create/managed checksum annotations on spec.template.metadata.annotations for relevant pod resources. This will automatically mark those pods to be recreated after the changes.

Related Issue or Design Document

#413

Checklist

  • I have read the contributing guidelines
    and signed the CLA.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added necessary documentation within the code base (if
    appropriate).

Further comments

I did a deep-dive into this while creating this PR, and I noticed an issue. The secrets in a few charts have auto-generated secret values by default. Since I am using include instead of lookup it means every time the annotations are rendered by the helm template, it would see it as a change/different value for every upgrade (and even a different value across different references in same chart, e.g. kratos and courier). Maybe there is an alternative approach, but I wondering whether this implementation is sufficient.

In addition, I needed to do some changes to helpers where .Values.....secrets are an empty dict to make the reference a safe access for null, since it was causing issues during my development.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2022

CLA assistant check
All committers have signed the CLA.

@bodymindarts
Copy link

Thank you for working on this. Just wanted to flag that I ran into exactly the same issue (config not causing a pod restart) and look forward to this being rolled out.

@Demonsthere
Copy link
Collaborator

Hi there, I see that Kratos is failing on the PR. This might be due to the last changes related to kratos 0.9, could you rebase to the current master so the CI can run again :)?

@nritholtz
Copy link
Contributor Author

OK, rebased to current master.

@Demonsthere
Copy link
Collaborator

Hmm, I see the CI failed again with error=map[message:dsn value requested an unknown driver] service_name=Ory Kratos service_version=v0.9.0-alpha.2, looks like the DSN value is somehow being corrupted here? Would need to run it locally and check what is happening with in the context of this changes

@nritholtz
Copy link
Contributor Author

@Demonsthere I believe I fixed the issue in the tests with my latest changes.

https://github.com/ory/k8s/pull/415/files#diff-feeebee2e39b1cef7f251ce97c0928f8c540e4e6e6ce556a4cf767054280abfeR108 is now using omit and deepCopy instead of unset, since unset was affecting the .Values.config object outside the scope of the helper function. This ultimately caused the .Values.config.dsn to be empty for the secret when being rendered.

Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

Great job, thanks! 🎉

@Demonsthere Demonsthere merged commit 7d5d297 into ory:master Apr 20, 2022
@nritholtz nritholtz deleted the issue_413 branch May 3, 2022 13:04
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.

None yet

4 participants