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

rbd: unexport existing KMS structs for vault, AWS and KeyProtect #2750

Merged
merged 7 commits into from
Jan 28, 2022
Merged

rbd: unexport existing KMS structs for vault, AWS and KeyProtect #2750

merged 7 commits into from
Jan 28, 2022

Conversation

humblec
Copy link
Collaborator

@humblec humblec commented Jan 6, 2022 •
edited
Loading

At present the KMS structs are exported and ideally we should be
able to work without exporting the same.

Updates #2725

Signed-off-by: Humble Chirammal hchiramm@redhat.com

@humblec humblec requested a review from nixpanic January 6, 2022 05:24
@mergify mergify bot added the component/rbd Issues related to RBD label Jan 6, 2022
@humblec
Copy link
Collaborator Author

humblec commented Jan 6, 2022

/retest ci/centos/upgrade-tests-rbd

@humblec humblec added this to the release-3.5.0 milestone Jan 6, 2022
@nixpanic nixpanic removed this from the release-3.5.0 milestone Jan 6, 2022
@nixpanic
Copy link
Member

nixpanic commented Jan 6, 2022

This is just a cleanup, there is no requirement in including this in 3.5.0. We should focus on other PRs that add functionality or fix bugs instead. The 3.5.0 release is already a day too late.

@mergify
Copy link
Contributor

mergify bot commented Jan 11, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@humblec
Copy link
Collaborator Author

humblec commented Jan 18, 2022

Rebased to devel.. ptal @nixpanic

@humblec humblec requested a review from a team January 18, 2022 08:15
@humblec
Copy link
Collaborator Author

humblec commented Jan 20, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.22

@humblec
Copy link
Collaborator Author

humblec commented Jan 20, 2022

/retest ci/centos/mini-e2e/k8s-1.21

@humblec
Copy link
Collaborator Author

humblec commented Jan 20, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.22

@humblec humblec requested a review from Madhu-1 January 20, 2022 07:03
@humblec
Copy link
Collaborator Author

humblec commented Jan 24, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.22

@humblec
Copy link
Collaborator Author

humblec commented Jan 24, 2022

@nixpanic @Rakshith-R can you take a look at this ?

Rakshith-R
Rakshith-R reviewed Jan 24, 2022
View reviewed changes
Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

unexport type DEKStoreType string, type IntegratedDEK struct{} and type DEKStore interface too ?

internal/kms/secretskms.go Outdated Show resolved Hide resolved
internal/kms/secretskms.go Outdated Show resolved Hide resolved
@humblec humblec force-pushed the unexport-2 branch 4 times, most recently from 13b22d5 to 3cc45c7 Compare January 24, 2022 08:09
@humblec
Copy link
Collaborator Author

humblec commented Jan 24, 2022

unexport type DEKStoreType string, type IntegratedDEK struct{} and type DEKStore interface too ?

@Rakshith-R done other both except the interface, which need by outside package?

@Rakshith-R
Copy link
Contributor

unexport type DEKStoreType string, type IntegratedDEK struct{} and type DEKStore interface too ?

@Rakshith-R done other both except the interface, which need by outside package?

👍, there seems to be a stray file internal/kms/.keyprotect.go.swp @humblec

@humblec
Copy link
Collaborator Author

humblec commented Jan 24, 2022

unexport type DEKStoreType string, type IntegratedDEK struct{} and type DEKStore interface too ?

@Rakshith-R done other both except the interface, which need by outside package?

+1, there seems to be a stray file internal/kms/.keyprotect.go.swp @humblec

ah. multiple window switching .. removed it .. thanks @Rakshith-R

nixpanic
nixpanic requested changes Jan 24, 2022
View reviewed changes
internal/kms/kms.go Outdated Show resolved Hide resolved
internal/kms/kms.go Outdated Show resolved Hide resolved
@humblec humblec force-pushed the unexport-2 branch 2 times, most recently from 1ad6c17 to 2b92647 Compare January 24, 2022 13:15
@nixpanic
Copy link
Member

At present the KMS structs are exported and ideally we should be able to work without exporting the same.

Updates #2750

I am wondering if there is a goal that you want to reach with this. The linked #2750 refers to this PR, and that is not helpful. Depending on the goal, IntegratedKMS may need to stay exported.

@humblec
Copy link
Collaborator Author

humblec commented Jan 25, 2022

At present the KMS structs are exported and ideally we should be able to work without exporting the same.
Updates #2750

I am wondering if there is a goal that you want to reach with this. The linked #2750 refers to this PR, and that is not helpful. Depending on the goal, IntegratedKMS may need to stay exported.

#2725 is the correct issue link which came out of a review comment (#2723 (comment)). Corrected the link now, may be we will keep IntegratedDEK unexported and open it up if we really need? that sound fine to me.

@humblec humblec requested a review from nixpanic January 25, 2022 08:07
@humblec
Copy link
Collaborator Author

humblec commented Jan 25, 2022

@nixpanic @Rakshith-R ptal.. thanks .

@humblec humblec requested a review from a team January 27, 2022 12:20
@humblec
rbd: unexport aws kms structs
d26308b 
At present the KMS structs are exported and ideally we should be
able to work without exporting the same.

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: unexport KeyProtect kms struct
1fbed7c 
At present the KMS structs are exported and ideally we should be
able to work without exporting the same.

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: Unexport IntegratedDEK struct from kms
86a97ad 
This commit unexport IntegratedDEK struct from KMS
implementation

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: Unexport SecretsMetadataKMS struct
d00b151 
This commit unexport SecretsMetadataKMS struct from KMS
implementation

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: Unexport VaultTenantSA struct from KMS implementation
d47c1cf 
This commit unexport VaultTenantSA struct from KMS implemenation
of Vault KMS.

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: unexport vaultTokenSA struct from KMS implementation
722af91 
This commit unexport the vaultTokenSA from the vault KMS
implementation

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec
rbd: unexport SecretsKMS from KMS implementation
ba0afc2 
This commit unexport SecretsKMS from KMS implementation.

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@humblec humblec added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Jan 28, 2022
@mergify mergify bot merged commit 4ee4fdf into ceph:devel Jan 28, 2022
nixpanic added a commit to nixpanic/ceph-csi that referenced this pull request Jun 27, 2022
@nixpanic
[DOWNSTEAM-ONLY] Bug 2098562: fix build failure related to VaultTenan…
a6e0e02 
…tSA.createToken

Pull-Request ceph#99 backported a commit that applies cleanly, but causes a
build failure:

    internal/kms/vault_sa.go:313:12: kms.createToken undefined (type *VaultTenantSA has no field or method createToken)
    internal/kms/vault_sa.go:344:12: undefined: vaultTenantSA

In recent versions, `vaultTenantSA` is used, but release-4.10 is stuck
on the old naming of `VaultTenantSA`. The unexporting was done in
upstream ceph#2750, which includes more changes than what we
want in a backport.

Signed-off-by: Niels de Vos <ndevos@redhat.com>
@nixpanic nixpanic mentioned this pull request Jun 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yati1998 yati1998 yati1998 approved these changes

@nixpanic nixpanic nixpanic approved these changes

@Madhu-1 Madhu-1 Awaiting requested review from Madhu-1

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

Assignees
No one assigned
Labels
ci/retry/e2e Label to retry e2e retesting on approved PR's cleanup component/rbd Issues related to RBD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@humblec @nixpanic @Rakshith-R @yati1998