feat: Enabling SSH on windows nodes by default #2759
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marosset The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #2759 +/- ##
========================================
Coverage 71.03% 71.04%
========================================
Files 147 147
Lines 25553 25679 +126
========================================
+ Hits 18152 18244 +92
- Misses 6271 6297 +26
- Partials 1130 1138 +8
Continue to review full report at Codecov.
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
marosset
force-pushed
the
win-ssh-default-on
branch
from
a25c4c5
to
46a7ea9
Compare
|
@jackfrancis @jadarsie @ksubrmnn PTAL |
| if w.SSHEnabled != nil { | ||
| return *w.SSHEnabled | ||
| } | ||
| return DefaultWindowsSSHEnabled |
There was a problem hiding this comment.
This is probably not what we want. Are we assuming that this method is only invoked in execution flows after default enforcement flows have been traversed? (And if so, I still don’t think we want this method on this pointer object in this way.)
There was a problem hiding this comment.
This is following the same behavior as GetEnabledWindowsUpdate directly below.
This was done this way in order to not perform updates to the stored apimodel.json in upgrade/scale scenarios but still be able to enforce a default value.
What do you have in mind for this logic?
There was a problem hiding this comment.
So these methods are just for the E2E test runner?
There was a problem hiding this comment.
And to protection when operating on older templates.
There was a problem hiding this comment.
(I think - I initially wrote this ~2 months ago)
There was a problem hiding this comment.
so I took a look and if there is an apimodel.json that does not have sshEnabled set is passed in to aks-engine generate the resulting apimodel.json in _output does persist the default value in code.
There was a problem hiding this comment.
The logic in defaults.go will set a value for w.SSHEnabled for generate operations but will not set a value for aks-engine upgrade or aks-engine scale operations - which looks like the behavior we want.
I see a unit test that verifies apimodel.json is not modified during those commands.
@jackfrancis i'm not following why you are against having a function first do a nil-check before returning a value - can you help me understand your concerns here?
There was a problem hiding this comment.
I think my main concern is that GetSSHEnabled is a accessor method that wraps the SSHEnabled bool pointer property. So I would expect a nil value of that to be falsy.
There was a problem hiding this comment.
This PR changed SSHEnabled property from a bool to a bool pointer so we could get an extra state - set to false, set to true, or unset by the user).
In the case where it is unset by the user wouldn't it make sense to set it to a default value instead of false?
There was a problem hiding this comment.
Right, perhaps I'm being too pedantic about that. It just seems that an accessor should return the truth, and if we want nil values to be semantically meaningful we should do that semantic enforcment so that it's sticky. I.e., so that by the time we use the accessors we can just have them be accessors, rather than being overloaded with that default value hijacking.
I think concretely what feels right to me is to set the default values during the defaults enforcement flow, and then confirm that the accessor is always being used only after that flow has been traversed and the data state representations have been rationalized (nil values converted to defaults, etc)
| @@ -298,7 +298,7 @@ var _ = Describe("Azure Container Cluster using the Kubernetes Orchestrator", fu | |||
| dockerVersionCmd := fmt.Sprintf("\"docker version\"") | |||
| for _, n := range nodes { | |||
| if n.IsWindows() { | |||
| if eng.ExpandedDefinition.Properties.WindowsProfile != nil && !eng.ExpandedDefinition.Properties.WindowsProfile.SSHEnabled { | |||
| if eng.ExpandedDefinition.Properties.WindowsProfile != nil && !eng.ExpandedDefinition.Properties.WindowsProfile.GetSSHEnabled() { | |||
There was a problem hiding this comment.
for example, the method invocation here will operate against a ContainerService object post-template generation (i.e., we can rely upon the actual data in the object and not have to worry about highjacking the underlying value w/ the default const value)
There was a problem hiding this comment.
What about the case where we are working with an template generated with an older version of aks-engine which may not have a value for sshEnabled stored?
If we call GetSSHEnabled() we are guaranteed to never have a nil value.
There was a problem hiding this comment.
Good point. I think we have the runner call a func/method in its own area that wraps nil values underneath appropriate defaults (true/false)
marosset
force-pushed
the
win-ssh-default-on
branch
from
ff8a694
to
5a681ff
Compare
* Add LicenseType in WindowsProfile * feat: Enabling SSH on windows nodes by default (#2759) Co-authored-by: Mark Rossetti <marosset@microsoft.com>
Reason for Change:
SSH has been supported on Windows since 1809/WS2019.
Since Windows 1803 is no longer being supported by MS this PR installs/configures SSH for windows agent pools unless WindowsProfile.SSHEnables is explicitly set to false.
Issue Fixed:
Requirements:
Notes: