Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/aws-iam-authenticator/root.go
View file
Open in desktop
Expand Up @@ -102,6 +102,7 @@ func getConfig() (config.Config, error) {
EC2DescribeInstancesQps: viper.GetInt("server.ec2DescribeInstancesQps"),
EC2DescribeInstancesBurst: viper.GetInt("server.ec2DescribeInstancesBurst"),
ScrubbedAWSAccounts: viper.GetStringSlice("server.scrubbedAccounts"),
EKSYaml: viper.GetBool("server.eksyaml"),
}
if err := viper.UnmarshalKey("server.mapRoles", &cfg.RoleMappings); err != nil {
return cfg, fmt.Errorf("invalid server role mappings: %v", err)
Expand Down
5 changes: 3 additions & 2 deletions pkg/config/types.go
View file
Open in desktop
Expand Up @@ -32,8 +32,8 @@ type IdentityMapping struct {
// list of Kubernetes groups. The username and groups are specified as templates
// that may optionally contain two template parameters:
//
// 1) "{{AccountID}}" is the 12 digit AWS ID.
// 2) "{{SessionName}}" is the role session name.
// 1. "{{AccountID}}" is the 12 digit AWS ID.
// 2. "{{SessionName}}" is the role session name.
//
// The meaning of SessionName depends on the type of entity assuming the role.
// In the case of an EC2 instance role this will be the EC2 instance ID. In the
Expand Down Expand Up @@ -169,4 +169,5 @@ type Config struct {
// understand we don't need to change
EC2DescribeInstancesQps int
EC2DescribeInstancesBurst int
EKSYaml bool
}
33 changes: 21 additions & 12 deletions pkg/mapper/configmap/configmap.go
View file
Open in desktop
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove or update comments

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nnmin-aws nnmin-aws Oct 6, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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}}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

userJson, err := utilyaml.ToJSON([]byte(userData))
if err == nil {
err = json.Unmarshal(userJson, &rawUserMappings)
}
}
if err != nil {
errs = append(errs, err)
} else {
err = json.Unmarshal(userJson, &rawUserMappings)
if err != nil {
errs = append(errs, err)
}

for _, userMapping := range rawUserMappings {
err = userMapping.Validate()
if err != nil {
Expand All @@ -138,15 +142,20 @@ func ParseMap(m map[string]string) (userMappings []config.UserMapping, roleMappi
rawRoleMappings := make([]config.RoleMapping, 0)
roleMappings = make([]config.RoleMapping, 0)
if roleData, ok := m["mapRoles"]; ok {
roleJson, err := utilyaml.ToJSON([]byte(roleData))
if err != nil {
errs = append(errs, err)
if EKSYaml {
//use Lenient Yaml Parser
err = yaml.Unmarshal([]byte(roleData), &rawRoleMappings)
} else {
err = json.Unmarshal(roleJson, &rawRoleMappings)
if err != nil {
errs = append(errs, err)
//use Strict Yaml Parser
roleJson, err := utilyaml.ToJSON([]byte(roleData))
if err == nil {
err = json.Unmarshal(roleJson, &rawRoleMappings)
}
}

if err != nil {
errs = append(errs, err)
} else {
for _, roleMapping := range rawRoleMappings {
err = roleMapping.Validate()
if err != nil {
Expand Down
1 change: 1 addition & 0 deletions pkg/mapper/configmap/configmap_test.go
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@ import (

func init() {
config.SSORoleMatchEnabled = true
EKSYaml = true
}

var (
Expand Down
3 changes: 3 additions & 0 deletions pkg/mapper/configmap/mapper.go
View file
Open in desktop
Expand Up @@ -13,7 +13,10 @@ type ConfigMapMapper struct {

var _ mapper.Mapper = &ConfigMapMapper{}

var EKSYaml = false

func NewConfigMapMapper(cfg config.Config) (*ConfigMapMapper, error) {
EKSYaml = cfg.EKSYaml
ms, err := New(cfg.Master, cfg.Kubeconfig)
if err != nil {
return nil, err
Expand Down
35 changes: 25 additions & 10 deletions pkg/mapper/configmap/yaml_test.go
View file
Open in desktop
Expand Up @@ -21,6 +21,11 @@ import (

var log = logrus.New()

func init() {
config.SSORoleMatchEnabled = true
EKSYaml = true
}

func TestConfigMap(t *testing.T) {
log.Level = logrus.DebugLevel

Expand Down Expand Up @@ -56,16 +61,6 @@ func TestConfigMap(t *testing.T) {
// Valid aws-auth.yaml based on one in EKS documentation.
"aws-auth.yaml", validRoleMappings, validUserMappings, validAWSAccounts, false,
},
{
// RoLeArN instead of rolearn
// parsing succeeds, values are case-insensitive for compatibility with upstream
"aws-auth-crazy-case-keys.yaml", validRoleMappings, validUserMappings, validAWSAccounts, false,
},
{
// roleARN instead of rolearn
// parsing succeeds, values are case-insensitive for compatibility with upstream
"aws-auth-open-source-case-keys.yaml", validRoleMappings, validUserMappings, validAWSAccounts, false,
},
// Fail cases -- ideally, validation should reject these before they reach us
{
// mapusers instead of mapUsers
Expand Down Expand Up @@ -94,6 +89,26 @@ func TestConfigMap(t *testing.T) {
"aws-auth-missing-bar.yaml", nil, nil, nil, true,
},
}
if !EKSYaml {
tests = append(tests, []struct {
configMapYaml string
expectedRoleMappings []config.RoleMapping
expectedUserMappings []config.UserMapping
expectedAWSAccounts map[string]bool
expectCreateError bool
}{
{
// RoLeArN instead of rolearn
// parsing succeeds, values are case-insensitive for compatibility with upstream
"aws-auth-crazy-case-keys.yaml", validRoleMappings, validUserMappings, validAWSAccounts, false,
},
{
// roleARN instead of rolearn
// parsing succeeds, values are case-insensitive for compatibility with upstream
"aws-auth-open-source-case-keys.yaml", validRoleMappings, validUserMappings, validAWSAccounts, false,
},
}...)
}

for _, tt := range tests {
t.Run(tt.configMapYaml, func(t *testing.T) {
Expand Down