feat: added regional secret support for secret-manager

[abheda-crest]

Issue: #3331

Description:

Added the support for regional secret creation/updation, deletion and fetch for secret manager service.

• Updated the autoconfigure/secretmanager to create the regional client and secretmanager module to support the regional secret operation.
• Added the optional property location in GcpSecretManagerProperties.java which will take region from application.properties file. Whenever the location is available, it will use to perform the operation on regional secret. If not provided the global stack will be served.
• Updated the documentation for this additional property in the docs/src/main/asciidoc/secretmanager.adoc
• Added the sample application for regional secret operations.
• Added/Updated the unit and integration tests.

Note: Fixed the integration test testUpdateSecrets in the file secretmanager/it/SecretManagerTemplateIntegrationTests.java

Performed the below mentioned manual unit tests to validate the working of the global and regional secret operations.

• Create secret with only secretId and payload
• Create secret with existing secretId and payload
• Create secret with secretId, payload and valid projectID
• Create secret with secretId, payload and invalid projectID (project on which user doesn't have access)
• Read an existing secret with secretId
• Read a non-existing secret with secretId
• Read a secret with secretId and existing version
• Read a secret with secretId and disabled version
• Read a secret with secretId and destroyed version
• Read a secret with secretId and non-existing version
• Read a secret with secretId and valid project
• Read a secret with secretId and invalid project (project on which user doesn't have access)
• Read a secret with secretId and existing version and valid project
• Read a secret with secretId and existing version and invalid project
• Read a secret with secretId and non-existing version and valid project
• Read a secret with secretId and non-existing version and invalid project
• Update an existing secret with only secretId and payload
• Update an existing secret with secretId, payload and valid projectID
• Update an existing secret with secretId, payload and invalid projectID (project on which user doesn't have access)
• Delete an existing secret with secretId
• Delete a non-existing secret with secretId
• Delete a secret with secretId and valid projectId
• Delete a secret with secretId and invalid projectId
• Enable an existing secret and valid version
• Enable an existing secret without version
• Enable an existing secret and invalid version
• Enable a non-existing secret
• Disable an existing secret and valid version
• Disable an existing secret without version
• Disable an existing secret and invalid version
• Disable a non-existing secret
• Check if secret exists for existing secret
• Check if secret exists for non-existing secret
• Check if secret exists for existing secret & valid projectId
• Check if secret exists for existing secret & invalid projectId
• Read secret with secretId and other project using service account
• Inject secret with the @Value annotation

More information about regional secrets: https://cloud.google.com/secret-manager/regional-secrets/data-residency

[mieseprem]

We, in my company, always try to avoid having AssertJ AND JUnit assertions mixed in one class.
At least in my team we prefere to only use AssertJ as it let you chain assertions on same object.

Not sure if there are coding guidlines in Google projects, but maybe you consider to only use one assertions library to keep the mental load low.

[abheda-crest]

I've replaced the JUnit Assertion with the AssertJ assertion.

[mieseprem]

Is it syntactically correct if there is no closing </p> tag?

Suggested change

	* <p>When not specified, the secret manager will use the Global Stack.
	* <p>When not specified, the secret manager will use the Global Stack.</p>

[abheda-crest]

I've referred other files for the docstrings and there is no use of closing </p> tag. Following are some of such files:

• https://github.com/GoogleCloudPlatform/spring-cloud-gcp/blob/main/spring-cloud-gcp-core/src/main/java/com/google/cloud/spring/core/DefaultGcpEnvironmentProvider.java#L26
• https://github.com/GoogleCloudPlatform/spring-cloud-gcp/blob/main/spring-cloud-gcp-kms/src/main/java/com/google/cloud/spring/kms/KmsOperations.java#L24
• https://github.com/GoogleCloudPlatform/spring-cloud-gcp/blob/main/spring-cloud-gcp-core/src/main/java/com/google/cloud/spring/core/Credentials.java#L33

Let me know your thoughts on this.

[mieseprem]

Oh, sorry. You are right. I wasn't aware that the closing tag is not mandatory.

[mieseprem]

First, I'm not from Google, so my opinion is truly not more than my opinion. Google may see things different 😉

Now, in assertions you normally test assumtions/expectations on an object. Having this on mind the assertions should be more in this way:

assertThat(objectToTest)
  .verificationOne(...)
  .verificationTwo(...);

The result is the same, but now it is in a more logical order:

assertThat(propertySource)
  .returns(Optional.ofNullable(location), SecretManagerPropertySource::getLocation);

You could add more verifications if suitable.

[abheda-crest]

I followed the existing convention used throughout the codebase, where assertions are written in the same order as the current pattern. However, I understand your perspective, and if the team prefers the alternative style you suggested, I’m happy to make the adjustment.

[mieseprem]

In reference to my other comment, this is how AssertJ chaining performs several checks on the same object:

assertThat(secretIdentifier)
  .returns("defaultProject", SecretVersionName::getProject)
  .returns("us-central1", SecretVersionName::getLocation)
  .returns("the-reg-secret", SecretVersionName::getSecret)
  .returns("latest", SecretVersionName::getSecretVersion);

[abheda-crest]

As mentioned in the previous comment, I opted to maintain consistency with other assertions used across the project.

[mieseprem]

As I think your PR (or the feature you're bringing) is totally cool, I've taken a look at the code in its entirety today, not just the delta shown up here in 'Files changed' view.

May I ask questions about the way you've implemented the 'location'?

The first one is, why as Optional? I like Optionals, but in this specific case, I don't see a great deal of added value.

• Similarly to projectId, the location can be unset. The projectId in the existing code is implemented as a simple String, and the location as Optional. In terms of code style (not in the sense of arrangement), I would tend to follow the direction of the rest of the code (String).
• There are no mappings (.map(...)) of the Optional type taking place. The only use case is the check for isPresent(). This could also be implemented - similarly to projectId - with an IF condition.

The other is, if the Optional type is still to be used, could the variable at least be renamed accordingly?

• In the spring-cloud-gcp library, it's common practice (although this is just a gut feeling and not backed up by evidence) to work with Strings or special objects (SecretVersionName, LocationName, ...). So, if an Optional is introduced, the variable name should somehow reflect this. Although I must admit that this is debatable. I know many voices that say technical implementation details shouldn't be included in variable names.
• One could, of course, argue that every IDE displays the return type of getLocation() as Optional<String>. However, if you look at the code without an IDE (as is the case here on GitHub), the return type is not immediately apparent and expectations are set. Providing a hint through variable names could at least be considered.

[abheda-crest]

The use of Optional here is to explicitly signal that location may or may not be present, which I believe makes the intent clearer than a plain String. While it's true we're not using .map() or chaining operations now, Optional forces consumers to consider the absence of location explicitly (rather than relying on null checks). This can help avoid future bugs or assumptions about the field always being set.

I understand your suggestion regarding variable naming, but I believe the intent is already clear from the type itself (Optional<String>), and I prefer to keep the variable name focused on the domain rather than the technical implementation detail.

Let me know your thoughts, and I'm happy to make further adjustments if needed!

[mieseprem]

Regarding the use of Optional, same thoughts here.

As in SecretManagerPropertyUtils is already a handling regarding if location is set or not:
https://github.com/abheda-crest/spring-cloud-gcp/blob/9f6bf3899938e195810b5f1cd2c585dd3be8e98d/spring-cloud-gcp-secretmanager/src/main/java/com/google/cloud/spring/secretmanager/SecretManagerPropertyUtils.java#L86-L93

Maybe you could ease things here if leaving location as String. Even in getProperty(String) is no special handling necessary (is done in SecretManagerPropertyUtils)

[abheda-crest]

The getProperty() method will still require special handling to account for different cases when retrieving the property value. Specifically, I have added an additional check for location, which is used to form the version name based on the location value. This logic will still be necessary, even if we keep the location as a String.