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

fix: CE-1078 persist secret content changes between rollouts #698

Merged
merged 18 commits into from
Oct 17, 2024

Conversation

jon-funk
Copy link
Collaborator

@jon-funk jon-funk commented Oct 8, 2024

Description

Allows developers to change the service-specific secret values for testing, and have those values persist between helm installs / re-deploys. Secrets are cleaned up on PR close still.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Tested PR close trigger, verified secrets are cleaned up with other workloads on close
  • Verified that secrets now have the helm.sh/resource-policy: keep annotation, and persist between deploys

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Further comments


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:


Thanks for the PR!

Deployments, as required, will be available below:

Please create PRs in draft mode. Mark as ready to enable:

After merge, new images are deployed in:

@jon-funk jon-funk added bug Something isn't working invalid This doesn't seem right labels Oct 8, 2024
@jon-funk jon-funk marked this pull request as ready for review October 8, 2024 20:05
@jon-funk jon-funk changed the base branch from main to release/lions-mane-jellyfish October 8, 2024 20:14
@jon-funk
Copy link
Collaborator Author

jon-funk commented Oct 8, 2024

Closing to test cleanup

@jon-funk jon-funk closed this Oct 8, 2024
@jon-funk jon-funk reopened this Oct 11, 2024
Copy link

Please retry analysis of this Pull-Request directly on SonarCloud

@@ -43,5 +43,6 @@ Selector labels
{{- define "selectorLabels" -}}
app.kubernetes.io/name: {{ include "fullname" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
app: {{ .Release.Name }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this new label?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's so that this label variant of the cleanup action can be re-used here

As the helm pr-close here https://github.com/bcgov/quickstart-openshift-helpers/blob/main/.github/workflows/.pr-close.yml#L161 does not allow flag passthrough. Specificaly in this case global.secrets.persist=false as the dev team would like secret workloads to persist in their dev environments.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changes to their secret workloads to persist between deployments*

Copy link
Collaborator

@mishraomp mishraomp Oct 11, 2024

Choose a reason for hiding this comment

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

lets discuss this, as clean up with label will make helm to stick around with secret objects, @DerekRoberts and I can look at the helper function to add in necessary flags as needed or we can add you as a collaborator on those repos :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am sorry.
I misunderstood the change, I see now, that you are cleaning up with helm then with label to make sure the persisted secret is deleted with labels, as helm wont delete them during uninstall

I will keep that into our helper as a backlog item, so we don't do twice cleanups :)

Copy link
Member

Choose a reason for hiding this comment

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

@jon-funk I would recommend strongly against persisting DEV secrets, since they tend to be abused. Keep them GitHub Actions (or even, yuk, Vault), then populate during PR environment creation.

Copy link
Collaborator

@afwilcox afwilcox Oct 15, 2024

Choose a reason for hiding this comment

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

@DerekRoberts @mishraomp - Let me provide a little context as to why we want this change.

Our project has two repositories. This one, and the nr-compliance-enforcement-cm repository that has the case management APIs and data that live in the emerald cluster.

By default the PRs in this repository are pointing to a static version of dev, in our emerald namespace.

However in the event of PRs that require changes both here and for the other repository we will end up needing to overwrite that secret to point to the environment where the other PR is deployed (e.g. dev-4).

The problem is... EVERY single commit on this repository (e.g. bringing PR up to date with the latest changes) will reset that overridden secret back to dev-static, which will usually result in integration tests failing.

As a result whenever changes are made to a PR under review one needs to

  • Commit Changes
  • Revert secret from default value back to overridden value
  • Ensure that initial helm deployment has finished
  • Nudge OpenShift pods so that proper secret is picked up
  • Restart integration tests if they were failing because earlier steps weren't done in time

Can you provide any suggestions for how we can streamline this that doesn't involve persisting secrets?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ent of PRs that require changes both here and for the other repository we will end up needing to overwrite that secret to point to the environment where the other PR is deployed (e.g. dev-4).

lets have a quick discussion on this one over teams, we can have the .deployer.yml in the repo itself and pass in github secrets, which would allow us github secrets as the source of truth , so we can keep a human generated secret in both repos, which can be fed to the pipeline and we don't need to persist secrets across prs or rollouts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jon-funk , lets update the logic here to look for pr specific secret objects, if they exist, read from them, if not read from default, https://github.com/bcgov/nr-compliance-enforcement/blob/main/charts/app/templates/secret.yaml#L5

Happy to have a chat on this over teams

Copy link
Member

Choose a reason for hiding this comment

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

@afwilcox @jon-funk How about reading secrets from the second repo and synching them over? It'd eliminate a lot of busy work. There are tons of alternatives too, like just plain copying the remote repo's config to GitHub secrets to easily (re-)population. @mishraomp's chat idea works too, since this isn't as easy as we usually see. ;)

@jon-funk
Copy link
Collaborator Author

Closing to test cleanup

@jon-funk jon-funk closed this Oct 11, 2024
@jon-funk jon-funk reopened this Oct 11, 2024
@jon-funk jon-funk removed the invalid This doesn't seem right label Oct 11, 2024
@jon-funk jon-funk added the pipeline change Change that updates the pipeline label Oct 16, 2024
Copy link
Collaborator

@nayr974 nayr974 left a comment

Choose a reason for hiding this comment

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

Looks fine so long as OM's concerns are considered

Copy link

@afwilcox afwilcox merged commit 9df80c5 into release/lions-mane-jellyfish Oct 17, 2024
15 checks passed
@afwilcox afwilcox deleted the CE-1078 branch October 17, 2024 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pipeline change Change that updates the pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants