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

Accept and seal stringData into secret #221

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

pinkavaj
Copy link

Ref #104

}
for key, values := range secret.StringData {
secret.Data[key] = []byte(values)
delete(secret.StringData, key)
Copy link

Choose a reason for hiding this comment

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

What is the reasoning for deleting the values? Just curious. :)

Copy link
Author

Choose a reason for hiding this comment

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

It is my obsession with doing thinks right. When for example someone latter would serialize this object it will get duplicate fileds in both data and stringData sections if delete is not done. I agree it is very improbable and actualy technicaly unnecessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I was first thinking of this issue I though we should make kubeseal take a secret that contains stringData as input, produce a sealed secret that once converted into a secret would be identical to the original, namely keeping the stringData intact. This means we would have to somehow encode that in the sealed secret CRD schema.

But this approach seems pragmatic enough. I wonder how would backwards compatibility look like if we later decide that reversibility is important.

Copy link

Choose a reason for hiding this comment

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

Makes good sense. Could a test for this maybe be usefull for future reference? So the behaviour becomes very explicit?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I can write right now more complicated test suite in go (this is my second commit in go ... :). But if You give me a hint I will try.

Copy link
Author

Choose a reason for hiding this comment

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

Actually kubectl does the same operation, data and stringData cannot be distinguished once pushed into Kubernetes api.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah interesting, didn't know (fwiw I'm not fond of this kind of magic in k8s...).

Happy to merge when we have some tests

Copy link
Author

@pinkavaj pinkavaj Aug 14, 2019

Choose a reason for hiding this comment

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

I have managed to dig this think a bit deeper and add some test, please check if it is enought. The whole thing have changed.

Copy link
Author

Choose a reason for hiding this comment

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

The TestSealRoundTripStringDataConversion is more or less duplicate of TestSealRoundTrip, so mey be keep only one of those?

@mkmik
Copy link
Collaborator

mkmik commented Aug 26, 2019

bors r+

bors bot added a commit that referenced this pull request Aug 26, 2019
221: Accept and seal stringData into secret r=mkmik a=pinkavaj

Ref #104

Co-authored-by: Jiri Pinkava <[email protected]>
@bors
Copy link
Contributor

bors bot commented Aug 26, 2019

Build succeeded

@bors bors bot merged commit e25eae4 into bitnami-labs:master Aug 26, 2019
@pinkavaj pinkavaj deleted the pi-stringdata branch August 26, 2019 18:30
@mkmik mkmik added this to the v0.8.2 milestone Aug 28, 2019
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.

4 participants