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

Fix normalization of base64 encoded secrets.data values to strip whitespace #2715

Merged
merged 6 commits into from
Dec 13, 2023

Conversation

rquitales
Copy link
Contributor

@rquitales rquitales commented Dec 13, 2023

Proposed changes

This pull request introduces a normalization mechanism for secret.data base64 encoded values, specifically stripping whitespace to prevent unintended spurious diffs.

Changes Made:

  • Created comprehensive integration and unit tests to verify and highlight the broken behavior associated with secret.data values containing whitespaces.
  • Updated the provider to incorporate the normalization process, ensuring that all related tests pass seamlessly.

Related issues (optional)

Fixes: #2681

@rquitales rquitales requested review from EronWright and a team December 13, 2023 20:46
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link
Contributor

@EronWright EronWright left a comment

Choose a reason for hiding this comment

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

I don't feel comfortable with this fix for a few reasons.

  1. I don't understand why one would get spurious diffs if the whitespace were present.
  2. I feel it is presumptuous to manipulate the value; could there be a legit use case for trailing whitespace?

@rquitales
Copy link
Contributor Author

rquitales commented Dec 13, 2023

I don't feel comfortable with this fix for a few reasons.

  1. I don't understand why one would get spurious diffs if the whitespace were present.
  2. I feel it is presumptuous to manipulate the value; could there be a legit use case for trailing whitespace?
  1. The occurrence of spurious diffs is attributed to Kubernetes stripping whitespace during the object creation process. Consequently, when comparing the object in its live state to our input, which includes whitespace, a diff arises. This can be illustrated with the following YAML manifest:
apiVersion: v1
kind: Secret
metadata:
  name: dotfile-secret
data:
  secret: "dmFsdWUtMg0KDQo=\n\n\n"

When this secret is applied to the cluster using kubectl apply -f secret.yaml, the output of kubectl get secret -f secret.yaml shows the absence of trailing whitespace in the stored value. Notice the difference between .data.secret and the one in kubectl.kubernetes.io/last-applied-configuration

apiVersion: v1
data:
  secret: dmFsdWUtMg0KDQo=
kind: Secret
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","data":{"secret":"dmFsdWUtMg0KDQo=\n"},"kind":"Secret","metadata":{"annotations":{},"name":"dotfile-secret","namespace":"default"}}
  creationTimestamp: "2023-12-13T21:55:13Z"
  name: dotfile-secret
  namespace: default
  resourceVersion: "54021"
  uid: 1d680c61-327f-4bdd-bc81-6e922fe525e4
type: Opaque
  1. Since Kubernetes removes trailing whitespace, it seems improbable for it to store whitespace. Furthermore, attempting to store a secret with multiple base64 strings separated by new lines results in rejection by the API server, underscoring the implementation's intolerance for trailing whitespace.

Note: A secret with multiple base64 strings separated by a new line would be rejected by the API server, e.g., secret: "dmFsdWUtMg0KDQo=\n\n\ndmFsdWUtMg0KDQo=" would return the following error: The request is invalid: patch: Invalid value: "map[data:map[secret:dmFsdWUtMg0KDQo=\n\n\ndmFsdWUtMg0KDQo=] metadata:map[annotations:map[kubectl.kubernetes.io/last-applied-configuration:{\"apiVersion\":\"v1\",\"data\":{\"secret\":\"dmFsdWUtMg0KDQo=\\n\\n\\ndmFsdWUtMg0KDQo=\"},\"kind\":\"Secret\",\"metadata\":{\"annotations\":{},\"name\":\"dotfile-secret\",\"namespace\":\"default\"}}\n]]]": error decoding from json: illegal base64 data at input byte 19

Trailing whitespace is stripped by Kubernetes due to the underlying implementation of the encoding/base64 package that the serializer codec uses: Kubernetes Source Code.

Refer to this simple Go Playground example (Go Playground) that demonstrates Golang's encoding/base64 stripping trailing whitespace after decoding and re-encoding which is what is done in the Kubernetes codebase.

@rquitales rquitales enabled auto-merge (squash) December 13, 2023 23:12
@rquitales rquitales merged commit 33cff26 into master Dec 13, 2023
18 checks passed
@rquitales rquitales deleted the rquitales/fix-secret-newline branch December 13, 2023 23:15
rquitales added a commit that referenced this pull request Dec 15, 2023
…espace (#2715)

### Proposed changes

This pull request introduces a normalization mechanism for `secret.data`
base64 encoded values, specifically stripping whitespace to prevent
unintended spurious diffs.

**Changes Made:**
- Created comprehensive integration and unit tests to verify and
highlight the broken behavior associated with `secret.data` values
containing whitespaces.
- Updated the provider to incorporate the normalization process,
ensuring that all related tests pass seamlessly.

### Related issues (optional)

Fixes: #2681
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.

When a Secret contains a \n at the end it constantly tries to replace it
2 participants