Skip to content

Add support for secrets in package configurations#8558

Merged
kritika-singh3 merged 5 commits intogocd:masterfrom
kritika-singh3:add-secrets-support-package-material
Sep 18, 2020
Merged

Add support for secrets in package configurations#8558
kritika-singh3 merged 5 commits intogocd:masterfrom
kritika-singh3:add-secrets-support-package-material

Conversation

@kritika-singh3
Copy link
Contributor

@kritika-singh3 kritika-singh3 commented Sep 15, 2020

Description: Added support to specify secrets in package configurations meaning both in package repository and the package.

  • the rules will be evaluated and resolved during
    • check connection call
    • while saving package configurations
    • while checking for material update (if the package is used as a material in any pipeline)
  • users can specify the rules for a secret config using package_repository as entity
  • the secrets in packages will be validated based upon the rules for the package repository.

Copy link
Contributor

@maheshp maheshp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks ok to me, I will test it before merging once the changes are made.

return this.getRepository().hasSecretParams()
|| this.getConfiguration()
.stream()
.anyMatch(ConfigurationProperty::hasSecretParams);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for hasSecretParams and getSecretParams is repeated across classes. Is it ok to move it to Configuration class?

}

protected Map<CaseInsensitiveString, StringBuilder> validate(SecretParams secretParams, Class<? extends Validatable> entityClass, String entityName, String entityNameOrErrorMessagePrefix) {
HashMap<CaseInsensitiveString, StringBuilder> pipelinesWithErrors = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map<CaseInsensitiveString, StringBuilder> errors = new HashMap<>();

@kritika-singh3 kritika-singh3 force-pushed the add-secrets-support-package-material branch 2 times, most recently from 60ef3c9 to 8690aee Compare September 18, 2020 04:55
@maheshp
Copy link
Contributor

maheshp commented Sep 18, 2020

@kritika-singh3 came across couple of issues,

  1. Secrets for package materials are not resolved before agent assignment, hence it fails.
  2. While configuring packages, any errors during secret resolution returns a 500 and appropriate errors are not shown to the user.

@kritika-singh3 kritika-singh3 force-pushed the add-secrets-support-package-material branch from 8690aee to 9594f44 Compare September 18, 2020 08:56
@kritika-singh3
Copy link
Contributor Author

@maheshp Pushed the fix for the scenarios you mentioned. 👍

 - Validate secrets in package materials wrt to rules for `package_repository`
 - resolve rules for mdu and before sending the values to the plugin
 - check connection of package repo and package def
 - isValid call for package repo and package def
@kritika-singh3 kritika-singh3 force-pushed the add-secrets-support-package-material branch from 9594f44 to 7d81c34 Compare September 18, 2020 09:42
if (e instanceof GoConfigInvalidException && !result.hasMessage()) {
result.unprocessableEntity(entityConfigValidationFailed(packageDeinition.getClass().getAnnotation(ConfigTag.class).value(), packageDeinition.getId(), e.getMessage()));
} else if (e instanceof RulesViolationException || e instanceof SecretResolutionFailureException) {
LOGGER.error(e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't log this error.

if (e instanceof GoConfigInvalidException && !result.hasMessage()) {
result.unprocessableEntity(entityConfigValidationFailed(repository.getClass().getAnnotation(ConfigTag.class).value(), repository.getId(), e.getMessage()));
} else if (e instanceof RulesViolationException || e instanceof SecretResolutionFailureException) {
LOGGER.error(e.getMessage(), e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't log this error.

Resolve secrets for package material as part of build work
For Package Repo And Def operations, check if RulesViolationException or Secret resolution fails is raised while saving. If yes, then return 422 else 500
 - Same for check connection call as well
Set error message on the modal itself for package operations
@kritika-singh3 kritika-singh3 force-pushed the add-secrets-support-package-material branch from 7d81c34 to c7651f8 Compare September 18, 2020 10:34
@kritika-singh3 kritika-singh3 merged commit 5224cab into gocd:master Sep 18, 2020
@kritika-singh3 kritika-singh3 deleted the add-secrets-support-package-material branch September 18, 2020 11:30
@maheshp
Copy link
Contributor

maheshp commented Sep 22, 2020

Verified on GoCD Version: 20.8.0 (12211-5d6b4a7a7145f728857afb8108b31effbbd1cd0c)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants