Skip to content

Commit

Permalink
Fix double-encoding of Kubernetes secrets (#6541)
Browse files Browse the repository at this point in the history
# Description

This change removes manual encoding from our interactions with
`secret.Data`, which should address the double-encoding issue.

The problem here was a wrong assumption about the way `secret.Data`
works in the Go Kubernetes client. This link documents the actual
behavior:
https://github.com/weibeld/kubernetes-client-go-examples/blob/master/README.md#ex5-secrets

I had a hint that something was wrong here while I was writing the code,
but I wrongly assumed it was a quirk of the envtest library. The right
behavior is to store the **decoded** value in `secret.Data`. Therefore
the fix is to remove all of the places we encode data before storing it.

## Type of change

- This pull request fixes a bug in Radius and has an approved issue
(issue link required).

Fixes: #6540

## Auto-generated summary

<!--
GitHub Copilot for docs will auto-generate a summary of the PR
-->

<!--
copilot:all
-->
### <samp>🤖 Generated by Copilot at 359f241</samp>

### Summary
🐛🧹🚀

<!--
1. 🐛 - This emoji represents a bug fix, which is the main purpose of the
first pull request. It shows that the test failure was resolved and the
unused import was removed.
2. 🧹 - This emoji represents a code cleanup or refactoring, which is the
main purpose of the second pull request. It shows that the code was
simplified, commented, and improved.
3. 🚀 - This emoji represents a performance improvement or optimization,
which is the main purpose of the third pull request. It shows that the
secret handling logic was made more efficient and correct.
-->
This pull request refactors and fixes the `reconciler` package, which
handles the creation and update of Kubernetes deployments and secrets
based on recipes. It removes unnecessary imports, encoding, and
complexity from the code and the tests.

> _Sing, O Muse, of the valiant code warriors who strove_
> _To mend the broken tests and reconcile the secrets_
> _That vexed the deployment function with double encoding_
> _And cluttered the package with imports that served no purpose_

### Walkthrough
* Remove unused imports of `encoding/base64` package from `reconciler`
package in `deployment_reconciler.go`, `deployment_reconciler_test.go`,
`recipe_reconciler.go`, and `recipe_reconciler_test.go`
([link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-87a7dfa06c174a6b41b671b2cfffb84c81e481881baec866a026e7dd00db8195L21),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-9d66db7eb311e875d019923b0892c9b51b9520295624cc0b6a0197cdbd04527cL20),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-565f9bf43f08d107a3218cdeb8b17eab13363d0661a65535e80ec4ef114f09e5L21),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-2b3aa3c5be32be791c9069d44213d653090aed157026f3d37f187ccded049cacL20))
* Simplify logic of `updateDeployment` and `updateSecret` functions by
removing unnecessary encoding of secret values using `base64` package in
`deployment_reconciler.go` and `recipe_reconciler.go`
([link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-87a7dfa06c174a6b41b671b2cfffb84c81e481881baec866a026e7dd00db8195L556-R558),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-565f9bf43f08d107a3218cdeb8b17eab13363d0661a65535e80ec4ef114f09e5L506-R505),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-565f9bf43f08d107a3218cdeb8b17eab13363d0661a65535e80ec4ef114f09e5L517-R515))
* Add comment to explain why `updateDeployment` function uses `Data`
field of `Secret` object instead of `StringData` field in
`deployment_reconciler.go`
([link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-87a7dfa06c174a6b41b671b2cfffb84c81e481881baec866a026e7dd00db8195R519-R520))
* Update expected secret data in tests to match new logic of
`updateDeployment` and `updateSecret` functions in
`deployment_reconciler_test.go` and `recipe_reconciler_test.go`
([link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-9d66db7eb311e875d019923b0892c9b51b9520295624cc0b6a0197cdbd04527cL369-R371),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-9d66db7eb311e875d019923b0892c9b51b9520295624cc0b6a0197cdbd04527cL415-R415),
[link](https://github.com/radius-project/radius/pull/6541/files?diff=unified&w=0#diff-2b3aa3c5be32be791c9069d44213d653090aed157026f3d37f187ccded049cacL306-R306))

Signed-off-by: willdavsmith <[email protected]>
  • Loading branch information
rynowak authored and willdavsmith committed Nov 6, 2023
1 parent 741c959 commit a8b1e09
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 19 deletions.
7 changes: 3 additions & 4 deletions pkg/controller/reconciler/deployment_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package reconciler

import (
"context"
"encoding/base64"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -517,6 +516,8 @@ func (r *DeploymentReconciler) updateDeployment(ctx context.Context, deployment
return fmt.Errorf("failed to fetch secret %s: %w", secretName, err)
}

// envtest has some quirky behavior around StringData which makes it hard to test. So we're
// using Data directly.
secret.Data = map[string][]byte{}

for name, source := range annotations.Configuration.Connections {
Expand Down Expand Up @@ -553,10 +554,8 @@ func (r *DeploymentReconciler) updateDeployment(ctx context.Context, deployment
return fmt.Errorf("failed to read values resource %s: %w", id, err)
}

// envtest has some quirky behavior around StringData which makes it hard to test. So we're
// using Data directly.
for k, v := range values {
secret.Data[k] = []byte(base64.RawStdEncoding.EncodeToString([]byte(v)))
secret.Data[k] = []byte(v)
}
}

Expand Down
13 changes: 6 additions & 7 deletions pkg/controller/reconciler/deployment_reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package reconciler

import (
"encoding/base64"
"fmt"
"testing"
"time"
Expand Down Expand Up @@ -366,10 +365,10 @@ func Test_DeploymentReconciler_Connections(t *testing.T) {
require.NoError(t, err)

expectedSecretData := map[string][]byte{
"CONNECTION_A_A-SECRET": []byte(base64.RawStdEncoding.EncodeToString([]byte("a"))),
"CONNECTION_A_A-VALUE": []byte(base64.RawStdEncoding.EncodeToString([]byte("a"))),
"CONNECTION_B_B-SECRET": []byte(base64.RawStdEncoding.EncodeToString([]byte("b"))),
"CONNECTION_B_B-VALUE": []byte(base64.RawStdEncoding.EncodeToString([]byte("b"))),
"CONNECTION_A_A-SECRET": []byte("a"),
"CONNECTION_A_A-VALUE": []byte("a"),
"CONNECTION_B_B-SECRET": []byte("b"),
"CONNECTION_B_B-VALUE": []byte("b"),
}
require.Equal(t, expectedSecretData, secret.Data)

Expand Down Expand Up @@ -412,8 +411,8 @@ func Test_DeploymentReconciler_Connections(t *testing.T) {
require.NoError(t, err)

expectedSecretData = map[string][]byte{
"CONNECTION_B_B-SECRET": []byte(base64.RawStdEncoding.EncodeToString([]byte("b"))),
"CONNECTION_B_B-VALUE": []byte(base64.RawStdEncoding.EncodeToString([]byte("b"))),
"CONNECTION_B_B-SECRET": []byte("b"),
"CONNECTION_B_B-VALUE": []byte("b"),
}
require.Equal(t, expectedSecretData, secret.Data)

Expand Down
7 changes: 2 additions & 5 deletions pkg/controller/reconciler/recipe_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package reconciler

import (
"context"
"encoding/base64"
"fmt"
"strings"
"time"
Expand Down Expand Up @@ -503,8 +502,7 @@ func (r *RecipeReconciler) updateSecret(ctx context.Context, recipe *radappiov1a
}

for k, v := range values {
encoded := base64.RawStdEncoding.EncodeToString([]byte(v))
secret.Data[k] = []byte(encoded)
secret.Data[k] = []byte(v)
}

secrets, err := r.Radius.Resources(recipe.Status.Scope, recipe.Spec.Type).ListSecrets(ctx, recipe.Name)
Expand All @@ -514,8 +512,7 @@ func (r *RecipeReconciler) updateSecret(ctx context.Context, recipe *radappiov1a
return fmt.Errorf("failed to list secrets: %w", err)
} else {
for k, v := range secrets.Value {
encoded := base64.RawStdEncoding.EncodeToString([]byte(*v))
secret.Data[k] = []byte(encoded)
secret.Data[k] = []byte(*v)
}
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/controller/reconciler/recipe_reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package reconciler

import (
"encoding/base64"
"errors"
"testing"
"time"
Expand Down Expand Up @@ -303,8 +302,8 @@ func Test_RecipeReconciler_WithSecret(t *testing.T) {
require.NoError(t, err)

expectedData := map[string][]byte{
"a-value": []byte(base64.RawStdEncoding.EncodeToString([]byte("a"))),
"b-secret": []byte(base64.RawStdEncoding.EncodeToString([]byte("b"))),
"a-value": []byte("a"),
"b-secret": []byte("b"),
}

require.Equal(t, expectedData, secret.Data)
Expand Down

0 comments on commit a8b1e09

Please sign in to comment.