add a configurable option to switch yaml parser between yaml and utilyaml #497

nnmin-aws · 2022-10-06T00:19:56Z

Add a configurable option to switch yaml parser between yaml and utilyaml. refer to #455

k8s-ci-robot · 2022-10-06T00:20:05Z

Hi @nnmin-aws. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-10-06T00:20:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nnmin-aws
Once this PR has been reviewed and has the lgtm label, please assign jyotimahapatra for approval by writing /assign @jyotimahapatra in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

nnmin-aws · 2022-10-06T00:20:09Z

/hold

nnmin-aws · 2022-10-06T00:20:17Z

/ok-to-test

k8s-ci-robot · 2022-10-06T00:20:31Z

@nnmin-aws: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nckturner · 2022-10-06T16:58:35Z

/ok-to-test

nckturner · 2022-10-06T17:02:18Z

pkg/mapper/configmap/configmap.go

@@ -115,15 +115,19 @@ func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappi
 	rawUserMappings := make([]config.UserMapping, 0)
 	userMappings = make([]config.UserMapping, 0)
 	if userData, ok := m["mapUsers"]; ok {
-		userJson, err := utilyaml.ToJSON([]byte(userData))
+		if LenientYamlEKS {


IMO one improvement would be push this logic into a utility. We can have a pkg/config/yaml which includes our own implementation of yaml.Unmarshal which makes this decision based on a struct variable. Then its set when the struct is initialized and we can do that once when authenticator starts up based on the flag value.

IMO one improvement would be push this logic into a utility. We can have a pkg/config/yaml which includes our own implementation of yaml.Unmarshal which makes this decision based on a struct variable. Then its set when the struct is initialized and we can do that once when authenticator starts up based on the flag value.

thank you for the comments. this can be a future goal to build a pkg/config/yaml if there are more changes about yaml parsing.

Why does it need to be a future change? Here is an example to give you an idea of what I was thinking:

In pkg/mapper/configmap/configmap.go:

import ( "sigs.k8s.io/aws-iam-authenticator/pkg/yaml" ) ... func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappings []config.RoleMapping, awsAccounts []string, err error) { ... if userData, ok := m["mapUsers"]; ok { if err := yaml.Unmarshal([]byte(userData), &userMappings); err != nil { errs = append(errs, err) } } ...

In pkg/yaml:

// compatibilityMode enables specific yaml parsing behavior used by EKS. var compatibilityMode = true func Unmarshal(b []byte, obj interface{}) error { if !compatibilityMode { if s, err := utilyaml.ToJSON(b); err != nil { return err } else if err := json.Unmarshal(s, obj); err != nil { return err } return nil } return yaml.Unmarshal(b, obj) }

Note that I suggested to use a struct above, but I used a global variable and function in this example, which might be cleaner. I also think pkg/yaml is fine for a location.

+1 for this idea, also could make a case where any other need for it would just be able to import it from pkg as I believe we already do farther down this file.

This is only short term fix to solve certain case in EKS configmap parsing. We are working on long term solution for this. Given essentially it is a short-term fix, I'd like to constrain it only in configmap.go. Because if exposed in a new package, we can't control how the new package will be used potentially. It is harder to clean it up in future.

wongma7 · 2022-10-06T18:01:24Z

pkg/mapper/configmap/configmap.go

+			//use Lenient Yaml Parser
+			err = yaml.Unmarshal([]byte(userData), &rawUserMappings)
+		} else {
+			//use Strict Yaml Parser


lenient/strict is not 100% accurate, the "lenient" parser is case insensitive, but it does not konw how to deal with username: {{SessionName}}, it expects quotes likeusername: "{{SessionName}}"

can we have a test case for the {{SessionName}} issue, I'll try to find a link to it. But basically we are using {{ in the golang template sense, but {{ also has a meaning in the yaml sense https://yaml.org/spec/1.2.2/#flow-mappings

lenient/strict is not 100% accurate, the "lenient" parser is case insensitive, but it does not konw how to deal with username: {{SessionName}}, it expects quotes likeusername: "{{SessionName}}"

can we have a test case for the {{SessionName}} issue, I'll try to find a link to it. But basically we are using {{ in the golang template sense, but {{ also has a meaning in the yaml sense https://yaml.org/spec/1.2.2/#flow-mappings

thank you for the comments. there are test yamls https://github.com/kubernetes-sigs/aws-iam-authenticator/tree/master/pkg/mapper/configmap/yaml. do they cover the case you mentioned? this cr is to include #455 on master branch, it is only on release-0.5 branch now.

With regards to the naming, I agree. I verified that sigs.k8s.io/yaml does a case insensitive key comparison ("lenient") but errors on an unquoted curly braces ("strict"). I think we should instead call the behavior "compatibility mode" with a comment explanation.

do they cover the case you mentioned?

We don't currently have a test for the case mentioned. The test needs to include a mapping of key to value where the first letter of the value is a curly brace and the entire value is unquoted:

key: {{value}}

yep the testes cases are here https://github.com/kubernetes-sigs/aws-iam-authenticator/pull/497/files#diff-8b827dc0fc13d692520fe80df4d6a52e15b065812a9f4a7b906f3ded58afd865R92

nckturner · 2022-10-11T09:15:28Z

pkg/mapper/configmap/configmap.go

+			//use Lenient Yaml Parser
+			err = yaml.Unmarshal([]byte(userData), &rawUserMappings)
+		} else {
+			//use Strict Yaml Parser


With regards to the naming, I agree. I verified that sigs.k8s.io/yaml does a case insensitive key comparison ("lenient") but errors on an unquoted curly braces ("strict"). I think we should instead call the behavior "compatibility mode" with a comment explanation.

nckturner · 2022-10-11T09:26:43Z

pkg/mapper/configmap/configmap.go

+			//use Lenient Yaml Parser
+			err = yaml.Unmarshal([]byte(userData), &rawUserMappings)
+		} else {
+			//use Strict Yaml Parser


do they cover the case you mentioned?

We don't currently have a test for the case mentioned. The test needs to include a mapping of key to value where the first letter of the value is a curly brace and the entire value is unquoted:

key: {{value}}

nckturner · 2022-10-11T10:22:09Z

pkg/mapper/configmap/configmap.go

@@ -115,15 +115,19 @@ func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappi
 	rawUserMappings := make([]config.UserMapping, 0)
 	userMappings = make([]config.UserMapping, 0)
 	if userData, ok := m["mapUsers"]; ok {
-		userJson, err := utilyaml.ToJSON([]byte(userData))
+		if LenientYamlEKS {


Why does it need to be a future change? Here is an example to give you an idea of what I was thinking:

In pkg/mapper/configmap/configmap.go:

import ( "sigs.k8s.io/aws-iam-authenticator/pkg/yaml" ) ... func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappings []config.RoleMapping, awsAccounts []string, err error) { ... if userData, ok := m["mapUsers"]; ok { if err := yaml.Unmarshal([]byte(userData), &userMappings); err != nil { errs = append(errs, err) } } ...

In pkg/yaml:

// compatibilityMode enables specific yaml parsing behavior used by EKS. var compatibilityMode = true func Unmarshal(b []byte, obj interface{}) error { if !compatibilityMode { if s, err := utilyaml.ToJSON(b); err != nil { return err } else if err := json.Unmarshal(s, obj); err != nil { return err } return nil } return yaml.Unmarshal(b, obj) }

Note that I suggested to use a struct above, but I used a global variable and function in this example, which might be cleaner. I also think pkg/yaml is fine for a location.

minj131 · 2022-10-11T19:07:59Z

pkg/mapper/configmap/mapper.go

@@ -13,7 +13,10 @@ type ConfigMapMapper struct {

 var _ mapper.Mapper = &ConfigMapMapper{}

+var LenientYamlEKS = false


What's the reasoning for this global exported variable here?

it is only meaningful in this file. it is a short term fix for EKS configmap

Thoughts on making it unexportable then if it's only meaningful in this package?

…yaml

nckturner · 2022-10-11T22:32:45Z

pkg/mapper/configmap/configmap.go

@@ -115,15 +115,19 @@ func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappi
 	rawUserMappings := make([]config.UserMapping, 0)
 	userMappings = make([]config.UserMapping, 0)
 	if userData, ok := m["mapUsers"]; ok {
-		userJson, err := utilyaml.ToJSON([]byte(userData))
+		if EKSYaml {
+			//use Lenient Yaml Parser


Remove or update comments

nckturner · 2022-10-12T22:52:45Z

/close

k8s-ci-robot · 2022-10-12T22:52:52Z

@nckturner: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2022

k8s-ci-robot requested review from jyotimahapatra and nckturner October 6, 2022 00:20

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 6, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 6, 2022

nckturner reviewed Oct 6, 2022

View reviewed changes

wongma7 reviewed Oct 6, 2022

View reviewed changes

nckturner requested changes Oct 11, 2022

View reviewed changes

minj131 reviewed Oct 11, 2022

View reviewed changes

add a configurable option to switch yaml parser between yaml and util…

f5b6e04

…yaml

nnmin-aws force-pushed the nnmin-release branch from bc69c3f to f5b6e04 Compare October 11, 2022 21:03

nckturner reviewed Oct 11, 2022

View reviewed changes

k8s-ci-robot closed this Oct 12, 2022

nnmin-aws deleted the nnmin-release branch November 5, 2022 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a configurable option to switch yaml parser between yaml and utilyaml #497

add a configurable option to switch yaml parser between yaml and utilyaml #497

nnmin-aws commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

nnmin-aws commented Oct 6, 2022

nnmin-aws commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

nckturner commented Oct 6, 2022

nckturner Oct 6, 2022

nnmin-aws Oct 6, 2022

nckturner Oct 11, 2022 •

edited

minj131 Oct 11, 2022 •

edited

nnmin-aws Oct 11, 2022

wongma7 Oct 6, 2022

nnmin-aws Oct 6, 2022 •

edited

nckturner Oct 11, 2022

nckturner Oct 11, 2022

nnmin-aws Oct 11, 2022

nckturner Oct 11, 2022

nckturner Oct 11, 2022

nckturner Oct 11, 2022 •

edited

minj131 Oct 11, 2022

nnmin-aws Oct 11, 2022

minj131 Oct 11, 2022

nckturner Oct 11, 2022

nckturner commented Oct 12, 2022

k8s-ci-robot commented Oct 12, 2022

		@@ -13,7 +13,10 @@ type ConfigMapMapper struct {

		var _ mapper.Mapper = &ConfigMapMapper{}

		var LenientYamlEKS = false

add a configurable option to switch yaml parser between yaml and utilyaml #497

add a configurable option to switch yaml parser between yaml and utilyaml #497

Conversation

nnmin-aws commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

nnmin-aws commented Oct 6, 2022

nnmin-aws commented Oct 6, 2022

k8s-ci-robot commented Oct 6, 2022

nckturner commented Oct 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nckturner Oct 11, 2022 • edited

Choose a reason for hiding this comment

minj131 Oct 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nnmin-aws Oct 6, 2022 • edited

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

nckturner Oct 11, 2022 • edited

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

nckturner commented Oct 12, 2022

k8s-ci-robot commented Oct 12, 2022

nckturner Oct 11, 2022 •

edited

minj131 Oct 11, 2022 •

edited

nnmin-aws Oct 6, 2022 •

edited

nckturner Oct 11, 2022 •

edited