Update bundle commands to allow JWKS format #1777
Conversation
set` commands that allows `pem` and `jwks` formats for those commands. - Add a deprecation notice for the `experimental bundle list`, `experimental bundle show` and `experimental bundle set` commands. Fixes spiffe#1743. Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
amartinezfayo
requested review from
APTy,
azdagron,
evan2645 and
mcpherrinm
as code owners
| -----END CERTIFICATE----- | ||
| ` | ||
|
|
||
| allBundlesJWKS = `**************************************** |
There was a problem hiding this comment.
This is the expected format.
| } | ||
| ` | ||
|
|
||
| allBundlesPEM = `**************************************** |
There was a problem hiding this comment.
This is the expected format.
| key1Pkix: key1Pkix, | ||
| ds: ds, | ||
| registrationClient: fakeregistrationclient.New(t, "spiffe://example.test", ds, nil), | ||
| stdin: stdin, |
There was a problem hiding this comment.
may we assing testEnv instead? and get stdin/out/err from there?
| s.Require().Equal(`Usage of bundle show: | ||
| func TestShowHelp(t *testing.T) { | ||
| test := setupTest(t) | ||
| defer test.cleanup() |
There was a problem hiding this comment.
nit: now that we use golang 1.14 you can use t.Cleanup instead, and it can go inside setupTest(t)
| expectedOut: cert1JWKS, | ||
| }, | ||
| } { | ||
| test := setupTest(t) |
There was a problem hiding this comment.
nit: is there a reason why it is not inside run body? (easier to read that way)
or maybe it can go out for? looks like it is always the same
There was a problem hiding this comment.
no good reason, I'll change that
| s.Require().Len(resp.Bundle.RootCas, 1) | ||
| s.Require().Equal(s.cert1.Raw, resp.Bundle.RootCas[0].DerBytes) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, resp) |
There was a problem hiding this comment.
can not you create a response and use assert proto equal instead?
| } | ||
| } | ||
|
|
||
| if strings.ToLower(format) == formatPEM { |
There was a problem hiding this comment.
to avoid lower you can return always lowered string from validate
cmd/spire-server/cli/bundle/set.go
Outdated
| } | ||
|
|
||
| bundle := ®istration.FederatedBundle{ | ||
| Bundle: bundleutil.BundleProtoFromRootCAs(id, rootCAs), | ||
| if strings.ToLower(format) == formatPEM { |
There was a problem hiding this comment.
maybe a switch with allowed types make it easier to read
| return err | ||
| } | ||
|
|
||
| if header { |
There was a problem hiding this comment.
This function is only used by the bundle show command, so it shouldn't really be part of this common file. I'll move it to show.go and remove the header parameter.
| **************************************** | ||
| ` | ||
| formatPEM = "pem" | ||
| formatJWKS = "jwks" |
There was a problem hiding this comment.
What do you think about "spiffe" for this format instead of JWKS?
Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>
| s.stdin = new(bytes.Buffer) | ||
| s.stdout = new(bytes.Buffer) | ||
| s.stderr = new(bytes.Buffer) |
There was a problem hiding this comment.
nit: the advantage of this was to not have type assertions everywhere you want to check the contents of the buffer...
There was a problem hiding this comment.
yea, I thought about that, but I was ok with either way
Pull Request check list
Affected functionality
bundleandexperimental bundlecommands.Description of change
formatflag for thebundle list,bundle showandbundle setcommands that allowspemandjwksformats for those commands.experimental bundle list,experimental bundle showandexperimental bundle setcommands.Which issue this PR fixes
Fixes #1743.