-
Notifications
You must be signed in to change notification settings - Fork 257
feat: Add security-bootstrap-redis service #334
feat: Add security-bootstrap-redis service #334
Conversation
releases/nightly-build/compose-files/docker-compose-nexus-arm64.yml
Outdated
Show resolved
Hide resolved
releases/nightly-build/compose-files/source/docker-compose-nexus-add-security.yml
Show resolved
Hide resolved
bnevis-i
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every database-consuming service needs to take a dependency on security-bootstrap-database. This new dependency needs to be added into add-security.yml. Otherwise there is no forcing function by any downstream service to ensure that the bootstrap is run.
|
P.S. I've gotten into using |
| @@ -0,0 +1,552 @@ | |||
| # * Copyright 2020 Intel Corporation. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to added the file to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies. Removed and added this file to .gitignore.
@bnevis-i , good catch! |
|
Latest push addresses issues raised in comments. |
lenny-goodell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can not merge until edgex-go PR merged and image for service is being produced in nexus.
fe1c964 to
97cf0b8
Compare
|
Squashed |
|
@andresrinivasan, My |
97cf0b8 to
8cf134d
Compare
compose-builder/add-security.yml
Outdated
| database: | ||
| env_file: | ||
| - database-security.env | ||
| command: | | ||
| /bin/sh -c " | ||
| until [ -r $${REDIS5_PASSWORD_PATHNAME} ] && [ -s $${REDIS5_PASSWORD_PATHNAME} ]; do sleep 1; done | ||
| exec /usr/local/bin/docker-entrypoint.sh --requirepass `cat $${REDIS5_PASSWORD_PATHNAME}` \ | ||
| --dir /data \ | ||
| --save 900 1 \ | ||
| --save 300 10 \ | ||
| --save 60 10000 | ||
| " | ||
| volumes: | ||
| - /tmp/edgex/secrets/edgex-redis:/tmp/edgex/secrets/edgex-redis:z | ||
| depends_on: | ||
| - vault-worker |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this database section at all. Only thin left is the depends_on which doesn't seem to be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with lenny. redis-bootstrap needs to depend on database and vault-worker. This can go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
@andresrinivasan , one more rebase needed for my pin versions PR. Last one for me for a bit.... ;-) |
8cf134d to
66591ae
Compare
lenny-goodell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but can't merge until docker image is being created in Nexus.
|
@andresrinivasan , you also need to add the |
76ed8b6 to
75998a5
Compare
Done |
compose-builder/add-security.yml
Outdated
|
|
||
| security-secrets-setup: | ||
| image: ${CORE_EDGEX_REPOSITORY}/docker-edgex-secrets-setup-go${ARCH}:${CORE_EDGEX_VERSION}${DEV} | ||
| image: ${CORE_EDGEX_REPOSITORY}/docker-secrets-setup-go${ARCH}:${CORE_EDGEX_VERSION}${DEV} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost, but missing security in the name. Needs to be docker-security-secrets-setup-go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
75998a5 to
0af12d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please run make build on your latest changes.
releases/nightly-build/compose-files/docker-compose-nexus-arm64.yml
Outdated
Show resolved
Hide resolved
0af12d2 to
98b7d27
Compare
compose-builder/add-security.yml
Outdated
|
|
||
| edgex-proxy: | ||
| image: ${CORE_EDGEX_REPOSITORY}/docker-edgex-security-proxy-setup-go${ARCH}:${CORE_EDGEX_VERSION}${DEV} | ||
| image: ${CORE_EDGEX_REPOSITORY}/docker-security-security-proxy-setup-go${ARCH}:${CORE_EDGEX_VERSION}${DEV} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong name. Extra security. Change to docker-security-proxy-setup-go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Signed-off-by: André Srinivasan <[email protected]>
98b7d27 to
dda617f
Compare
Signed-off-by: André Srinivasan [email protected]
PR Checklist
Please check if your PR fulfills the following requirements:
If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/developer-scripts/blob/master/.github/Contributing.md.
What is the current behavior?
Issue Number:
Resolves #332
What is the new behavior?
Add new security-bootstrap-redis service
Does this PR introduce a breaking change?
This PR is dependent on actually adding the service to edgex-go. See edgex-go issue #2503
Specific Instructions
Other information
I'm creating this PR for the purposes of review. Please do not merge yet.