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

Implemented optional use of existing secret for mgmt token #175

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

amentebekele-okta
Copy link
Contributor

This PR adds an optional functionality to mount and use existing secret as mgmt-token for the helm chart, instead of generating a new one. This is useful when specifying more than 1 replica and we want them to use the same token.

@amentebekele-okta
Copy link
Contributor Author

@eshepelyuk when you get a chance, can I get a review for this. Thanks!

Copy link
Contributor

@eshepelyuk eshepelyuk left a comment

Choose a reason for hiding this comment

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

Hello

This PR requires lint test cases and I also I would like to see a new e2e scenario showing that multiple instances can share the secret resource.

@amentebekele-okta
Copy link
Contributor Author

@eshepelyuk Thanks for taking a look. I have added the lint and e2e tests.

# Used for setting the mgmt token used for authz instead of auto generated default
# mgmtToken:
# secretName: name of the secret
# secretKey: key from the secret
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to create a default value for secretKey, so this field can be optional.

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

Copy link
Contributor

@eshepelyuk eshepelyuk left a comment

Choose a reason for hiding this comment

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

Instead of introducing setup.sh and changes to e2e pipeline, use special e2e helm template charts/opa-kube-mgmt/templates/ingressroute.yaml.

Add there a new block that renders your secret, if a new, e2e only value is set, that you can add to values.yaml of your e2e test.

test/linter/test.sh Show resolved Hide resolved
@amentebekele-okta
Copy link
Contributor Author

Instead of introducing setup.sh and changes to e2e pipeline, use special e2e helm template charts/opa-kube-mgmt/templates/ingressroute.yaml.

Add there a new block that renders your secret, if a new, e2e only value is set, that you can add to values.yaml of your e2e test.

I can see that would also work. But, using the e2e flag, will make it render for all the other tests which don't use this secret. However, I think the setup.sh hook approach is useful, for other cases as well. It is also not affecting other tests, because the script is called only if it exists. What do you think?

@eshepelyuk
Copy link
Contributor

Instead of introducing setup.sh and changes to e2e pipeline, use special e2e helm template charts/opa-kube-mgmt/templates/ingressroute.yaml.
Add there a new block that renders your secret, if a new, e2e only value is set, that you can add to values.yaml of your e2e test.

I can see that would also work. But, using the e2e flag, will make it render for all the other tests which don't use this secret. However, I think the setup.sh hook approach is useful, for other cases as well. It is also not affecting other tests, because the script is called only if it exists. What do you think?

Apparently, as I already mentioned, you have to introduce another value that will trigger render of the secret.

@amentebekele-okta
Copy link
Contributor Author

Instead of introducing setup.sh and changes to e2e pipeline, use special e2e helm template charts/opa-kube-mgmt/templates/ingressroute.yaml.
Add there a new block that renders your secret, if a new, e2e only value is set, that you can add to values.yaml of your e2e test.

I can see that would also work. But, using the e2e flag, will make it render for all the other tests which don't use this secret. However, I think the setup.sh hook approach is useful, for other cases as well. It is also not affecting other tests, because the script is called only if it exists. What do you think?

Apparently, as I already mentioned, you have to introduce another value that will trigger render of the secret.

Sure, no problem. I have pushed changes.

Copy link
Contributor

@eshepelyuk eshepelyuk left a comment

Choose a reason for hiding this comment

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

LGTM
Plz rebase and squash into a single commit, if CI job is green - ready to merge.

Signed-off-by: Amente Bekele <amente.bekele@okta.com>
@eshepelyuk
Copy link
Contributor

Hello @amentebekele-okta
The feature is available in 7.2.0.
Thanks for the contribution.

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

2 participants