add EnableRBAC to v20180331 api model #2647

rjtsdl · 2018-04-09T23:57:24Z

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #2646

Special notes for your reviewer:

If applicable:

documentation
unit tests
tested backward compatibility (ie. deploy with previous version, upgrade with this branch)

Release note:

amanohar · 2018-04-10T00:05:58Z

pkg/api/convertertoagentpoolonlyapi.go

@@ -349,6 +350,13 @@ func convertV20180331AgentPoolOnlyWindowsProfile(obj *v20180331.WindowsProfile)
 	}
 }

+func convertV20180331AgentPoolOnlyKubernetesConfig(enableRBAC bool) *KubernetesConfig {


@rjtsdl thanks for making this change as this also has a good side effect of resolving bug 2265988 in VSTS! You can also now remove lines 126-141 from putmanagedcluster.go and close out bug 2265988 in VSTS when new ACS E is vendored.

Yeah, I will deal with that.

I think we also need to modify the older agentPoolOnly api version behavior. To always set those two flags to be false ptr.

Made the change for v20170831 to make sure those two flags to be false ptr. Then RP don't need to deal with that.

amanohar · 2018-04-10T00:08:41Z

pkg/api/converterfromagentpoolonlyapi.go

+		enableRBAC = enableRBAC && *kc.EnableRbac
+	}
+
+	if kc.EnableSecureKubelet == nil {


IMO if the code in convertertoagentpoolonlyapi.go is written as expected then we shouldn't need to check for EnableSecureKubelet state. Checking enableRBAC flag state should suffice.

The way we want to make it, is to have EnableSecureKubelet also enabled when use specify enableRBAC. I should make it clear in the enableRBAC flag description. Check the email thread I included you.

Yes, agreed. We are on the same page regarding:

The way we want to make it, is to have EnableSecureKubelet also enabled when use specify enableRBAC.

I am suggesting that if RBAC is enabled in API model then EnableSecureKubelet should/would have been set to true. So when converting it back to versioned API model state of EnableSecureKubelet should not need to be validated.

Another way to put it is RBAC can still be technically enabled even if EnableSecureKubelet was set to false because customer only controls setting of enableRBAC. So if they set enableRBAC to true then they should get the same value back regardless of the state of EnableSecureKubelet.

why enableRBAC && false when it must be false?

what email thread?

Check the email thread I included you.

@amanohar I got your point. However, I think we still need to validate EnableSecureKubelet. Because,

if EnableRbac == true && EnableSecureKubelet == false, then the cluster won't work properly. In the api model's conversion back and forth, there is no way to make them different. However, I can still see hacks like 126-141 from putmanagedcluster.go could make them different. That is why I want to validate both of them.

Validation is used to surface hacks :)

JunSun17 · 2018-04-10T00:25:17Z

pkg/api/agentPoolOnlyApi/v20180331/types.go

@@ -40,6 +40,7 @@ type Properties struct {
 	AccessProfiles          map[string]AccessProfile `json:"accessProfiles,omitempty"`
 	AddonProfiles           map[string]AddonProfile  `json:"addonProfiles,omitempty"`
 	NodeResourceGroup       string                   `json:"nodeResourceGroup,omitempty"`
+	EnableRBAC              bool                     `json:"enableRBAC"`


does omitempty matter here?

should this be on by default?

can this be changed? (turning on and off?)

Change to *bool, so we have 3 state

nil -> true
true -> true
false -> false

Note, we default behavior is to set it to true.

JunSun17 · 2018-04-10T00:31:05Z

pkg/api/converterfromagentpoolonlyapi.go

+	}
+	// Only if both KubernetesConfig.EnableRbac and KubernetesConfig.EnableSecureKubelet true
+	// We think EnableRBAC is on
+	enableRBAC := true


the code seems pretty complicated to me. why not base on the comments:

enableRBAC := (KubernetesConfig.EnableRbac == true) && (KubernetesConfig.EnableSecureKubelet == true)

EnableRbac is *bool !

I see, could this work then:
enableRBAC := (KubernetesConfig.EnableRbac != nil) && ((KubernetesConfig.EnableRbac) == true) && (KubernetesConfig.EnableSecureKubelet != nil) && ((KubernetesConfig.EnableSecureKubelet) == true)

no big deal, just want to make code a bit more easy to read. :)

Made it easier to read.

JunSun17 · 2018-04-10T00:32:33Z

pkg/api/converterfromagentpoolonlyapi.go

 	if api.OrchestratorProfile != nil {
 		if api.OrchestratorProfile.OrchestratorVersion != "" {
 			p.KubernetesVersion = api.OrchestratorProfile.OrchestratorVersion
 		}
+		p.EnableRBAC = convertKubernetesConfigToEnableRBACV20180331AgentPoolOnly(api.OrchestratorProfile.KubernetesConfig)
+		p.EnableRBAC = *api.OrchestratorProfile.KubernetesConfig.EnableRbac &&


is this line redundant?

Yeah, it is :) Removed

rjtsdl · 2018-04-10T16:26:24Z

I have change the default behavior to be true for EnableRBAC api flag.
I also modified the older agentPoolOnly api model to always set those to be false, then we can remove the hack in RP codebase.

…d to hack

… add unittest

amanohar · 2018-04-10T17:08:55Z

pkg/api/converterfromagentpoolonlyapi.go

+	// Only if both KubernetesConfig.EnableRbac and KubernetesConfig.EnableSecureKubelet true
+	// We think EnableRBAC is on
+	if kc != nil && kc.EnableRbac != nil && *kc.EnableRbac &&
+		kc.EnableSecureKubelet != nil && *kc.EnableSecureKubelet {


I still think that EnableSecureKubelet should not factor into converting back to versioned API model. if *kc.EnableRbac then it should be enough to say that RBAC is enabled. You can also always log a warning line that EnableSecureKubelet was not enabled.

I don't have strong opinion on this. Updated only use *kc.EnableRbac to deduct the versioned api model.

amanohar · 2018-04-10T17:09:29Z

pkg/api/convertertoagentpoolonlyapi.go

@@ -349,6 +354,20 @@ func convertV20180331AgentPoolOnlyWindowsProfile(obj *v20180331.WindowsProfile)
 	}
 }

+func convertV20180331AgentPoolOnlyKubernetesConfig(enableRBAC *bool) *KubernetesConfig {
+	if enableRBAC == nil || *enableRBAC == true {


If if enableRBAC == nil shouldn't EnableRbac be false?

Oh, we want the default behavior to be true.

Default behavior is when enableRBAC == nil

Ok, cool. Thanks!

amanohar

Added some feedback. PTAL.

rjtsdl · 2018-04-10T17:22:36Z

TODO:

@JunSun17 have a link to the api document? I need to update that
We need to update the template used in Build to specify this field to be false.
@amanohar , you probably also want to add enableAADAuth in a similar fashion. I will leave it to you :)

CecileRobertMichon · 2018-04-10T19:12:11Z

pkg/api/common/helper.go

@@ -51,3 +51,8 @@ func HandleValidationErrors(e validator.ValidationErrors) error {
 	}
 	return fmt.Errorf("Namespace %s is not caught, %+v", ns, e)
 }
+
+// BoolPtr convert a bool to *bool
+func BoolPtr(b bool) *bool {


there is already a function doing this at

acs-engine/pkg/helpers/helpers.go

Line 55 in 7272611

func PointerToBool(b bool) *bool {

I think you could re-use it

yep, will do

CecileRobertMichon

lgtm

* add EnableRBAC to v20180331 api model * behavior change to set default EnableRBAC to be true for v20180331 api * set KubernetesConfig to be false for those two flags, so RP don't need to hack * set older version's kubernetesConfig's two flags to be false ptr, and add unittest * only use EnableRBAC to convert to versioned api model * re-use PointerToBool

ghost assigned rjtsdl Apr 9, 2018

ghost added the in progress label Apr 9, 2018

rjtsdl requested review from weinong, amanohar and JunSun17 April 9, 2018 23:57

rjtsdl force-pushed the jiren-addrbacflag branch from 81e9755 to a0bead6 Compare April 9, 2018 23:59

add EnableRBAC to v20180331 api model

3a5a6c9

rjtsdl force-pushed the jiren-addrbacflag branch from a0bead6 to 3a5a6c9 Compare April 10, 2018 00:01

amanohar suggested changes Apr 10, 2018

View reviewed changes

JunSun17 reviewed Apr 10, 2018

View reviewed changes

behavior change to set default EnableRBAC to be true for v20180331 api

312e6c3

Jingtao Ren added 2 commits April 10, 2018 09:45

set KubernetesConfig to be false for those two flags, so RP don't nee…

cebd6c0

…d to hack

set older version's kubernetesConfig's two flags to be false ptr, and…

21eb25f

… add unittest

rjtsdl added the ready label Apr 10, 2018

amanohar reviewed Apr 10, 2018

View reviewed changes

only use EnableRBAC to convert to versioned api model

9a6f3b5

ghost removed the ready label Apr 10, 2018

amanohar previously approved these changes Apr 10, 2018

View reviewed changes

weinong previously approved these changes Apr 10, 2018

View reviewed changes

CecileRobertMichon reviewed Apr 10, 2018

View reviewed changes

rjtsdl dismissed stale reviews from weinong and amanohar via ec87d4d April 10, 2018 20:10

re-use PointerToBool

10bc6a0

rjtsdl force-pushed the jiren-addrbacflag branch from ec87d4d to 10bc6a0 Compare April 10, 2018 20:17

CecileRobertMichon approved these changes Apr 10, 2018

View reviewed changes

jackfrancis merged commit cf0f10d into Azure:master Apr 10, 2018

ghost removed the in progress label Apr 10, 2018

jackfrancis deleted the jiren-addrbacflag branch April 10, 2018 22:18

rjtsdl mentioned this pull request Apr 10, 2018

add Merge function for v20180331 to guard update scenario #2653

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add EnableRBAC to v20180331 api model #2647

add EnableRBAC to v20180331 api model #2647

rjtsdl commented Apr 9, 2018 •

edited

Loading

amanohar Apr 10, 2018 •

edited

Loading

rjtsdl Apr 10, 2018

rjtsdl Apr 10, 2018

amanohar Apr 10, 2018 •

edited

Loading

rjtsdl Apr 10, 2018

amanohar Apr 10, 2018 •

edited

Loading

weinong Apr 10, 2018

weinong Apr 10, 2018

rjtsdl Apr 10, 2018 •

edited

Loading

JunSun17 Apr 10, 2018

weinong Apr 10, 2018

rjtsdl Apr 10, 2018

JunSun17 Apr 10, 2018

rjtsdl Apr 10, 2018

JunSun17 Apr 10, 2018

rjtsdl Apr 10, 2018

JunSun17 Apr 10, 2018

rjtsdl Apr 10, 2018 •

edited

Loading

rjtsdl commented Apr 10, 2018 •

edited

Loading

amanohar Apr 10, 2018

rjtsdl Apr 10, 2018

amanohar Apr 10, 2018

rjtsdl Apr 10, 2018

amanohar Apr 10, 2018

amanohar left a comment

rjtsdl commented Apr 10, 2018 •

edited

Loading

CecileRobertMichon Apr 10, 2018

rjtsdl Apr 10, 2018

CecileRobertMichon left a comment

add EnableRBAC to v20180331 api model #2647

add EnableRBAC to v20180331 api model #2647

Conversation

rjtsdl commented Apr 9, 2018 • edited Loading

amanohar Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanohar Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjtsdl Apr 10, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjtsdl Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

rjtsdl commented Apr 10, 2018 • edited Loading

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

amanohar left a comment

Choose a reason for hiding this comment

rjtsdl commented Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon left a comment

Choose a reason for hiding this comment

rjtsdl commented Apr 9, 2018 •

edited

Loading

amanohar Apr 10, 2018 •

edited

Loading

amanohar Apr 10, 2018 •

edited

Loading

amanohar Apr 10, 2018 •

edited

Loading

rjtsdl Apr 10, 2018 •

edited

Loading

rjtsdl Apr 10, 2018 •

edited

Loading

rjtsdl commented Apr 10, 2018 •

edited

Loading

rjtsdl commented Apr 10, 2018 •

edited

Loading