feat(init): enable switching between nodeport and proxy mode#4608
feat(init): enable switching between nodeport and proxy mode#4608AustinAbro321 wants to merge 12 commits intomainfrom
Conversation
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Austin Abro <austinabro321@gmail.com>
AustinAbro321
force-pushed
the
nodeport-to-proxy-mode
branch
from
681bdc8 to
da155fb
Compare
Codecov Report❌ Patch coverage is
... and 9 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
Signed-off-by: Austin Abro <austinabro321@gmail.com>
AustinAbro321
changed the title
brandtkeller
left a comment
brandtkeller
left a comment
There was a problem hiding this comment.
One observation - otherwise documentation for switching from one to another as a supported operation could be helpful if not currently planned.
src/pkg/state/state.go
Outdated
| } | ||
|
|
||
| // CheckIfCredsChanged compares two RegistryInfo structs and returns true if any non-empty fields have changed | ||
| func CheckIfCredsChanged(existing, given RegistryInfo) bool { |
There was a problem hiding this comment.
This does not check NodePort - whereas it would have before. Which is intentional in the context of running zarf init and changing the mode from nodeport to proxy.
But does/should a nodeport to nodeport upgrade with a nodeport change silently pass here now? Should we establish a check back in the caller for this case?
There was a problem hiding this comment.
Yeah it does pass where it didn't before, but I think that's a benefit. I don't think there's an advantage to stopping the update of the nodeport. I believe the historical reason we did this is because it was more convenient to use helpers.IsNotZeroAndNotEqual instead of a custom function I created here
There was a problem hiding this comment.
That sounds fair. If the nodeport is updated - do we ever hit a scenario where an artifact can not be pulled due to a change in the registry service? Everything GitOps pulls from the internal service (so we're fine there). Pods should be maintained by a cache on the node until another mutation request is sent to update the host.
There was a problem hiding this comment.
Zarf and the kubelet won't because of the situation you described. If the user has something else pulling from the nodeport (discouraged outside of the kubelet case) it's possible, but since the user is opting into the nodeport change, I think that's reasonable.
Signed-off-by: Austin Abro <austinabro321@gmail.com>
brandtkeller
left a comment
brandtkeller
left a comment
There was a problem hiding this comment.
minor comments - an interesting side effect I didn't see mentioned is that deploying nodeport -> proxy -> nodeport looks to have retained the mtls settings on the nodeport solution. I am thinking about the implications here but I also don't know if this was intended to be a fully supported workflow. Documentation here would be useful - to include the upgrade support on init such that you do not need to re-declare your previous mode.
| } | ||
|
|
||
| // CheckIfRegistryCredsChanged compares two RegistryInfo structs and returns true if any non-empty fields have changed | ||
| func CheckIfRegistryCredsChanged(existing, given RegistryInfo) bool { |
There was a problem hiding this comment.
Slightly misleading function name. We include Address in the comparison but not NodePort or Mode. So it is not strictly creds nor is it a comparison for potential drift between two RegistryInfo Structs (See doccomment).
| o.registryInfo.RegistryMode = state.RegistryModeExternal | ||
| } | ||
|
|
||
| if o.registryInfo.NodePort == 0 && o.registryInfo.RegistryMode == state.RegistryModeNodePort { |
There was a problem hiding this comment.
Initially this feels duplicative of something that could occur in FillInEmptyValues but in tracing I see that this is needed for InitState's opts.RegistryInfo.NodePort != 0 guard to work on mode switches.
This works in the given state but feels a little confusing. My only thought is that being able to discern empty from go default could be handy - but I don't know if we want to change the surface of those fields now... (thinking *int).
2 participants
Description
This allows switching between nodeport and proxy mode. When a user runs a secondary
zarf init, if no mode is specified, then the mode that this currently in Zarf state will be used. If a mode is specified, then that mode will be used insteadRelated Issue
Fixes #4586
Checklist before merging