Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Add ServiceAccount annotations #686

Merged
merged 5 commits into from
Jun 26, 2020
Merged

Add ServiceAccount annotations #686

merged 5 commits into from
Jun 26, 2020

Conversation

jim-barber-he
Copy link
Contributor

@jim-barber-he jim-barber-he commented Jun 24, 2020

Add the ability to add annotations to the service accounts created for
logstash and elasticsearch.

Addresses: #627

  • Chart version not bumped (the versions are all bumped and released at the same time)
  • README.md updated with any new values or changes
  • Updated template tests in ${CHART}/tests/*.py
  • Updated integration tests in ${CHART}/examples/*/test/goss.yaml

Add the ability to add annotations to the service accounts created for
logstash and elasticsearch.

Addresses: #627
@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@cla-checker-service
Copy link

cla-checker-service bot commented Jun 24, 2020

💚 CLA has been signed

@jim-barber-he
Copy link
Contributor Author

jim-barber-he commented Jun 24, 2020

I didn't update the README.md files because the description of the rbac field seems suitable still.

I don't know how to add tests for the changes I've made.

@fatmcgav
Copy link
Contributor

Jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 25, 2020

jenkins test this please

1 similar comment
@jmlrt
Copy link
Member

jmlrt commented Jun 25, 2020

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 25, 2020

Thanks for this PR @jim-barber-he,
It could be worse adding ServiceAccount annotations to apm-server, filebeat, kibana and metricbeat charts also.
Would you be interested to do it in this PR?

I don't know how to add tests for the changes I've made.

A test like this one could be added

def test_adding_multiple_persistence_annotations():
config = """
persistence:
enabled: true
annotations:
hello: world
world: hello
"""
r = helm_template(config)
annotations = r["statefulset"][name]["spec"]["volumeClaimTemplates"][0]["metadata"][
"annotations"
]
assert annotations["hello"] == "world"
assert annotations["world"] == "hello"

@jim-barber-he
Copy link
Contributor Author

@jmlrt I'm happy to do annotations for those other charts.
The only reason I didn't is they take a different approach to how they were getting their names, and didn't really know what the best value name to use would be.
I guess I could introduce a .Values.serviceAccountAnnotations value; does that sound suitable?

Originally I submitted this as a feature request, but had a go at it based on @fatmcgav telling me it should be simple.
I didn't realise I'd be having to work out how to write tests so I'm going to need more help...
For the logstash chart where I've added the following to the top-level metadata key:

  annotations:
    {{- with .Values.rbac.serviceAccountAnnotations }}
    {{- toYaml . | nindent 4 }}
    {{- end }}

Would the test be something like the following?

def test_adding_serviceaccount_annotations():
    config = """
rbac:
  serviceAccountAnnotations:
    eks.amazonaws.com/role-arn: arn:aws:iam::111111111111:role/k8s.clustername.elasticsearch.logstash
"""
    r = helm_template(config)
    assert (
        r["serviceaccount"][name]["metadata"]["annotations"][
            "eks.amazonaws.com/role-arn"
        ]
        == "arn:aws:iam::111111111111:role/k8s.clustername.elasticsearch.logstash"
    )

- Add the ability to annotate the service account for `apm-server`,
`filebeat`, and `metricbeat`

- Add tests for the new annotations.
@jim-barber-he
Copy link
Contributor Author

In the interests of keeping things moving I've added tests that I hope will work.
Also I've added the ability to add annotations to the apm-server, filebeat, and metricbeat charts as requested by @jmlrt
The kibana chart does not create its own service account and so I didn't do it for that one.

@jmlrt
Copy link
Member

jmlrt commented Jun 26, 2020

jenkins test this please

Copy link
Member

@jmlrt jmlrt left a comment

Choose a reason for hiding this comment

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

A few changes are required for the tests:

  • Elasticsearch and Logstash chart require rbac.create=true to create the service accounts
  • Elasticsearch chart use uname instead of name

elasticsearch/tests/elasticsearch_test.py Show resolved Hide resolved
elasticsearch/tests/elasticsearch_test.py Outdated Show resolved Hide resolved
logstash/tests/logstash_test.py Show resolved Hide resolved
@jmlrt
Copy link
Member

jmlrt commented Jun 26, 2020

jenkins test this please

@jmlrt
Copy link
Member

jmlrt commented Jun 26, 2020

I guess I could introduce a .Values.serviceAccountAnnotations value; does that sound suitable?

.Values.serviceAccountAnnotations is very suitable. I'd like to standardize values naming and the way we do thing across all the charts at some time but meanwhile having the same features in all charts when relevant is already really good.

I didn't realise I'd be having to work out how to write tests so I'm going to need more help...

Your tests were mostly good, a few changes were required, I commited so we can merge your work soon 🤞

@jmlrt jmlrt merged commit 175325d into elastic:master Jun 26, 2020
jmlrt added a commit that referenced this pull request Jun 26, 2020
jmlrt added a commit that referenced this pull request Jun 26, 2020
jmlrt added a commit that referenced this pull request Jun 26, 2020
@jmlrt
Copy link
Member

jmlrt commented Jun 26, 2020

backported to 6.8, 7.8and 7.x branches

@jim-barber-he
Copy link
Contributor Author

Awesome. Thank you very much.

This was referenced Jul 16, 2020
This was referenced Jul 27, 2020
@jmlrt jmlrt mentioned this pull request Oct 28, 2020
This was referenced Nov 17, 2020
@jmlrt jmlrt mentioned this pull request Feb 8, 2021
This was referenced Mar 15, 2021
@jmlrt jmlrt mentioned this pull request May 25, 2021
@jmlrt jmlrt mentioned this pull request Mar 8, 2022
@jmlrt jmlrt mentioned this pull request Apr 21, 2022
This was referenced Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants