OpenSearch cloudwatch logging

[jmarcos135]

Changes proposed in this pull request:

• Adds self-service enablement of Audit, Error, SearchSlow, and IndexSlow logs publishing to cloudwatch for OpenSearch domains.
  • The broker provisions and manages cloudwatch log groups and then enables opensearch domains to publish logs to these groups.
  • Enabling Audit will enable FGAC on the domain in order to actually generate these logs. This is done as a convenience for customers. Since this is a permanent operation, it will be noted in the user docs.
  • SearchSlow and IndexSlow will need additional configuration of slow thresholds to generate anything meaningful, but this is a user responsibility. This is done with a PUT request to their domain using their app/domain credentials. This will be explained in the docs.

Things to check

• For any logging statements, is there any chance that they could be logging sensitive data?
• Are log statements using a logging library with a logging level set? Setting a logging level means that log statements "below" that level will not be written to the output. For example, if the logging level is set to INFO and debugging statements are written with log.debug or similar, then they won't be written to the otput, which can prevent unintentional leaks of sensitive data.

Security considerations

[Note the any security considerations here, or make note of why there are none]

[markdboyd]

Suggested change

	if !errors.As(err, &alreadyExists) {
	logger.Error("ensureLogGroups: CreateLogGroup err", "err", err, "logGroup", name)
	return err
	}
	if !errors.As(err, &alreadyExists) {
	logger.Error("ensureLogGroups: CreateLogGroup err", "err", err, "logGroup", name)
	return err
	}
	return err

So we're catching the already exists error just to log it specifically? Or do we intend to ignore that error?

I also think we need to return the error if it's not already exists, so we don't trap those errors

[jmarcos135]

The intention of this block was to catch errors that are NOT alreadyExists and return those. I'm treating alreadyExists errors as successful here. I can change the logic here to make it more clear to read.

[markdboyd]

Nit: would it be easier to have an expectedInstance for each test case and compare against that instead of comparing each property individually?

[jmarcos135]

Refactored this test file to match the pattern used in other unit tests

[markdboyd]

Suggested change

	gotCreated := append([]string{}, client.createdLogGroups...)
	wantCreated := append([]string{}, test.expectedCreated...)
	sort.Strings(gotCreated)
	sort.Strings(wantCreated)
	if diff := deep.Equal(gotCreated, wantCreated); diff != nil {
	t.Errorf("createdLogGroups: %v", diff)
	}
	sort.Strings(client.createdLogGroup)
	sort.Strings(test.expectedCreated)
	if diff := deep.Equal(client.createdLogGroup, test.expectedCreated); diff != nil {
	t.Errorf("createdLogGroups: %v", diff)
	}

It's not important, but is there a reason this form wouldn't work?

[markdboyd]

Great work @jmarcos135