🐛 Use API Server DNS Names by default when logging into VM Web Console #735
+734
−52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ammujumdar-bcom
force-pushed
the
topic/ammujumdar/use-fqdn-if-given
branch
4 times, most recently
from
4296bf1
to
60882f1
Compare
github-actions
bot
added
size/XL
ammujumdar-bcom
force-pushed
the
topic/ammujumdar/use-fqdn-if-given
branch
from
cdb36ef
to
4f4742b
Compare
ammujumdar-bcom
changed the title
sreyasn
reviewed
Oct 18, 2024
| } | ||
|
|
||
| // ProxyAddressFromVirtualIP retrieves the virtual IP, which will be used as the Proxy Address. | ||
| func (r *Reconciler) ProxyAddressFromVirtualIP(ctx *pkgctx.WebConsoleRequestContextV1) (string, error) { |
There was a problem hiding this comment.
wondering if this method can also go into the pkg/util/webconsole/ now? @dilyar85 ?
There was a problem hiding this comment.
The thing is that the contexts for this and the v1alpha1 function are different data types. Does this matter?
If we move this to the webconsole directory, could we just use context.Context as the type?
There was a problem hiding this comment.
the contexts for this and the v1alpha1 function are different data types. Does this matter?
shouldn't matter. cause it's only doing client.Get() with the ctx.
could we just use context.Context as the type?
yep.
I'll let @dilyar85 comment if we need to keep it here or move.
There was a problem hiding this comment.
Moved it for now.
There was a problem hiding this comment.
Yep agree better to move to the util package (also commented on it below).
sreyasn
reviewed
Oct 18, 2024
controllers/virtualmachinewebconsolerequest/v1alpha1/webconsolerequest_controller.go
Outdated
Show resolved
Hide resolved
ammujumdar-bcom
force-pushed
the
topic/ammujumdar/use-fqdn-if-given
branch
from
641051a
to
374470e
Compare
dilyar85
requested changes
Oct 22, 2024
controllers/virtualmachinewebconsolerequest/v1alpha1/webconsolerequest_unit_test.go
Outdated
Show resolved
Hide resolved
pkg/webconsoleurl/url_lookup.go
Outdated
There was a problem hiding this comment.
I think a better place and name for this file could be pkg/util/proxy_address.go, as none of the functions inside are exclusive to the web console URL.
Also, can we add a test file for this? It should be easy to reach 100% CC since it simply gets the CR and returns the proxy address.
bryanv
reviewed
Oct 23, 2024
| ) | ||
| }) | ||
|
|
||
| scenarios := []struct { |
There was a problem hiding this comment.
Using DescribeTable - https://onsi.github.io/ginkgo/#table-specs - is the more conventional way to write tests like this for Ginkgo.
Why is there a webconsolerequest.NewReconciler() in both the inner and outer JustBeforeEach()?
There was a problem hiding this comment.
If I set the FSS in each iteration (in the same newreconciler call as the API Server DNS Name setting), then it will not actually set the FSS properly in fail. Hence, I do it up above.
There was a problem hiding this comment.
The problem that I am facing is that I am trying to modify the config (reconciler) just before each entry. It seems that JustBeforeEach and DescribeTable are not compatible with each other, so whenever I try to update proxySvcDNS within each Entry, the changes aren't actually going in affect.
controllers/virtualmachinewebconsolerequest/v1alpha2/webconsolerequest_controller.go
Outdated
Show resolved
Hide resolved
Minimum allowed line rate is |
ammujumdar-bcom
force-pushed
the
topic/ammujumdar/use-fqdn-if-given
branch
from
4f3d1dc
to
a3823cf
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do, and why is it needed?
In certain environments, a load balancer, and therefore a virtual IP, may not be present. In these cases, rather than relying on the virtual IP to log into the VM web console, we need to instead rely on an FQDN / DNS name to login.
This change plumbs the API Server DNS Names from the app platform CRD, and uses that by default to login to the VM Web Console. If no DNS Name is found, then we fall back to the previous method of using the virtual IP to login.
Testing Done:
Used an existing testbed - note that this setup has a load balancer already
Setup steps:
my-vmontest-namespace/usr/lib/vmware-wcp/objects/PodVM-GuestCluster/30-vmop/vmop.yamlfile to add rbac permissions for appplatform (since vmop tar won't load those changes) and re-applied yamlmake docker-build,docker save docker.io/library/vmoperator-controller:latest > vmopfqdn.tar, anddeploy-wcp.shNext, verified that web console returns API Server DNS name for login:
Which issue(s) is/are addressed by this PR? (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Are there any special notes for your reviewer:
Please add a release note if necessary: