-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: initial implemention of openziti to go-mod-bootstrap #659
Conversation
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.
Initial comments. Still more to review.
9e3ce1e
to
285799d
Compare
74087bf
to
e65d472
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.
LGTM, Nice work!
5427a8f
to
81f355e
Compare
@jumpingliu , @cloudxxx8 , @JamesKButcher , there are two new dependencies added by this PR. @clint has done the vetting and put the information in the PR description per our process. Though it is not explicit in the documented process, we have always had a TSC vote to add new 3rd party dependencies, |
81f355e
to
24d1ce0
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #659 +/- ##
==========================================
- Coverage 58.61% 56.01% -2.60%
==========================================
Files 31 32 +1
Lines 2537 2658 +121
==========================================
+ Hits 1487 1489 +2
- Misses 953 1070 +117
- Partials 97 99 +2 ☔ View full report in Codecov by Sentry. |
c9f8607
to
3729b23
Compare
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
…is used in edgex-go but not in this project Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
…e mock regen issue Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
…on as it doesn't seem used Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
fix: make and use constants fix: add a tiny bit of additional debug logging just in case fix: move the ConnContext before the .Serve calls fix: make the `SetHttpTransport` function one-time only (guard from being overwritten) Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
3729b23
to
9f9a216
Compare
i've amended the initial commit and signed it to fix the DCO and addressed the PR comments. |
DCO may be failed by one of your commits, so you can squash all into one commit and make sure it can pass DCT |
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
4c525f1
to
96197b9
Compare
Signed-off-by: dovholuknf <46322585+dovholuknf@users.noreply.github.com>
thanks. fixed. i really do need to setup 'auto signoff'... |
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.
LGTM
break | ||
} | ||
|
||
zc.c = &zitiCtx |
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.
@dovholuknf I'm refactoring your code for #789. I'm confused about the purpose of local zc
variable. It looks to me that the code only assigns &zitiCtx
to zc.c
here without any further processing. Do we really need to keep zc
?
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.
from what I see, it appears to be totally vestigal. I don't find any references to it whatsoever other than this one line. I expect at one point a function was defined on the struct, the function removed, leaving this behind.
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.
Thanks for reply. I'll remove both this variable and ZitiContext struct in my PR.
The usage of OpenZiti packages to support zero trust feature significantly increases the build size. For example, core-metadata increases from 14MB to 21MB, core-command increases from 9.8MB to 17MB, device-virtual increases from 18MB to 31MB, and app-service-configurable increases from 22Mb to 34MB. As many edge user scenarios require to deploy EdgeX services on resource-constrained devices without security, allowing that the services can be built without OpenZiti packages and ZeroTrust features can be helpful to user cases which don't need zero trust feature. This commit refactors the codes importing openziti packages with //go:build !no_openziti directive and creates para codes that don't use openziti packages with //go:build no_openziti directive, so users can simply build services by specifying no_openziti tag. Also remove vestigal zc variables and ZitiContext struct per discussion with https://github.com/dovholuknf in edgexfoundry#659 (comment) Signed-off-by: Jude Hung <jude@iotechsys.com>
closes edgexfoundry#789 The usage of OpenZiti packages to support zero trust feature significantly increases the build size. For example, core-metadata increases from 14MB to 21MB, core-command increases from 9.8MB to 17MB, device-virtual increases from 18MB to 31MB, and app-service-configurable increases from 22Mb to 34MB. As many edge user scenarios require to deploy EdgeX services on resource-constrained devices without security, allowing that the services can be built without OpenZiti packages and ZeroTrust features can be helpful to user cases which don't need zero trust feature. This commit refactors the codes importing openziti packages with //go:build !no_openziti directive and creates para codes that don't use openziti packages with //go:build no_openziti directive, so users can simply build services by specifying no_openziti tag. Also remove vestigal zc variables and ZitiContext struct per discussion with https://github.com/dovholuknf in edgexfoundry#659 (comment) Signed-off-by: Jude Hung <jude@iotechsys.com>
closes edgexfoundry#789 The usage of OpenZiti packages to support zero trust feature significantly increases the build size. For example, core-metadata increases from 14MB to 21MB, core-command increases from 9.8MB to 17MB, device-virtual increases from 18MB to 31MB, and app-service-configurable increases from 22Mb to 34MB. As many edge user scenarios require to deploy EdgeX services on resource-constrained devices without security, allowing that the services can be built without OpenZiti packages and ZeroTrust features can be helpful to user cases which don't need zero trust feature. This commit refactors the codes importing openziti packages with //go:build !no_openziti directive and creates para codes that don't use openziti packages with //go:build no_openziti directive, so users can simply build services by specifying no_openziti tag. Also remove vestigal zc variables and ZitiContext struct per discussion with https://github.com/dovholuknf in edgexfoundry#659 (comment) Signed-off-by: Jude Hung <jude@iotechsys.com>
* feat: Add new go build tag no_openziti to reduce build size closes #789 The usage of OpenZiti packages to support zero trust feature significantly increases the build size. For example, core-metadata increases from 14MB to 21MB, core-command increases from 9.8MB to 17MB, device-virtual increases from 18MB to 31MB, and app-service-configurable increases from 22Mb to 34MB. As many edge user scenarios require to deploy EdgeX services on resource-constrained devices without security, allowing that the services can be built without OpenZiti packages and ZeroTrust features can be helpful to user cases which don't need zero trust feature. This commit refactors the codes importing openziti packages with //go:build !no_openziti directive and creates para codes that don't use openziti packages with //go:build no_openziti directive, so users can simply build services by specifying no_openziti tag. Also remove vestigal zc variables and ZitiContext struct per discussion with https://github.com/dovholuknf in #659 (comment) Signed-off-by: Jude Hung <jude@iotechsys.com> * feat: rename the zerotrust_no_ziti.go to no_ziti.go Signed-off-by: Jude Hung <jude@iotechsys.com> --------- Signed-off-by: Jude Hung <jude@iotechsys.com>
closes #650
This is the initial PR to adopt OpenZiti into EdgexFoundry go-mod-bootstrap. Much of the functionality will be used in other repos. I've tried to be as thorough testing this as I could but I'm sure there will need to be work to fix/doc/etc. I wanted to get the code PR up as soon as possible to start getting feedback.
PR Checklist
Please check if your PR fulfills the following requirements:
BREAKING CHANGE:
describing the break)Testing Instructions
This is a sweeping change that crosses numerous repos. I don't know how to effectively answer this particular question and this one change is insufficient for testing. A larger, cross-cutting PR/effort is needed to put all the pieces together. I could use help with knowing how to satisfy this
New Dependency Instructions (If applicable)
Using https://wiki.edgexfoundry.org/display/FA/Vetting+Process+for+3rd+Party+Dependencies - new dependency review:
Total Increase in new imports: 2
github.com/openziti/sdk-golang
Releases/Tags Count: 705+ (frequent updates)
Contributor Count: 18
License: Apache v2
Stars/Forks/Watchers: on this repo - 86/12/15 but the main /ziti repo has 1768/113/34
godoc.org imported by count: 2
Subjective opinion of the reviewers: necessary to implement zero trust connectivity. the project is very well-established and lots of engineering effort has already been put into the project. recreating the same level of complexity and completeness would not be a good investment of time.
github.com/sirupsen/logrus
Releases/Tags Count: 18 (it's not the kind of project that needs many updates, still actively maintained)
Contributor Count: 262
License: MIT license
Stars/Forks/Watchers: on this repo - 23.8k/2.3k/315
godoc.org imported by count: 145,083
Subjective opinion of the reviewers: necessary only to quiet logging from OpenZiti. OpenZiti include logrus transitively, but to quiet the logging and provide an adapter to edgex's logging, it is needed.