Chart always expects accessKey and secretKey to be defined when using s3 storage

[robmyersrobmyers]

The current chart always secrets.s3.accessKey and secrets.s3.secretKey to be defined when using s3 storage, which can break if you rely on ec2 instance profiles.

The diff below checks to make sure .Values.secrets.s3 is defined before using it, which appears to resolve my issue.

diff -uNrp twuni-docker-registry.helm-cb69658/templates/deployment.yaml twuni-docker-registry.helm-cb69658-working/templates/deployment.yaml
--- twuni-docker-registry.helm-cb69658/templates/deployment.yaml        2022-08-09 17:13:42.000000000 +0000
+++ twuni-docker-registry.helm-cb69658-working/templates/deployment.yaml        2022-08-15 19:01:12.357116958 +0000
@@ -124,6 +124,7 @@ spec:
                   name: {{ template "docker-registry.fullname" . }}-secret
                   key: azureContainer
 {{- else if eq .Values.storage "s3" }}
+            {{- if .Values.secrets.s3 }}
             {{- if or (and .Values.secrets.s3.secretKey .Values.secrets.s3.accessKey) .Values.secrets.s3.secretRef }}
             - name: REGISTRY_STORAGE_S3_ACCESSKEY
               valueFrom:
@@ -136,6 +137,7 @@ spec:
                   name: {{ if .Values.secrets.s3.secretRef }}{{ .Values.secrets.s3.secretRef }}{{ else }}{{ template "docker-registry.fullname" . }}-secret{{ end }}
                   key: s3SecretKey
             {{- end }}
+            {{- end }}
             - name: REGISTRY_STORAGE_S3_REGION
               value: {{ required ".Values.s3.region is required" .Values.s3.region }}
           {{- if .Values.s3.regionEndpoint }}
diff -uNrp twuni-docker-registry.helm-cb69658/templates/secret.yaml twuni-docker-registry.helm-cb69658-working/templates/secret.yaml
--- twuni-docker-registry.helm-cb69658/templates/secret.yaml    2022-08-09 17:13:42.000000000 +0000
+++ twuni-docker-registry.helm-cb69658-working/templates/secret.yaml    2022-08-15 18:58:39.077118130 +0000
@@ -25,7 +25,7 @@ data:
   azureAccountKey: {{ .Values.secrets.azure.accountKey | b64enc | quote }}
   azureContainer: {{ .Values.secrets.azure.container | b64enc | quote }}
     {{- end }}
-  {{- else if eq .Values.storage "s3" }}
+  {{- else if and (eq .Values.storage "s3") .Values.secrets.s3 }}
     {{- if and .Values.secrets.s3.secretKey .Values.secrets.s3.accessKey }}
   s3AccessKey: {{ .Values.secrets.s3.accessKey | b64enc | quote }}
   s3SecretKey: {{ .Values.secrets.s3.secretKey | b64enc | quote }}

Please let me know if I'm doing it wrong or missed some documentation. Thanks!

[canterberry]

Thanks, @robmyersrobmyers! This diff looks fine to me, but conflicts, in part, with #62, since it touches some of the same code.

cc @ddelange @kuzaxak This might have been why there was a default value being set for secrets.s3 in an iteration of #68. The diff above seems like a less fragile solution, though it does add some more complexity. Is there a maybe syntax/function/pattern that could be used for deep object traversal? Something akin to JavaScript's foo?.bar syntax, maybe?

[ddelange]

For the 'functional' approach ref https://github.com/twuni/docker-registry.helm/pull/62#discussion_r942064266

[jrkarnes]

Given the status of this issue as OPEN, I'm going to assume this was never actually fixed.

I have now run into this defect today.

[jrkarnes]

For anyone that isn't a maintainer:

The fastest way to get around this problem and use IAM roles is to set secrets.s3.accessKey and secrets.s3.secretKey to ''. This will cause the if conditionals in the template to evaluate to false and will allow the template to render correctly (avoiding the nilPointer issue).

For the maintainers

This is the current state of your secrets.yaml template (the part that's relevant anyway): templates/secrets.yaml

  {{- else if eq .Values.storage "s3" }}
    {{- if and .Values.secrets.s3.secretKey .Values.secrets.s3.accessKey }}
  s3AccessKey: {{ .Values.secrets.s3.accessKey | b64enc | quote }}
  s3SecretKey: {{ .Values.secrets.s3.secretKey | b64enc | quote }}
    {{- end }}

The problem with this is that it assumes that someone who sets storage == "s3" will also set secrets.s3.* to something, even if that something is empty. This is not called out in the documentation and is actually directly contrary to the CNCF Distribution documentation.

A better approach to this is as follows:

  {{- else if eq .Values.storage "s3" }}
    {{- if and (ne (default .Values.secrets.s3.secretKey "empty") "empty") (ne (default .Values.secrets.s3.accessKey "empty") "empty") }}
  s3AccessKey: {{ .Values.secrets.s3.accessKey | b64enc | quote }}
  s3SecretKey: {{ .Values.secrets.s3.secretKey | b64enc | quote }}
    {{- end }}

This should cause the undefined keys to no longer produce errors when users do not define secrets.s3 when they use storage = "s3". A non-empty string evaluates to true when used in boolean expressions in Jinja2, so if these values are both set, then the data element should be populated correctly.

[DavidMakin]

Even with setting the key and secret as '' I was getting panic: s3aws: NoCredentialProviders: no valid providers in chain. Deprecated. errors.

I also needed to change image.tag=3.0.0-beta.1 to pull in this fix https://github.com/distribution/distribution/pull/3245