Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Allow setting annotations on service accounts #964

Conversation

cooperbenson-qz
Copy link
Contributor

@cooperbenson-qz cooperbenson-qz commented May 21, 2021

Fixes #235.

Changes proposed in this PR:
This is a redux of #236

  • Added values for service account annotations
  • These values expect a multi-line YAML string of the annotations map

How I've tested this PR:

  • Unit tests and some test renders

How I expect reviewers to test this PR:

  • Same

Checklist:

  • Bats tests added

@hashicorp-cla
Copy link

hashicorp-cla commented May 21, 2021

CLA assistant check
All committers have signed the CLA.

@cooperbenson-qz
Copy link
Contributor Author

Here's the full context on why I decided to limit the service accounts I added this feature for. I started adding annotations to all of the service accounts but I ran into issues with two which lead me to rethink the scope:

  • templates/connect-inject-authmethod-serviceaccount.yaml - I couldn't work out what this was being used for and so was having a hard time working out where in the values structure it should go
  • templates/create-federation-secret-serviceaccount.yaml - The job is currently configured with a boolean global.federation.createFederationSecret. I didn't want to break the values API by changing that to be global.federation.createFederationSecret.enabled and adding global.federation.createFederationSecret.serviceAccount.annotations. The alternative would be to add global.federation.createFederationSecretServiceAccountAnnotations, but that felt like a bad design choice. This will also likely be a problem for all of the other bootsrapping jobs (ACL and TLS in particular).

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Thanks so much for following up on this @cooperbenson-qz! Can you go ahead and sign the CLA, and could we use the same format as other annotations in the helm chart for consistency?

values.yaml Outdated Show resolved Hide resolved
@cooperbenson-qz
Copy link
Contributor Author

I'm still waiting on our legal team to review the CLA so I'll work on adding more annotations

@cooperbenson-qz
Copy link
Contributor Author

Added annotations for all service accounts except for the following. All of them lack a suitable area in the values file to add the option to. I can add them if I can get some help determining the correct place to add values for them.

  • templates/connect-inject-authmethod-serviceaccount.yaml
  • templates/webhook-cert-manager-serviceaccount.yaml
  • Jobs:
    • templates/create-federation-secret-serviceaccount.yaml
    • templates/enterprise-license-serviceaccount.yaml
    • templates/server-acl-init-cleanup-serviceaccount.yaml
    • templates/server-acl-init-serviceaccount.yaml
    • templates/tls-init-cleanup-serviceaccount.yaml
    • templates/tls-init-serviceaccount.yaml

@cooperbenson-qz cooperbenson-qz changed the title Allow setting annotations on client and server service accounts Allow setting annotations on service accounts May 24, 2021
@cooperbenson-qz
Copy link
Contributor Author

@ndhanushkodi mind taking another look now that the CLA is signed? Really eager to get this merged 😃

@david-yu
Copy link
Contributor

david-yu commented May 26, 2021

Hi @cooperbenson-qz thank you for signing the CLA and the updating the PR! We are going to do a release shortly for our 1.10 beta3 so we will wait to review this until after the release is done this week. Stay tuned.

@cooperbenson-qz
Copy link
Contributor Author

Just wanted to check in since it's been a bit. Still would love to see this merged soon 🙂

Copy link
Contributor

@ndhanushkodi ndhanushkodi left a comment

Choose a reason for hiding this comment

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

Thank you for making the changes! It looks great!! Sorry for the late review

@ndhanushkodi ndhanushkodi requested review from kschoche and removed request for ishustava June 7, 2021 16:55
Copy link
Contributor

@kschoche kschoche left a comment

Choose a reason for hiding this comment

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

Thanks for adding this, it looks great!

@cooperbenson-qz
Copy link
Contributor Author

Thanks for the reviews 😄 When you merge, it would be great if you could also close #236

@ndhanushkodi ndhanushkodi merged commit 5f367ca into hashicorp:master Jun 7, 2021
@cooperbenson-qz cooperbenson-qz deleted the fix-235-service-account-annotations branch June 7, 2021 17:13
ndhanushkodi added a commit that referenced this pull request Jun 7, 2021
ndhanushkodi added a commit that referenced this pull request Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for annotations on ServiceAccounts
5 participants