Add mount and namespace for Vault Enterprise use in new Kubernetes pr…#93
Add mount and namespace for Vault Enterprise use in new Kubernetes pr…#93briantopping wants to merge 1 commit intoupbound:mainfrom
Conversation
|
I'm not sure why the build is failing, |
|
Hey @turkenh! Would it be possible to get a preliminary review? |
|
@briantopping i think there is a accidential deletion of a CRD, this is why |
|
Hi @sergenyalcin I saw you approving a few of the latest PRs. |
|
@yehlo, we'll soon release a new version of this provider with Crossplane v2 support. We're targeting to include this PR in this upcoming release. |
sergenyalcin
left a comment
sergenyalcin
left a comment
There was a problem hiding this comment.
Hi @briantopping, thank you for this PR. I left a comment in terms of backward compatibility.
Apart from that, we will migrate this provider to crossplane v2 compatible state and namespaced providers and whole repo structure will change. Since of this, before merging the v2 migration work, merging your PR will be better for getting rid of resolving conflicts in your side.
As far as I see, check-diff is failing. Since of logs are deleted in github I can't see the reason in here but I ran a make check-diff in my local. And there are really diff in there. It seems that there are some unintentioanl changes in the generated codes:
Could you please check this again? Thanks!
| return errors.New(errNoRole) | ||
| } | ||
|
|
||
| if pc.Spec.Mount == nil { |
There was a problem hiding this comment.
To be honest, I don't have a strong knowledge of provider-vault auth methods. But it seems that here we are making a breaking change for the secret cred auth and making the Mount required, right?
I see that there was a defaulting here (to kubernetes value before. And I assume that it worked that way. Therefore, I think we should preserve this old behaviour. Currently, this value is required, and we return an error when it is not provided. However, in the past, we defaulted when it was not provided.
From the perspective of preserving the old behaviour, I think we should continue to default instead of returning an error here. (Of course, I assume that the old situation, i.e., when ‘kubernetes’ was passed to Mount, worked.) What do you think?
There was a problem hiding this comment.
To be honest, I don't have a strong knowledge of provider-vault auth methods. But it seems that here we are making a breaking change for the secret cred auth and making the Mount required, right?
If you take a look at the git blame, this is new code. I found these errors before the code was merged but lost track of my commits before they were pushed. This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".
From the perspective of preserving the old behaviour, I think we should continue to default instead of returning an error here. (Of course, I assume that the old situation, i.e., when ‘kubernetes’ was passed to Mount, worked.) What do you think?
Yes, that makes good sense, thanks. I will update the PR.
There was a problem hiding this comment.
Btw, I am also not a vault expert too, looking at the changes purely from the provider perspective. I am a bit confused, I would be happy if you can clarify the following:
This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".
I see that "capability-wise" Vault OSS users are not affected, but from ProviderConfig API perspective this is breaking. The kubernetes auth code (prior to this PR) is already shipped with v2.2.0.
So there can be potential adopters, and they can create a kubernetes auth ProviderConfig without specifying spec.mount. After this PR, their ProviderConfig would not work, and they would be forced to configure a parameter, which they actually has no other option.
My questions are:
-
(prior to this PR) did the kubernetes auth configuration never work or was it not valid at all, even for OSS? In other words, are we fixing a bug for something that never worked here? I am assuming that is not the case.
-
What are the potential
spec.mountvalues for Vault Enterprise users that configure kubernetes auth? Wouldkubernetesstill be a sane default for Enterprise users? I assume that Enterprise users would need to remember to configure this in any case since you said:
It's really only required for Vault Enterprise users, who will almost never use "kubernetes".
I am in favor of defaulting it to kubernetes,
but I am also trying to assess the scenario, where an Enterprise users "forgets" to set spec.mount and it gets defaulted to kubernetes. Would there be an adverse effect of this? Would it fail or result in an unintended valid configuration?
There was a problem hiding this comment.
This change really doesn't affect Vault OSS users because they can't change their instance from "kubernetes" (VOSS only allows the one). It's really only required for Vault Enterprise users, who will almost never use "kubernetes".
VOSS users can indeed have several kubernetes secret engines enabled = having an option to set mount point is not a Vault Enterprise feature. It is a must if several kubernetes secret engines exist on a Vault instance (OSS, or enterprise).
(prior to this PR) did the kubernetes auth configuration never work or was it not valid at all, even for OSS? In other words, are we fixing a bug for something that never worked here? I am assuming that is not the case.
It did work, with default kubernetes path / mount point only.
I am in favor of defaulting it to
kubernetes,
but I am also trying to assess the scenario, where an Enterprise users "forgets" to set spec.mount and it gets defaulted to kubernetes. Would there be an adverse effect of this? Would it fail or result in an unintended valid configuration?
Fortunately keeping the default kubernetes shouldn't have side affects, i.e. setting wrong mount point wouldn't validate any auth token as it would be provided by wrong cluster / wouldn't match required role.
|
Are there any updates on this ? Does any additional work need to be done here? My team needs the ability to use k8s auth in vault enterprise. |
|
@jtucci the PR has been reviewed and feedback left. To move this PR forward we need to conflicts resolved and tests to pass. |
8 participants
Description of your changes
Fixes #78 with additions for Mount and Namespace
I have:
make reviewable testto ensure this PR is ready for review.