Introduce the `aws_s3` BundlePublisher plugin #4355

amartinezfayo · 2023-07-24T00:33:41Z

This PR introduces the aws_s3 BundlePublisher plugin, that puts the current trust bundle of the server in a designated
Amazon S3 bucket, keeping it updated.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

EItanya · 2023-07-26T16:36:15Z

conf/server/server_full.conf

+    #         # object_key: The object key inside the bucket. Default: "".
+    #         # object_key = "example.org"
+
+    #         # format: Format in which the trust bundle is stored, <spiffe | jwks | pem>. Default: "".


What format is defaulted to when unset?

The plugin requires that a format is explicitly configured, so there is not a default format set.

Do you think a default would make sense here, pem for instance since it's the most well understood format?

If we have a default, I would probably suggest to be the spiffe format. PEM does not allow the encoding of JWT authorities, which may be needed. Also, agents can be configured to use spiffe format for the initial SPIRE Server trust bundle.
The idea behind requiring a value here is to avoid the inadvertently misconfiguration of the plugin, but we can revisit that decision.

The idea behind requiring a value here is to avoid the inadvertently misconfiguration of the plugin

No, that's ok. I think this convinced me to be honest.

EItanya · 2023-07-26T16:40:11Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	config    *Config
+	configMtx sync.RWMutex


Please excuse my ignorance, but how is the config updated dynamically in a way that would require a RWLock?

The configuration can be updated at any time by a different thread if the Configure function is called.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY

thanks Agustin!! this looks great!

MarcosDY · 2023-07-26T19:23:20Z

conf/server/server_full.conf

+    # BundlePublisher "aws_s3" {
+    #     plugin_data {
+    #         # region: AWS region to store the trust bundle. Default: "".
+    #         # region = "us-east-1"


in another examples we have region empty

MarcosDY · 2023-07-27T12:57:36Z

doc/plugin_server_bundlepublisher_aws_s3.md

+| region            | AWS region to store the trust bundle.                                                                                                                          |                                                     |
+| bucket            | The Amazon S3 bucket name to which the trust bundle is uploaded.                                                                                               |                                                     |
+| object_key        | The object key inside the bucket.                                                                                                                              |                                                     |
+| format            | Format in which the trust bundle is stored, &lt;spiffe &vert; jwks &vert; pem&gt;. See [Supported bundle formats](#supported-bundle-formats) for more details. |                                                     |


may we add a (required) here?

MarcosDY · 2023-07-27T13:32:28Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	log      hclog.Logger
+}
+
+func (p *Plugin) SetLogger(log hclog.Logger) {


nit: since another public functions has a comment, can you add a comment here too?

MarcosDY · 2023-07-27T13:36:39Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	return &configv1.ConfigureResponse{}, nil
+}
+
+// PublishBundle puts the bundle in the specified S3 bucket and key.


what is that and key? may we reword this to make sure readers understand that the key is for the published bucket object?

The key refers to the object key of the configured bucket name. I'll update the comment.

MarcosDY · 2023-07-27T13:38:50Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	currentBundle := p.getBundle()
+	if proto.Equal(req.Bundle, currentBundle) {
+		// Bundle not changed. No need to publish.
+		return &bundlepublisherv1.PublishBundleResponse{}, nil


is it possible that the bundle does not changed, but someone changed the format? (we are not reloading configs yet but maybe in a future)

That's an interesting scenario, good catch. I'll clean the current bundle when Configure() is called, to make sure that the content is refreshed.

MarcosDY · 2023-07-27T18:29:59Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+func parseAndValidateConfig(c string) (*Config, error) {
+	config := new(Config)
+
+	if err := hcl.Decode(config, c); err != nil {


is not region a required field?

Yes! Thanks, I'll add that check.

MarcosDY · 2023-07-27T19:33:31Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+			if tt.config != nil {
+				plugintest.Load(t, builtin(p), nil, options...)
+				require.NoError(t, err)
+				p.s3Client.(*fakeClient).expectBucket = aws.String(tt.config.Bucket)


may we use newClient directly?

newClient is a function. I'll reduce the type assertions and check that succeeded.

MarcosDY · 2023-07-27T19:55:04Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+}
+
+func testMultiplePuts(t *testing.T, p *Plugin) {
+	// Test multiple put operations, and check that onlt a call to PutObject is


Suggested change

// Test multiple put operations, and check that onlt a call to PutObject is

// Test multiple put operations, and check that onlt a call to PutObject is

Suggested change

// Test multiple put operations, and check that onlt a call to PutObject is

// Test multiple put operations, and check that only a call to PutObject is

MarcosDY · 2023-07-27T20:00:25Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+			require.NotNil(t, resp)
+
+			if tt.testMultiplePuts {
+				testMultiplePuts(t, p)


it looks to me, like a good option to have another unit test, not a test case inside this functions

MarcosDY · 2023-07-27T20:08:33Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+
+func getTestBundle(t *testing.T) *types.Bundle {
+	const (
+		certPEM = `-----BEGIN CERTIFICATE-----


nit: may we use test fixture here?

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY · 2023-08-01T13:28:10Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+		{
+			name:   "multiple times",
+			bundle: testBundle,
+			config: &Config{
+				AccessKeyID:     "access-key-id",
+				SecretAccessKey: "secret-access-key",
+				Region:          "region",
+				Bucket:          "bucket",
+				ObjectKey:       "object-key",
+				Format:          "spiffe",
+			},
+			testMultiplePuts: true,
+		},


looks like this test case must be removed? since testMutiplePuts is never used

Forgot to remove it, thanks!

MarcosDY · 2023-08-01T13:28:55Z

pkg/server/plugin/bundlepublisher/awss3/awss3_test.go

+	return &s3.PutObjectOutput{}, nil
+}
+
+func TestPublishMultiple(t *testing.T) {


can you move this test before fakeClient declaration?

MarcosDY · 2023-08-01T13:36:14Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	if err != nil {
+		return nil, status.Errorf(codes.Internal, "failed to create client configuration: %v", err)
+	}
+	s3Client, err := p.hooks.newS3ClientFunc(awsCfg)


looks like env variables are never readed? can you add a test case to validate that we are able to use AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESSKEY ?

We have pretty much the same situation in all the plugins where we rely on the AWS SDK to use the creds from the env vars if they are available, so it looks like this would be a test of the AWS SDK itself rather than a test of this plugin.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron · 2023-08-01T16:34:05Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+
+// Configure configures the plugin.
+func (p *Plugin) Configure(ctx context.Context, req *configv1.ConfigureRequest) (*configv1.ConfigureResponse, error) {
+	if req == nil {


nit: this check isn't practical since a nil request here would imply the gRPC library is broken

azdagron · 2023-08-01T16:34:52Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+		return nil, err
+	}
+
+	if req == nil {


nit: same thing here. gRPC will always pass a request.

azdagron · 2023-08-01T16:35:40Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+		return &bundlepublisherv1.PublishBundleResponse{}, nil
+	}
+
+	bundle := bundleformat.NewFormatter(req.Bundle)


nit:

Suggested change

bundle := bundleformat.NewFormatter(req.Bundle)

formatter := bundleformat.NewFormatter(req.Bundle)

azdagron · 2023-08-01T16:38:15Z

pkg/server/plugin/bundlepublisher/awss3/awss3.go

+	if err != nil {
+		return nil, status.Errorf(codes.InvalidArgument, "could not parse bundle format from configuration: %v", err)
+	}
+	// The bundleformat package may support formats that this plugin do not


Suggested change

// The bundleformat package may support formats that this plugin do not

// The bundleformat package may support formats that this plugin does not

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

achaurasiaConfluent · 2023-08-01T22:41:29Z

Thank you for doing this, we were looking at implemented this plugin as well.
Can't wait for his plugin to be released.

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY

LGTM!

Introduce the aws_s3 BundlePublisher plugin

7c8b71f

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

amartinezfayo requested review from evan2645, azdagron, MarcosDY and rturner3 as code owners July 24, 2023 00:33

amartinezfayo assigned MarcosDY Jul 25, 2023

EItanya reviewed Jul 26, 2023

View reviewed changes

Update description in server_full.conf

b034f24

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY reviewed Jul 27, 2023

View reviewed changes

amartinezfayo added 2 commits July 31, 2023 18:15

Address PR comments

5a47907

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Merge remote-tracking branch 'origin' into bundlepublisher-awss3

fa3d2bf

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY reviewed Aug 1, 2023

View reviewed changes

Address PR comments

eb75512

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

azdagron reviewed Aug 1, 2023

View reviewed changes

Address PR comments

07423a0

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

Merge remote-tracking branch 'origin' into bundlepublisher-awss3

3ade895

Signed-off-by: Agustín Martínez Fayó <amartinezfayo@gmail.com>

MarcosDY approved these changes Aug 2, 2023

View reviewed changes

amartinezfayo merged commit 867a000 into spiffe:main Aug 2, 2023

amartinezfayo added this to the 1.7.2 milestone Aug 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce the `aws_s3` BundlePublisher plugin #4355

Introduce the `aws_s3` BundlePublisher plugin #4355

amartinezfayo commented Jul 24, 2023 •

edited

Loading

EItanya Jul 26, 2023

amartinezfayo Jul 26, 2023

EItanya Jul 26, 2023

amartinezfayo Jul 26, 2023

EItanya Jul 27, 2023

EItanya Jul 26, 2023

amartinezfayo Jul 26, 2023

MarcosDY left a comment

MarcosDY Jul 26, 2023

MarcosDY Jul 27, 2023

MarcosDY Jul 27, 2023

MarcosDY Jul 27, 2023 •

edited

Loading

amartinezfayo Jul 28, 2023

MarcosDY Jul 27, 2023

amartinezfayo Jul 28, 2023

MarcosDY Jul 27, 2023

amartinezfayo Jul 28, 2023

MarcosDY Jul 27, 2023

amartinezfayo Jul 31, 2023

MarcosDY Jul 27, 2023

MarcosDY Jul 27, 2023

MarcosDY Jul 27, 2023

MarcosDY Aug 1, 2023

amartinezfayo Aug 1, 2023

MarcosDY Aug 1, 2023

MarcosDY Aug 1, 2023

amartinezfayo Aug 1, 2023

azdagron Aug 1, 2023

azdagron Aug 1, 2023

azdagron Aug 1, 2023

azdagron Aug 1, 2023

achaurasiaConfluent commented Aug 1, 2023

MarcosDY left a comment

	// Test multiple put operations, and check that onlt a call to PutObject is
	// Test multiple put operations, and check that onlt a call to PutObject is

	bundle := bundleformat.NewFormatter(req.Bundle)
	formatter := bundleformat.NewFormatter(req.Bundle)

	// The bundleformat package may support formats that this plugin do not
	// The bundleformat package may support formats that this plugin does not

Introduce the aws_s3 BundlePublisher plugin #4355

Introduce the aws_s3 BundlePublisher plugin #4355

Conversation

amartinezfayo commented Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcosDY Jul 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achaurasiaConfluent commented Aug 1, 2023

MarcosDY left a comment

Choose a reason for hiding this comment

Introduce the `aws_s3` BundlePublisher plugin #4355

Introduce the `aws_s3` BundlePublisher plugin #4355

amartinezfayo commented Jul 24, 2023 •

edited

Loading

MarcosDY Jul 27, 2023 •

edited

Loading