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.

Sign up for GitHub

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

Jump to bottom

Initial External AUTH Secret #359

Merged
merged 1 commit into from
Mar 20, 2023
Merged

Initial External AUTH Secret #359

merged 1 commit into from
Mar 20, 2023

Conversation

bmarick
Copy link
Contributor

@bmarick bmarick commented Feb 7, 2023 •
edited
Loading

This PR is intended to resolve #332.
Provides a way to disable the generation of the {{Release.name}}-st2-auth Secret.
This will allow users to manage this secret outside of from the HELM process.

Limitation -> The customer provided secret must have the same name and keys as what would normally be generated.

@pull-request-size pull-request-size bot added the size/M PR that changes 30-99 lines. Good size to review. label Feb 7, 2023
@bmarick
Copy link
Contributor Author

bmarick commented Feb 7, 2023

I have done some initial testing already, but will do a more detailed testing tomorrow.

@cognifloyd and @armab -> I would appreciate your input on the name of the property to disable the Auth secret from being generated. I have no preference on the name of the property.

cognifloyd
cognifloyd reviewed Feb 7, 2023
View reviewed changes
values.yaml Outdated Show resolved Hide resolved
@bmarick
Copy link
Contributor Author

bmarick commented Mar 15, 2023

FYI - I got pull off this work to do different tasks at my Job.
I am back and working to address the issues raised in this pr.
My hope is to have an update later today or tomorrow!

@bmarick
Copy link
Contributor Author

bmarick commented Mar 15, 2023

Please let me know what you think of the values file.
I would still like to add 1 test to validate the secret name in the deployment/secret get properly generated

cognifloyd
cognifloyd reviewed Mar 15, 2023
View reviewed changes
Chart.yaml Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
templates/deployments.yaml Outdated Show resolved Hide resolved
templates/secrets_st2auth.yaml Outdated Show resolved Hide resolved
@pull-request-size pull-request-size bot added size/L PR that changes 100-499 lines. Requires some effort to review. and removed size/M PR that changes 30-99 lines. Good size to review. labels Mar 15, 2023
@bmarick bmarick requested a review from cognifloyd March 15, 2023 19:06
cognifloyd
cognifloyd reviewed Mar 15, 2023
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

LGTM. Let me know when you're ready for the final review. :)

@bmarick
Copy link
Contributor Author

bmarick commented Mar 15, 2023

LGTM. Let me know when you're ready for the final review. :)

Will test today in my environment for real world testing. So request should come in later today

@bmarick bmarick marked this pull request as ready for review March 16, 2023 08:10
@bmarick
Copy link
Contributor Author

bmarick commented Mar 16, 2023 •
edited
Loading

I have done live testing in my lab environment and confirmed this is functional

Correction the jobs failed due to missing the st2-admin secret. checking that now

@bmarick
Copy link
Contributor Author

bmarick commented Mar 16, 2023

Okay I can confirm that the process fully works!

@bmarick bmarick force-pushed the externalAuthSecret branch 3 times, most recently from 9cfd9cf to 9239520 Compare March 16, 2023 08:51
@bmarick
Copy link
Contributor Author

bmarick commented Mar 16, 2023

The only things missing are tests for the following files.
Please let me know if these are required.

  • templates/jobs.yaml
  • templates/tests/st2tests-pod.yaml

@bmarick bmarick force-pushed the externalAuthSecret branch 2 times, most recently from db07c14 to a7f7bc9 Compare March 16, 2023 18:39
@bmarick
Copy link
Contributor Author

bmarick commented Mar 17, 2023

LGTM. Let me know when you're ready for the final review. :)

Ready for review!

cognifloyd
cognifloyd reviewed Mar 17, 2023
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

Nice! One nitpick to save my sanity in the tests, and then I'll approve and merge.

Thank you for working on this!

tests/unit/secrets_test.yaml Outdated
- equal:
path: spec.template.spec.initContainers[2].envFrom[0].secretRef.name
value: "hello-world"
documentIndex: 0
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment on the documentIndex for deployments and jobs to say which deployment/job this test is for?

Aside: I really wish we could use something other than index to select documents (and containers, ...) With helm-unittest. But oh well.

Copy link
Contributor Author

@bmarick bmarick Mar 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I added a comment saying

# Checking Deployment/Job {name} was properly updated

The {name} is a place holder for the metadata.name without the {{ .Release.Name }} included
Please let me know if that works for you

Copy link
Member

Choose a reason for hiding this comment

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

oh. Sorry. I was not very specific. I was thinking of something more terse:

documentIndex: 0 # st2auth

If you look in the other test files, I've done something like that. Is that helpful or confusing for you? If adding this comment is confusing, then I can change how I do it in the other files for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@bmarick bmarick force-pushed the externalAuthSecret branch 2 times, most recently from dd7e2ee to 0bfbffb Compare March 17, 2023 17:13
@bmarick
Copy link
Contributor Author

bmarick commented Mar 17, 2023

The tests appear to have failed due to a rate limit

@bmarick
Enable Existing Auth Secret
7f74438 
Adjust authExistingSecret to existingAuthSecret; to match similar existingConfigSecret
Add job tests and catch a missing location
Properly document which Job/deployment was updated
@bmarick bmarick requested a review from cognifloyd March 20, 2023 18:06
@cognifloyd cognifloyd merged commit 7ee24be into StackStorm:master Mar 20, 2023
@cognifloyd cognifloyd mentioned this pull request Mar 20, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cognifloyd cognifloyd cognifloyd approved these changes

Assignees
No one assigned
Labels
size/L PR that changes 100-499 lines. Requires some effort to review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable Secret Generation / Provide external secret names
2 participants
@bmarick @cognifloyd