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

mount empty secrets #3081

Merged
merged 1 commit into from
Sep 7, 2022
Merged

mount empty secrets #3081

merged 1 commit into from
Sep 7, 2022

Conversation

coryb
Copy link
Collaborator

@coryb coryb commented Sep 3, 2022

This is a somewhat pedantic edge case, but I think we should create the mount point for secret files even if the source secret file is empty. Otherwise, it is hard to debug why secrets are not showing up when requested in the LLB. Currently, they are just silently ignored which was pretty confusing and caused me to waste time debugging my pipeline only to finally realize my secrets were corrupt and empty.

If allowing an empty secret mount is not acceptable, perhaps we could just add a warning to the buildkit log to indicate we are ignoring the empty mount, although I suspect other users might not have ready access to the buildkit logs and maybe continue to be confused like me.

We should create the mount point for secret files even if the source
secret file is empty.  Otherwise it is hard to debug why secrets
are not showing up when requested in the LLB.  Currently they are just
silently ignored which was pretty confusing.

Signed-off-by: coryb <cbennett@netflix.com>
@tonistiigi
Copy link
Member

Looks like this was added in https://github.com/moby/buildkit/pull/1551/files#diff-e1fc4676394384145cb0effe7c7e3337b8f18a3ee0184e5a7bc9d1167f34420cR442 , possibly to (erroneously) handle a case where session callback was called in some weird order. But I don't really understand what that case even was.

@tonistiigi
Copy link
Member

It is a bit weird though that dt is nil if the secret was indeed passed. I would understand that this is wrong if it read len(dt) == 0 instead.

@coryb
Copy link
Collaborator Author

coryb commented Sep 4, 2022

It is a bit weird though that dt is nil if the secret was indeed passed. I would understand that this is wrong if it read len(dt) == 0 instead.

Yeah, I agree. I confirmed that I see the moby.buildkit.secrets.v1.Secrets/GetSecret grpc call, and it succeeds. I also confirmed removing the dt == nil check allows the mount to get created for the zero-sized secret. I am not sure why it is nil. I confirmed that grpc should set the Data to an empty slice in the zero size case: https://github.com/moby/buildkit/blob/master/session/secrets/secrets.pb.go#L773

So it seems like "something" is missing, but not sure what that might be.

@coryb
Copy link
Collaborator Author

coryb commented Sep 4, 2022

It seems to be a general issue with grpc messages that use bytes. If the bytes field is zero, then the marshaling is skipped. It is derived from the message Size func which causes dAtA to be an empty slice with another optimization to skip adding the length when the Data is empty. So the marshaled dAtA message is empty and I confirmed the unmarshalled message is also empty. So we never reach the m.Data = []byte{} initialization.

So any case where we use bytes it seems we cannot treat nil to determine the validity of the contents where empty content is valid.

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