ecs_ecr - Add encryption_configuration option #1623

rwha · 2022-12-20T18:00:18Z

SUMMARY

Adds an encryption_configuration option for new repositories to allow specifying a KMS key. Fixes #1203.

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

ecs_ecr

ADDITIONAL INFORMATION

github-actions · 2022-12-20T18:05:29Z

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

softwarefactory-project-zuul · 2022-12-20T18:19:22Z

Build failed.

ansibullbot · 2022-12-20T18:28:13Z

cc @jillr @leedm777 @markuman @s-hertel @tremble
click here for bot help

softwarefactory-project-zuul · 2022-12-20T18:37:28Z

Build succeeded.

plugins/modules/ecs_ecr.py

markuman · 2022-12-20T19:49:20Z

@rwha thanks for your first contribution.
Can you also please expand the integration test tests/integration/targets/ecs_ecr/?

softwarefactory-project-zuul · 2022-12-21T16:52:42Z

Build succeeded.

rwha · 2022-12-21T17:30:44Z

I have something written to check for modifications to the encryption config, but I'm unsure of how to handle an unsupported change.

If the encryption configuration for an existing repository is different from what is defined in the task, should the task fail, or should a warning be emitted? I would prefer a failure, however showing a warning was mentioned in #1203. If a failure is preferred, should the code at that point do something like this?

result['msg'] = 'Repository encryption configuration cannot be modified.'
return False, result

markuman · 2022-12-21T17:36:01Z

If the encryption configuration for an existing repository is different from what is defined in the task, should the task fail, or should a warning be emitted? I would prefer a failure,

I'll prefer a warning instead of a complete interruption.
Imo it must use also module.warn( ... )

But let's wait was others are expecting/prefer.
cc @alinabuzachis @jatorcasso @jillr @tremble

TL;DR: Once the ECR is created, the encryption mode is not changeable. What should the module do when the requested encryption differs from existing encryption? Fail or warn?

tremble · 2022-12-22T08:53:21Z

TL;DR: Once the ECR is created, the encryption mode is not changeable. What should the module do when the requested encryption differs from existing encryption? Fail or warn?

I would lean towards fail rather than warn especially with something security related: When Ansible's running within a tool like Ansible ~~Tower~~ Automation Platform, the warnings may be completely missed unless someone goes explicitly looking for them. While it seems minor, with the same encryption algorithms in use, the difference between who controls the encryption keys, and what keys are reused for what purposes may be important to folks.

plugins/modules/ecs_ecr.py

softwarefactory-project-zuul · 2022-12-22T15:58:48Z

Build failed.

softwarefactory-project-zuul · 2022-12-22T16:42:54Z

Build failed.

Arguments with a default should not be marked as required

softwarefactory-project-zuul · 2022-12-22T17:15:03Z

Build succeeded.

tremble

I'm not overly familiar with ECR, however in general things look good

tests/integration/targets/ecs_ecr/tasks/main.yml

…g resources

softwarefactory-project-zuul · 2023-01-18T15:40:37Z

Build succeeded.

softwarefactory-project-zuul · 2023-01-18T19:49:38Z

Build succeeded (gate pipeline).

patchback · 2023-01-18T19:49:43Z

Backport to stable-5: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-5/efbe85034d64bf0f48e128683f06272fdfc54911/pr-1623

Backported as #1661

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

ecs_ecr - Add encryption_configuration option SUMMARY Adds an encryption_configuration option for new repositories to allow specifying a KMS key. Fixes #1203. ISSUE TYPE Feature Pull Request COMPONENT NAME ecs_ecr ADDITIONAL INFORMATION Reviewed-by: Markus Bergholz <[email protected]> Reviewed-by: Mark Chappell <None> Reviewed-by: Alina Buzachis <None> (cherry picked from commit efbe850)

[PR #1623/efbe8503 backport][stable-5] ecs_ecr - Add encryption_configuration option This is a backport of PR #1623 as merged into main (efbe850). SUMMARY Adds an encryption_configuration option for new repositories to allow specifying a KMS key. Fixes #1203. ISSUE TYPE Feature Pull Request COMPONENT NAME ecs_ecr ADDITIONAL INFORMATION Reviewed-by: Markus Bergholz <[email protected]>

Add encryption_config option

2ae5cc1

Add changelog fragment

95a9be9

ansibullbot added community_review feature This issue/PR relates to a feature request module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) labels Dec 20, 2022

rwha changed the title ~~Add encryption_config option~~ Add encryption_configuration option Dec 20, 2022

markuman reviewed Dec 20, 2022

View reviewed changes

plugins/modules/ecs_ecr.py Show resolved Hide resolved

markuman reviewed Dec 20, 2022

View reviewed changes

plugins/modules/ecs_ecr.py Outdated Show resolved Hide resolved

rwha added 2 commits December 21, 2022 11:27

Add version_added info

c0b0d37

Add integration tests

67aab4d

ansibullbot added integration tests/integration tests tests and removed needs_triage labels Dec 21, 2022

markuman added the backport-5 PR should be backported to the stable-5 branch label Dec 21, 2022

tremble changed the title ~~Add encryption_configuration option~~ ecs_ecr - Add encryption_configuration option Dec 22, 2022

ansibullbot removed the new_contributor Help guide this first time contributor label Dec 22, 2022

tremble requested changes Dec 22, 2022

View reviewed changes

plugins/modules/ecs_ecr.py Outdated Show resolved Hide resolved

plugins/modules/ecs_ecr.py Outdated Show resolved Hide resolved

Use snake case instead of camel case

3c01595

rwha added 3 commits December 22, 2022 10:36

Update argument spec

a812103

Fail when attempting to modify

7ff6ad3

Update integration tests

ebbaae2

Update arg spec and documentation block

77e1059

Fix validation test

c36bfd7

Arguments with a default should not be marked as required

rwha requested a review from tremble December 22, 2022 17:22

markuman approved these changes Jan 12, 2023

View reviewed changes

markuman requested review from tremble and alinabuzachis and removed request for tremble January 12, 2023 20:07

alinabuzachis approved these changes Jan 18, 2023

View reviewed changes

tremble approved these changes Jan 18, 2023

View reviewed changes

tests/integration/targets/ecs_ecr/tasks/main.yml Outdated Show resolved Hide resolved

tests/integration/targets/ecs_ecr/tasks/main.yml Outdated Show resolved Hide resolved

Use ECR in the name of the KMS CMK - makes it easier to trace danglin…

28df6d6

…g resources

markuman added the mergeit Merge the PR (SoftwareFactory) label Jan 18, 2023

softwarefactory-project-zuul bot merged commit efbe850 into ansible-collections:main Jan 18, 2023

patchback bot mentioned this pull request Jan 18, 2023

[PR #1623/efbe8503 backport][stable-5] ecs_ecr - Add encryption_configuration option #1661

Merged

rwha deleted the ecs_ecr/add-encryption-config branch January 18, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ecs_ecr - Add encryption_configuration option #1623

ecs_ecr - Add encryption_configuration option #1623

rwha commented Dec 20, 2022

github-actions bot commented Dec 20, 2022 •

edited

Loading

softwarefactory-project-zuul bot commented Dec 20, 2022

ansibullbot commented Dec 20, 2022

softwarefactory-project-zuul bot commented Dec 20, 2022

markuman commented Dec 20, 2022

softwarefactory-project-zuul bot commented Dec 21, 2022

rwha commented Dec 21, 2022

markuman commented Dec 21, 2022

tremble commented Dec 22, 2022 •

edited

Loading

softwarefactory-project-zuul bot commented Dec 22, 2022

softwarefactory-project-zuul bot commented Dec 22, 2022

softwarefactory-project-zuul bot commented Dec 22, 2022

tremble left a comment

softwarefactory-project-zuul bot commented Jan 18, 2023

softwarefactory-project-zuul bot commented Jan 18, 2023

patchback bot commented Jan 18, 2023 •

edited

Loading

ecs_ecr - Add encryption_configuration option #1623

ecs_ecr - Add encryption_configuration option #1623

Conversation

rwha commented Dec 20, 2022

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

github-actions bot commented Dec 20, 2022 • edited Loading

Docs Build 📝

softwarefactory-project-zuul bot commented Dec 20, 2022

ansibullbot commented Dec 20, 2022

softwarefactory-project-zuul bot commented Dec 20, 2022

markuman commented Dec 20, 2022

softwarefactory-project-zuul bot commented Dec 21, 2022

rwha commented Dec 21, 2022

markuman commented Dec 21, 2022

tremble commented Dec 22, 2022 • edited Loading

softwarefactory-project-zuul bot commented Dec 22, 2022

softwarefactory-project-zuul bot commented Dec 22, 2022

softwarefactory-project-zuul bot commented Dec 22, 2022

tremble left a comment

Choose a reason for hiding this comment

softwarefactory-project-zuul bot commented Jan 18, 2023

softwarefactory-project-zuul bot commented Jan 18, 2023

patchback bot commented Jan 18, 2023 • edited Loading

Backport to stable-5: 💚 backport PR created

github-actions bot commented Dec 20, 2022 •

edited

Loading

tremble commented Dec 22, 2022 •

edited

Loading

patchback bot commented Jan 18, 2023 •

edited

Loading