fix: fixed security-bootstrapper Docker volume init semantics #4085

vli11 · 2022-07-07T18:16:08Z

fixes: #3851
Signed-off-by: Valina Li valina.li@intel.com

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
I am not introducing a new dependency (add notes below if you are)
I have added unit tests for the new feature or bug fix (if not, why?) NA; only Dockerfile, entrypoint.sh changed
I have fully tested (add details below) this the new feature or bug fix (if not, why?)
I have opened a PR for the related docs change (if not, why?) NA; because it is fix for a WA

Testing Instructions

New Dependency Instructions (If applicable)

lenny-goodell · 2022-07-07T18:21:59Z

@vli11 , please complete the PR checklist with a why comment for those items not checked. Can simply be N/A because ...

lenny-goodell · 2022-07-07T18:24:00Z

cmd/security-bootstrapper/Dockerfile


 RUN apk add --update --no-cache dumb-init su-exec

-ENV SECURITY_INIT_DIR /edgex-init
-ARG BOOTSTRAP_REDIS_DIR=${SECURITY_INIT_DIR}/bootstrap-redis
+ENV SECURITY_INIT_STAGING_DIR /edgex-init-staging


@bnevis-i , @jim-wang-intel , Isn't this a breaking change? i.e. the ENV name change now requires end user to change the name?? Also the change to /edgex-init-staging is breaking.

End users/adopters will have existing compose files working with the previous release and should just be able to change the version of images to bump to the next release.

good point, @lenny-intel , i guess we could still keep the original ENV SECURITY_INIT_DIR untouched. but we are adding a staging directory so that we can remove the work-around in Kubernetes' world. cc: @vli11

Yep, that is probably the better approach, i.e. support both old and new way and then in 3.0 remove old way.

SECURITY_INIT_DIR was only ever used locally, so I don't think it will break anything to change it. But I have other problems with the PR that I will detail. However, I think it should it should be left as it was and add the new one.

vli11 · 2022-07-07T18:33:28Z

I have not added unit tests for this bug fix because NA; only Dockerfile, entrypoint.sh changed

vli11 · 2022-07-07T18:35:03Z

I have NOT opened a PR for the related docs change because it is fix for a WA

lenny-goodell · 2022-07-07T19:12:15Z

@vli11 , looking for you to edit the PR description and add those comment where it says (if not, why?)

example:

I have added unit tests for the new feature or bug fix (if not, why?) NA; only Dockerfile, entrypoint.sh changed

vli11 · 2022-07-07T19:48:21Z

@vli11 , looking for you to edit the PR description and add those comment where it says (if not, why?)

example:
* [ ]  I have added unit tests for the new feature or bug fix (if not, why?) **NA; only Dockerfile, entrypoint.sh changed**

DONE

bnevis-i · 2022-07-07T20:13:58Z

cmd/security-bootstrapper/Dockerfile


 # Expose the file directory as a volume since there's long-running state
-VOLUME ${SECURITY_INIT_DIR}
+VOLUME ${SECURITY_INIT_STAGING_DIR}


Remove VOLUME line. No longer needed

bnevis-i · 2022-07-07T20:28:29Z

cmd/security-bootstrapper/Dockerfile


 RUN apk add --update --no-cache dumb-init su-exec

-ENV SECURITY_INIT_DIR /edgex-init
-ARG BOOTSTRAP_REDIS_DIR=${SECURITY_INIT_DIR}/bootstrap-redis
+ENV SECURITY_INIT_STAGING_DIR /edgex-init-staging


SECURITY_INIT_DIR was only ever used locally, so I don't think it will break anything to change it. But I have other problems with the PR that I will detail. However, I think it should it should be left as it was and add the new one.

bnevis-i · 2022-07-07T20:32:20Z

cmd/security-bootstrapper/Dockerfile

+RUN mkdir -p ${SECURITY_INIT_STAGING_DIR} \
+    && mkdir -p /edgex-init \


mkdir -p ${BOOTSTRAP_REDIS_DIR} is the only thing needed; it will create any parent directories as needed; remove the others.

bnevis-i · 2022-07-07T20:32:38Z

cmd/security-bootstrapper/Dockerfile

    && mkdir -p ${BOOTSTRAP_REDIS_DIR}

-WORKDIR ${SECURITY_INIT_DIR}
+WORKDIR ${SECURITY_INIT_STAGING_DIR}


bnevis-i · 2022-07-07T20:32:49Z

cmd/security-bootstrapper/Dockerfile

+COPY --from=builder /edgex-go/cmd/security-bootstrapper/entrypoint-scripts/ ${SECURITY_INIT_STAGING_DIR}/
+RUN chmod +x ${SECURITY_INIT_STAGING_DIR}/*.sh


bnevis-i · 2022-07-07T20:35:00Z

cmd/security-bootstrapper/Dockerfile

@@ -59,10 +60,10 @@ COPY --from=builder /edgex-go/cmd/security-bootstrapper/res/configuration.toml .
 COPY --from=builder /edgex-go/cmd/security-bootstrapper/res-bootstrap-redis/configuration.toml ${BOOTSTRAP_REDIS_DIR}/res/

 # copy Consul ACL related configs
-COPY --from=builder /edgex-go/cmd/security-bootstrapper/consul-acl/ ${SECURITY_INIT_DIR}/consul-bootstrapper/
+COPY --from=builder /edgex-go/cmd/security-bootstrapper/consul-acl/ ${SECURITY_INIT_STAGING_DIR}/consul-bootstrapper/


bnevis-i · 2022-07-07T20:36:18Z

cmd/security-bootstrapper/entrypoint.sh

@@ -37,6 +37,7 @@ EDGEX_USER_ID=${EDGEX_USER:-$DEFAULT_EDGEX_USER_ID}
 # which then injecting into all other related containers on other services' entrypoint scripts
 # if the executable is not 'security-bootstrapper'; then we consider it not running the bootstrapping process
 # for the user may just want to debug into the container shell itself
+cp -rpd /edgex-init-staging/* /edgex-init/


This cp should use the env vars defined in the dockerfile. Copy from STAGING_DIR to INIT_DIR.

vli11 · 2022-07-07T20:54:19Z

@lenny-intel @jim-wang-intel all are updated, please review again

jim-wang-intel · 2022-07-07T21:15:05Z

@vli11 i think you lost your Dockerfile changes.... please do not squash the commits before others finish your change reviews...

fixes: edgexfoundry#3851 Signed-off-by: Valina Li <valina.li@intel.com>

bnevis-i

LGTM.

cmd/security-bootstrapper/entrypoint.sh

Signed-off-by: Valina Li <valina.li@intel.com>

sonarcloud · 2022-07-07T23:41:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

jim-wang-intel

LGTM

lenny-goodell

LGTM, but deferring to @bnevis-i & @jim-wang-intel for approval.

bnevis-i · 2022-07-08T02:33:44Z

LGTM, but deferring to @bnevis-i & @jim-wang-intel for approval.

@lenny-intel can you dismiss your review?

addressed

jim-wang-intel · 2022-07-11T21:00:29Z

recheck

jim-wang-intel · 2022-07-11T21:04:01Z

Hi @ernestojeda how do we do a ci/cd re-build? .. want to re-run the build to see if this is just intermittent issue

ernestojeda · 2022-07-11T21:40:47Z

recheck

ernestojeda · 2022-07-11T21:41:37Z

Hi @ernestojeda how do we do a ci/cd re-build? .. want to re-run the build to see if this is just intermittent issue

"recheck" is correct. However, you may not have the correct permissions, so Jenkins might have ignored.

jim-wang-intel · 2022-07-11T21:49:25Z

Hi @ernestojeda how do we do a ci/cd re-build? .. want to re-run the build to see if this is just intermittent issue

"recheck" is correct. However, you may not have the correct permissions, so Jenkins might have ignored.

weird, it used to be working for me though...

jim-wang-intel requested review from bnevis-i, lenny-goodell and cloudxxx8 July 7, 2022 18:19

jim-wang-intel assigned vli11 Jul 7, 2022

jim-wang-intel added bug Something isn't working security-services labels Jul 7, 2022

jim-wang-intel added tech-debt issue_type denoting refactoring to improve design or removal of temporary workarounds and removed bug Something isn't working labels Jul 7, 2022

lenny-goodell previously requested changes Jul 7, 2022

View reviewed changes

bnevis-i requested changes Jul 7, 2022

View reviewed changes

vli11 force-pushed the 3851 branch from 96a4441 to b4ae71c Compare July 7, 2022 20:53

fix: fixed security-bootstrapper Docker volume init semantics

cae5ad0

fixes: edgexfoundry#3851 Signed-off-by: Valina Li <valina.li@intel.com>

vli11 force-pushed the 3851 branch from 39bf749 to cae5ad0 Compare July 7, 2022 21:53

bnevis-i previously approved these changes Jul 7, 2022

View reviewed changes

jim-wang-intel reviewed Jul 7, 2022

View reviewed changes

cmd/security-bootstrapper/entrypoint.sh Outdated Show resolved Hide resolved

fix: add comment for edge-init detail

0be212d

Signed-off-by: Valina Li <valina.li@intel.com>

vli11 dismissed bnevis-i’s stale review via 0be212d July 7, 2022 23:41

vli11 requested review from lenny-goodell, bnevis-i and jim-wang-intel July 7, 2022 23:42

jim-wang-intel approved these changes Jul 7, 2022

View reviewed changes

lenny-goodell reviewed Jul 7, 2022

View reviewed changes

jim-wang-intel mentioned this pull request Jul 7, 2022

fix: fixed security-bootstrapper Docker volume init semantics edgexfoundry/edgex-examples#124

Merged

2 tasks

vli11 requested review from jim-wang-intel and lenny-goodell July 8, 2022 16:13

jim-wang-intel approved these changes Jul 8, 2022

View reviewed changes

bnevis-i approved these changes Jul 8, 2022

View reviewed changes

vli11 merged commit ad21f98 into edgexfoundry:main Jul 11, 2022

vli11 deleted the 3851 branch July 11, 2022 22:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fixed security-bootstrapper Docker volume init semantics #4085

fix: fixed security-bootstrapper Docker volume init semantics #4085

vli11 commented Jul 7, 2022 •

edited

Loading

lenny-goodell commented Jul 7, 2022

lenny-goodell Jul 7, 2022

jim-wang-intel Jul 7, 2022

lenny-goodell Jul 7, 2022

bnevis-i Jul 7, 2022

vli11 commented Jul 7, 2022

vli11 commented Jul 7, 2022

lenny-goodell commented Jul 7, 2022

vli11 commented Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

bnevis-i Jul 7, 2022

vli11 commented Jul 7, 2022

jim-wang-intel commented Jul 7, 2022

bnevis-i left a comment

sonarcloud bot commented Jul 7, 2022

jim-wang-intel left a comment

lenny-goodell left a comment

bnevis-i commented Jul 8, 2022

jim-wang-intel commented Jul 11, 2022

jim-wang-intel commented Jul 11, 2022

ernestojeda commented Jul 11, 2022

ernestojeda commented Jul 11, 2022

jim-wang-intel commented Jul 11, 2022

		RUN mkdir -p ${SECURITY_INIT_STAGING_DIR} \
		&& mkdir -p /edgex-init \

		COPY --from=builder /edgex-go/cmd/security-bootstrapper/entrypoint-scripts/ ${SECURITY_INIT_STAGING_DIR}/
		RUN chmod +x ${SECURITY_INIT_STAGING_DIR}/*.sh

fix: fixed security-bootstrapper Docker volume init semantics #4085

fix: fixed security-bootstrapper Docker volume init semantics #4085

Conversation

vli11 commented Jul 7, 2022 • edited Loading

PR Checklist

Testing Instructions

New Dependency Instructions (If applicable)

lenny-goodell commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vli11 commented Jul 7, 2022

vli11 commented Jul 7, 2022

lenny-goodell commented Jul 7, 2022

vli11 commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vli11 commented Jul 7, 2022

jim-wang-intel commented Jul 7, 2022

bnevis-i left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 7, 2022

jim-wang-intel left a comment

Choose a reason for hiding this comment

lenny-goodell left a comment

Choose a reason for hiding this comment

bnevis-i commented Jul 8, 2022

jim-wang-intel commented Jul 11, 2022

jim-wang-intel commented Jul 11, 2022

ernestojeda commented Jul 11, 2022

ernestojeda commented Jul 11, 2022

jim-wang-intel commented Jul 11, 2022

vli11 commented Jul 7, 2022 •

edited

Loading