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.

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

Make Ansible Operator parameter case conversion optional #3245

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions changelog/fragments/ansible-operator-optional-case.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
entries:
- description: >
The `snakeCaseParameters` option has been added to the `watches.yaml` for
Ansible-based Operators. This allows the user to configure whether parameters
in the resource spec are automatically converted from camelCase to snake_case.
The default is `true`, so there is no behavior change for existing operators,
but it can now be disabled.

kind: "addition"
breaking: false
45 changes: 28 additions & 17 deletions pkg/ansible/runner/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,27 +131,29 @@ func New(watch watches.Watch) (Runner, error) {
}

return &runner{
Path: path,
cmdFunc: cmdFunc,
Vars: watch.Vars,
Finalizer: watch.Finalizer,
finalizerCmdFunc: finalizerCmdFunc,
GVK: watch.GroupVersionKind,
maxRunnerArtifacts: watch.MaxRunnerArtifacts,
ansibleVerbosity: watch.AnsibleVerbosity,
Path: path,
cmdFunc: cmdFunc,
Vars: watch.Vars,
Finalizer: watch.Finalizer,
finalizerCmdFunc: finalizerCmdFunc,
GVK: watch.GroupVersionKind,
maxRunnerArtifacts: watch.MaxRunnerArtifacts,
ansibleVerbosity: watch.AnsibleVerbosity,
snakeCaseParameters: watch.SnakeCaseParameters,
}, nil
}

// runner - implements the Runner interface for a GVK that's being watched.
type runner struct {
Path string // path on disk to a playbook or role depending on what cmdFunc expects
GVK schema.GroupVersionKind // GVK being watched that corresponds to the Path
Finalizer *watches.Finalizer
Vars map[string]interface{}
cmdFunc cmdFuncType // returns a Cmd that runs ansible-runner
finalizerCmdFunc cmdFuncType
maxRunnerArtifacts int
ansibleVerbosity int
Path string // path on disk to a playbook or role depending on what cmdFunc expects
GVK schema.GroupVersionKind // GVK being watched that corresponds to the Path
Finalizer *watches.Finalizer
Vars map[string]interface{}
cmdFunc cmdFuncType // returns a Cmd that runs ansible-runner
finalizerCmdFunc cmdFuncType
maxRunnerArtifacts int
ansibleVerbosity int
snakeCaseParameters bool
}

func (r *runner) Run(ident string, u *unstructured.Unstructured, kubeconfig string) (RunResult, error) {
Expand Down Expand Up @@ -307,7 +309,16 @@ func (r *runner) makeParameters(u *unstructured.Unstructured) map[string]interfa
spec = map[string]interface{}{}
}

parameters := paramconv.MapToSnake(spec)
parameters := map[string]interface{}{}

if r.snakeCaseParameters {
parameters = paramconv.MapToSnake(spec)
} else {
for k, v := range spec {
parameters[k] = v
}
}
camilamacedo86 marked this conversation as resolved.
Show resolved Hide resolved

parameters["meta"] = map[string]string{"namespace": u.GetNamespace(), "name": u.GetName()}

objKey := escapeAnsibleKey(fmt.Sprintf("_%v_%v", r.GVK.Group, strings.ToLower(r.GVK.Kind)))
Expand Down
11 changes: 10 additions & 1 deletion pkg/ansible/watches/watches.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ type Watch struct {
ManageStatus bool `yaml:"manageStatus"`
WatchDependentResources bool `yaml:"watchDependentResources"`
WatchClusterScopedResources bool `yaml:"watchClusterScopedResources"`
SnakeCaseParameters bool `yaml:"snakeCaseParameters"`
Copy link
Contributor

Choose a reason for hiding this comment

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

The issue faced in the lint is maligned. To solve it you can just keep the bools at the ends. Move the Selector before of them.


To know more about it see: https://medium.com/@sebassegros/golang-dealing-with-maligned-structs-9b77bacf4b97
Running golangci-lint
pkg/ansible/watches/watches.go:44:12: struct of size 200 bytes could be of size 192 bytes (maligned)
type Watch struct {
           ^
Makefile:89: recipe for target 'lint' failed
make: *** [lint] Error 1
The command "make test-sanity" exited with 2.
cache.2
store build cache

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm speechless at this lint check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@camilamacedo86 Thanks for the pointer. I didn't realize that Go is so hypercritical and nitpicking ... and I'm not sure if it is always a good thing. But anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarifies, the maligned is a lint check. It checks the usage of memory space of a structure and alert when we are using more spaces than required. Why it would be a bad thing at all?

However, in the cases that we decide to prioritize the code understatement instead of it, we can just say for the lint ignore the specific check in the specific place by using // nolint:maligned

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is always the right thing to do, the compiler should do this byte alignment transparently rather than forcing the developer to think about these details. There are other cases I want to order my fields for different purposes, e.g. semantic grouping, deriving Web UI for an input struct etc.

Selector metav1.LabelSelector `yaml:"selector"`

// Not configurable via watches.yaml
Expand All @@ -73,6 +74,7 @@ var (
manageStatusDefault = true
watchDependentResourcesDefault = true
watchClusterScopedResourcesDefault = false
snakeCaseParametersDefault = true
selectorDefault = metav1.LabelSelector{}

// these are overridden by cmdline flags
Expand Down Expand Up @@ -123,14 +125,14 @@ type alias struct {
ManageStatus *bool `yaml:"manageStatus,omitempty"`
WatchDependentResources *bool `yaml:"watchDependentResources,omitempty"`
WatchClusterScopedResources *bool `yaml:"watchClusterScopedResources,omitempty"`
SnakeCaseParameters *bool `yaml:"snakeCaseParameters"`
Blacklist []schema.GroupVersionKind `yaml:"blacklist,omitempty"`
Finalizer *Finalizer `yaml:"finalizer"`
Selector tempLabelSelector `yaml:"selector"`
}

// buildWatch will build Watch based on the values parsed from alias
func (w *Watch) setValuesFromAlias(tmp alias) error {

// by default, the operator will manage status and watch dependent resources
if tmp.ManageStatus == nil {
tmp.ManageStatus = &manageStatusDefault
Expand All @@ -150,10 +152,15 @@ func (w *Watch) setValuesFromAlias(tmp alias) error {
if tmp.WatchClusterScopedResources == nil {
tmp.WatchClusterScopedResources = &watchClusterScopedResourcesDefault
}

if tmp.Blacklist == nil {
tmp.Blacklist = blacklistDefault
}

if tmp.SnakeCaseParameters == nil {
tmp.SnakeCaseParameters = &snakeCaseParametersDefault
}

gvk := schema.GroupVersionKind{
Group: tmp.Group,
Version: tmp.Version,
Expand All @@ -174,6 +181,7 @@ func (w *Watch) setValuesFromAlias(tmp alias) error {
w.ReconcilePeriod = tmp.ReconcilePeriod.Duration
w.ManageStatus = *tmp.ManageStatus
w.WatchDependentResources = *tmp.WatchDependentResources
w.SnakeCaseParameters = *tmp.SnakeCaseParameters
w.WatchClusterScopedResources = *tmp.WatchClusterScopedResources
w.Finalizer = tmp.Finalizer
w.AnsibleVerbosity = getAnsibleVerbosity(gvk, ansibleVerbosityDefault)
Expand Down Expand Up @@ -301,6 +309,7 @@ func New(gvk schema.GroupVersionKind, role, playbook string, vars map[string]int
ManageStatus: manageStatusDefault,
WatchDependentResources: watchDependentResourcesDefault,
WatchClusterScopedResources: watchClusterScopedResourcesDefault,
SnakeCaseParameters: snakeCaseParametersDefault,
Finalizer: finalizer,
AnsibleVerbosity: ansibleVerbosityDefault,
Selector: selectorDefault,
Expand Down
6 changes: 6 additions & 0 deletions pkg/ansible/watches/watches_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ func TestNew(t *testing.T) {
t.Fatalf("Unexpected watchDependentResources %v expected %v", watch.WatchDependentResources,
watchDependentResourcesDefault)
}
if watch.SnakeCaseParameters != snakeCaseParametersDefault {
t.Fatalf("Unexpected snakeCaseParameters %v expected %v", watch.SnakeCaseParameters,
snakeCaseParametersDefault)
}
if watch.WatchClusterScopedResources != watchClusterScopedResourcesDefault {
t.Fatalf("Unexpected watchClusterScopedResources %v expected %v",
watch.WatchClusterScopedResources, watchClusterScopedResourcesDefault)
Expand Down Expand Up @@ -143,6 +147,7 @@ func TestLoad(t *testing.T) {
ReconcilePeriod: twoSeconds,
WatchDependentResources: true,
WatchClusterScopedResources: false,
SnakeCaseParameters: true,
},
Watch{
GroupVersionKind: schema.GroupVersionKind{
Expand All @@ -153,6 +158,7 @@ func TestLoad(t *testing.T) {
Playbook: validTemplate.ValidPlaybook,
ManageStatus: true,
WatchDependentResources: true,
SnakeCaseParameters: false,
WatchClusterScopedResources: false,
Finalizer: &Finalizer{
Name: "finalizer.app.example.com",
Expand Down
22 changes: 22 additions & 0 deletions test/ansible/deploy/crds/test.example.com_casetest_crd.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: casetests.test.example.com
spec:
group: test.example.com
names:
kind: CaseTest
listKind: CaseTestList
plural: casetests
singular: casetest
scope: Namespaced
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
type: object
x-kubernetes-preserve-unknown-fields: true
served: true
storage: true
subresources:
status: {}
25 changes: 25 additions & 0 deletions test/ansible/molecule/cluster/tasks/case_test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
---
- name: Create the test.example.com/v1alpha1.CaseTest
k8s:
state: present
definition:
apiVersion: test.example.com/v1alpha1
kind: CaseTest
metadata:
name: case-test
namespace: '{{ namespace }}'
spec:
camelCaseVar: "true"
wait: yes
wait_timeout: 300
wait_condition:
type: Running
reason: Successful
status: "True"
register: case_test

- name: Assert sentinel ConfigMap has been created for Molecule Test
assert:
that: cm.data.shouldBeCamel == 'true'
vars:
cm: "{{ q('k8s', api_version='v1', kind='ConfigMap', namespace=namespace, resource_name='case-test').0 }}"
17 changes: 17 additions & 0 deletions test/ansible/playbooks/case.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
---
- hosts: localhost
gather_facts: no
collections:
- community.kubernetes

tasks:
- name: Create configmap
k8s:
definition:
apiVersion: v1
kind: ConfigMap
metadata:
name: '{{ meta.name }}'
namespace: '{{ meta.namespace }}'
data:
shouldBeCamel: '{{ camelCaseVar | default("false") }}'
1 change: 0 additions & 1 deletion test/ansible/playbooks/secret.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,3 @@
data:
'{{ item.key }}': '{{ item.value | b64decode }}'
with_dict: '{{ __secret.data }}'

5 changes: 5 additions & 0 deletions test/ansible/watches.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,8 @@
matchExpressions:
- {key: testLabel, operator: Exists, values: []}

- version: v1alpha1
group: test.example.com
kind: CaseTest
playbook: playbooks/case.yml
snakeCaseParameters: false
3 changes: 2 additions & 1 deletion website/content/en/docs/ansible/quickstart.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,8 @@ Resource spec field along to Ansible as extra
[variables](https://docs.ansible.com/ansible/2.5/user_guide/playbooks_variables.html#passing-variables-on-the-command-line).
The names of all variables in the spec field are converted to snake_case
by the operator before running ansible. For example, `serviceAccount` in
the spec becomes `service_account` in ansible.
the spec becomes `service_account` in ansible. You can disable this case conversion
by setting the `snakeCaseParameters` option to `false` in your `watches.yaml`.
It is recommended that you perform some type validation in Ansible on the
variables to ensure that your application is receiving expected input.

Expand Down
4 changes: 4 additions & 0 deletions website/content/en/docs/ansible/reference/watches.md
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ Some features can be overridden per resource via an annotation on that CR. The o
| Max Runner Artifacts | `maxRunnerArtifacts` | Manages the number of [artifact directories](https://ansible-runner.readthedocs.io/en/latest/intro.html#runner-artifacts-directory-hierarchy) that ansible runner will keep in the operator container for each individual resource. | ansible.operator-sdk/max-runner-artifacts | 20 | |
| Finalizer | `finalizer` | Sets a finalizer on the CR and maps a deletion event to a playbook or role | | | [finalizers](../finalizers)|
| Selector | `selector` | Identifies a set of objects based on their labels | | None Applied | [Labels and Selectors](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/)|
| Automatic Case Conversion | `snakeCaseParameters` | Determines whether to convert the CR spec from camelCase to snake_case before passing the contents to Ansible as extra_vars| | true | |


#### Example
```YaML
---
Expand All @@ -117,6 +120,7 @@ Some features can be overridden per resource via an annotation on that CR. The o
reconcilePeriod: 5s
manageStatus: False
watchDependentResources: False
snakeCaseParameters: False
finalizer:
name: finalizer.app.example.com
vars:
Expand Down