-
Notifications
You must be signed in to change notification settings - Fork 30
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
Implement kustomizeOptions
#109
Implement kustomizeOptions
#109
Conversation
3769597
to
75ea537
Compare
@dhaiducek are there any security repercussions with this approach? I'm thinking in particular that when running in App Sub, the Helm execution would end up using App Sub's service account but don't know for sure. |
I was wondering about input on security implications. The risks feel similar to enabling Kustomize. I'm thinking it's toggled on by the user, and I think would call the internal Helm CLI, but that doesn't exist in App Sub, so it'd just fail in that case. (I just tried executing into the subscription container, and That said, I suppose I'll have to do some more digging around ArgoCD, but the situation there might be similar. |
hi @dhaiducek .. this just "works" in argocd .. as it has helm and kustomize support already i.e. in the argocd-repo-server pod they have helm in /usr/local/bin/helm |
This would be helpful. as stated above, this works in ArgoCD. Looks like a maintainer just needs to add a |
@dhaiducek I think we should approve it, I've a customer waiting on this and we reviewed it again and we do not see the risk, the customer would use ArgoCD and this would just use ArgoCD's Service Account. We could also doc any security related topics. What do you think? |
@ch-stark and @dhaiducek , how about we require the user setting an environment variable to enable Helm in Kustomize? That way they opt-in to this feature and ensure it is secure to do so. In Application Subscription, we can probably enable this by default and document it for ArgoCD. |
That works for me--I've been thinking we just need to get this feature out there, and having a higher-level opt in sounds like a good way to go. |
75ea537
to
e22a6df
Compare
/hold for reviews ...and tests? Though that may be better handled in an integration test... |
@dhaiducek could you please add documentation of the environment variable in the read me? |
e22a6df
to
9c7a3db
Compare
9c7a3db
to
227a9e6
Compare
227a9e6
to
b3ad200
Compare
Updating the |
Allows passing flags equivalent to `--enable-helm` via the environment variable `POLICY_GEN_ENABLE_HELM` ref: https://issues.redhat.com/browse/ACM-9060 Signed-off-by: Dale Haiducek <19750917+dhaiducek@users.noreply.github.com>
b3ad200
to
40f8f2d
Compare
/unhold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, mprahl The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4bf2b47
into
open-cluster-management-io:main
Allows passing flags equivalent to
--enable-helm
via the environment variablePOLICY_GEN_ENABLE_HELM
ref: https://issues.redhat.com/browse/ACM-9060
Closes #105