Missing annotations & labels

[jalberto]

When creating a new sealed-secret based on a existing secret.yaml labels and annotations are lost.

This is very important, as some labels and annotations are related to helm lifecycle.

[jalberto]

is this intended behaviour?

[anguslees]

No, it's another thing that got lost in the 0.6.0 change to per-key encryption. If you use 0.5.x kubeseal (perhaps even with a 0.6 controller), then you should have annotations+labels preserved.

I think the desired semantics with per-key encryption, is that the labels+annotations will end up in plaintext (no encryption), and in a new spec.template subfield of the SealedSecret. Would this be ok for your usecase (in particular, that the labels/annotations are not hidden by encryption)?

[jalberto]

@anguslees yes, that will be perfect, labels/annotations should not keep any sensible data.

In other hand, "another thing that got lost in the 0.6.0 change" I am worried to use kubeseal in production if there is risk of accidentally loosing features like this from one stable version to the next.

[cknowles]

@anguslees we'd be happy to fix this. I can't see much difference between combined encryption and per-key in the PR for that. I went through and didn't spot much about the labels/annotations from the original import so probably missed something. If you could point us in the right direction we can submit a PR.

[anguslees]

In other hand, "another thing that got lost in the 0.6.0 change" I am worried to use kubeseal in production if there is risk of accidentally loosing features like this from one stable version to the next.

(Fwiw, the "other thing" was the Secret.Type value - ie: secrets of types other than "Opaque")

I'm afraid I can't give a useful response to non-specific future concerns. As with everything else, a conservative deployment involves qualifying new releases of your infrastructure against your requirements. If you can't/don't want to do that, then you can also just stick with a fixed (unchanging) version. Again, as with everything else, the safest option for continuing upgrades is to become actively involved in upstream development and ensure that your particular use case is well represented in tests and design discussions.

In this case, any existing SealedSecrets created using older versions of sealed-secrets continue to work just fine (with metadata labels, etc) on the new 0.6.0 sealed-secrets controller. It's "just" SealedSecrets created with the 0.6.0 kubeseal which use a newer encoding scheme, which (unfortunately) doesn't capture this additional secret metadata. (This is what I was trying to describe in my first comment on this issue above)

[anguslees]

@c-knowles thanks for the offer. I think I've got most of this implemented already, however (see below).
The "real" fix is adding tests that better exercises these corner-cases with the new per-key scheme.

If you could point us in the right direction we can submit a PR

The following line in the "backwards compatibility" branch means we initialise the full v1.Secret object from the encrypted blob, potentially including metadata and secret.type. In the "per-key" branch, we start with an uninitialised (default) v1.Secret, and just set specific values.

https://github.com/sullerandras/sealed-secrets/blob/52f32b5683b7ebda5b3336560e6a9ef1a59930b6/pkg/apis/sealed-secrets/v1alpha1/sealedsecret_expansion.go#L134

That's the cause.

For the fix, we need somewhere to put the labels/metadata for the per-key case - and we don't want to just blindly copy them from the SealedSecret (since some annotations might be misinterpreted on the "wrong" object). I'm currently thinking of adding a "template" that encompasses everything except the actual secret.data:

ie:

// SecretTemplateSpec describes the structure a Secret should have when created
// from a template
type SecretTemplateSpec struct {
	// Standard object's metadata.
	// More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#metadata
	// +optional
	metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`

	// Used to facilitate programmatic handling of secret data.
	// +optional
	Type apiv1.SecretType `json:"type,omitempty" protobuf:"bytes,3,opt,name=type,casttype=SecretType"`
}

// SealedSecretSpec is the specification of a SealedSecret
type SealedSecretSpec struct {
	// Template defines the structure of the Secret that will be
	// created from this sealed secret.
	// +optional
	Template SecretTemplateSpec `json:"template,omitempty"`

	// Data is deprecated and will be removed eventually. Use per-value EncryptedData instead.
	Data          []byte            `json:"data,omitempty"`
	EncryptedData map[string][]byte `json:"encryptedData"`
}

Note I've also moved the Type from #88 into the Template. This should be ok without migration code, since we haven't made a release including #88 yet.

@c-knowles (or others) does that look reasonable? Can you see a better / more-obvious way to encode the extra "non-secret" parts of the Secret?

[cknowles]

@anguslees That makes sense and yes we should add more tests, if you want some help with adding more let us know. The template way seems good, a bit like the jobTemplate in CronJob.

CC @sullerandras.

[jalberto]

thanks for the work!
Any ETA for a release?

[anguslees]

Any ETA for a release?

None yet. It will take a few weeks, since I haven't finished writing the code referred to above (then tests, etc).

Note again that in the meantime you can use kubeseal from an earlier release, and it will preserve annotations and labels from the Secret.

[mrballcb]

Is this the same issue as "when I create a new sealed secret with labels, the resulting secret does not contain any labels" ? Basically our sealedsecrets template looks like this in helm:

{{- if .Values.encryptedData }}
apiVersion: bitnami.com/v1alpha1
kind: SealedSecret
metadata:
  name: {{ template "uniquename" . }}
  labels:
    app: {{ template "name" . }}
    chart: "{{ .Chart.Name }}-{{ .Chart.Version | replace "+" "_" }}"
    release: "{{ .Release.Name }}"
    heritage: "{{ .Release.Service }}"
spec:
  encryptedData:
{{ toYaml .Values.encryptedData | indent 4 }}
{{- end }}

I am trying to search with kubectl get sealedsecrets,secrets,pods -n ENV -l app=APP. It works for sealedsecrets and pods, but the secrets are being generated without labels. Are we missing something obvious that would create the secrets with labels?

I'll create a new issue if not related to this issue (#92).

[anguslees]

@mrballcb yes, same issue with same solution (use older kubeseal on a Secret that already has the required labels. Older kubeseal should produce a SealedSecret with a single spec.data base64 blob which includes more of the original Secret than the newer spec.encryptedData form).
The newer controller will understand both forms, so this only affects the way you generate your SealedSecrets.

[linoecarrillo]

@anguslees

For the fix, we need somewhere to put the labels/metadata for the per-key case - and we don't want to just blindly copy them from the SealedSecret (since some annotations might be misinterpreted on the "wrong" object).

What do you mean by annotations being misinterpreted on the "wrong" object? Can you give an example?

[anguslees]

What do you mean by annotations being misinterpreted on the "wrong" object? Can you give an example?

One (poor) implementation might be to just copy the annotations from SealedSecret to the Secret we're creating. The downsides of this are that the annotations might mean something on the SealedSecret that doesn't make sense on the Secret, eg: the kubectl.kubernetes.io/last-applied-configuration used by kubectl apply will be incorrect if it is blindly copied to another resource.

This is why all the core resources have a "template.metadata" that gets used to create the derived object's metadata - and not just copying the top-level metadata over.

[peterbosalliandercom]

any progress?

[starkers]

This would be really good to fix please.. its a blocker to stop using a tool like kubed to distribute the secrets after-the-fact

[DewaldDeJager]

Our team is also facing issues because of this bug. Do you need any help with this fix @anguslees?

[mkmik]

@DewaldDeJager, @chrisharm made some progress in #170; it's right next in my review queue