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

Allow for multiple service accounts to have ImagePullSecrets applied #6

Closed
wants to merge 2 commits into from
Closed

Allow for multiple service accounts to have ImagePullSecrets applied #6

wants to merge 2 commits into from

Conversation

davidcollom
Copy link

@davidcollom davidcollom commented Oct 9, 2020 •
edited by alexellis
Loading

Description

Allow for ImagePullSecrets to be applied to other Service Accounts than the default.

Which issue # does this fix? And was it approved before you worked on it?

No Issue, however a feature I felt worth while.

Fixes: #7

How Has This Been Tested?

Multiple times on my own cluster.
Scenarios Tried:

  • New NS = Default Changed
  • New SA w/o NS Ignore Annotation = No Change
  • New SA w/ NS Ignore Annotation = No Change
  • New SA w/o NS ignore Annotation and SA annotation added (Invalid) = No Change
  • New SA w/o NS ignore Annotation and SA annotation added (valid) = PullSecret added

How are existing users impacted? What migration steps/scripts do we need?

No change, the default logic of the default SA is still valid.
Only when users add the alexellis.io/registry-creds.include annotation to their ServiceAccounts will the ImagePullSecret be added.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • read the CONTRIBUTION guide
  • signed-off my commits with git commit -s
  • added unit tests (No Present within current repo)

@derek derek bot added the new-contributor label Oct 9, 2020
@alexellis
Copy link
Owner

Hi David,

Thank you for your interest in the project. The contribution process is something that everyone needs to follow. I can see you've filled out part of the PR template, so that's something. The testing is also appreciated.

Which issue # does this fix? And was it approved before you worked on it?

When you just put "no, but I wanted to" it doesn't create a good first impression, but I want to give you benefit of the doubt.

So please can you raise an issue and start a discussion about why this is needed.

Alex

@davidcollom
Copy link
Author

Hi Alex,
Apologies I hit submit too early 🤦 on that section, I should have expanded on this a little further.

The use case for this, was that I have multiple namespaces and deployments all using their own ServiceAccounts, while the original controller works with a many pull policies to one service account, my contribution allows for a Many:Many relationship of ImagePullPolicies to many ServiceAccounts.

A better example would be that a helmchart in which requires no ability to customise the ServiceAccount in which is used (enforced new SA) - I'm unable to pull images from a private repository.

With the patch/contribution applied, I'm able to patch said SA with the annotation alexellis.io/registry-creds.include=CLUSTERPULLSECRETNAME and now able to pull from said private repository.

@davidcollom davidcollom mentioned this pull request Oct 9, 2020
Closed
@alexellis
Copy link
Owner

@davidcollom how similar is your change to the following in #4?

https://github.com/alexellis/registry-creds/pull/4/files#diff-cace653417c1c5f437a99e61b744b78680b56528d614d2ef3092494ee8abf49cR78-R91

alexellis
alexellis reviewed Oct 29, 2020
View reviewed changes
controllers/secret_reconciler.go
return nil
}

r.Log.Info(fmt.Sprintf("Getting SA for: %v", namespaceName))
r.Log.Info(fmt.Sprintf("Getting ServiceAccount: %v", namespacedName.Namespace))
Copy link
Owner

Choose a reason for hiding this comment

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

Wouldn't this print the wrong info? "Getting ServiceAccount NAMESPACE"

@alexellis
Copy link
Owner

I'm uncomfortable with the additional number of lines added to secret_reconciler.go, it makes it very difficult to review and follow what is going on. Can you consider breaking some of that out into utility methods, and to remove nested expressions wherever you can? i.e. if you have an if/else and can return from the first condition, then do so and let the else be unindented as an expression.

@davidcollom
Copy link
Author

Sure, I can take a look this weekend or later this week.

With regards to #4, these changes are very similar, #4 does have the addition of:

Upon ClusterPullSecret removal, the secret is removed from all namespaces and serviceaccounts

Which is not included within my changes.

alexellis added a commit that referenced this pull request Oct 29, 2020
@alexellis
Add comments, extract methods for secret creation
44d42b0 
In light of recent PRs: #4 and #6, this PR aims to break
out code so that contributors can take the style for
inspiration.

Breaking out methods also makes the code self-describe.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@alexellis alexellis mentioned this pull request Oct 29, 2020
Merged
alexellis added a commit that referenced this pull request Oct 29, 2020
@alexellis
Add comments, extract methods for secret creation
9fc0a91 
In light of recent PRs: #4 and #6, this PR aims to break
out code so that contributors can take the style for
inspiration.

Breaking out methods also makes the code self-describe.

Signed-off-by: Alex Ellis (OpenFaaS Ltd) <alexellis2@gmail.com>
@davidcollom
Allow for multiple service accounts to have ImagePullSecrets applied
9977c3f 
Signed-off-by: David Collom <david@collom.co.uk>
@davidcollom davidcollom force-pushed the allow-multiple-service-accounts branch from 4a8d1be to 458a8e9 Compare October 30, 2020 12:43
@derek derek bot added the no-dco label Oct 30, 2020
@derek
Copy link

derek bot commented Oct 30, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@davidcollom
Removal of excessive RBAC - create/delete namespaces and serviceaccounts
452438b 
Signed-off-by: David Collom <david.collom@jetstack.io>
@davidcollom davidcollom force-pushed the allow-multiple-service-accounts branch from 458a8e9 to 452438b Compare October 30, 2020 12:45
@derek derek bot removed the no-dco label Oct 30, 2020
@alexellis alexellis closed this Nov 12, 2020
@alexellis
Copy link
Owner

Let's start over with a new PR that makes fewer, smaller changes.

@davidcollom
Copy link
Author

Sorry for the lack of updates, I've been working on a new PR to fix this issue. I hope to get something ready for review this week, early next.

@alexellis
Copy link
Owner

We should have this covered now by the work @developer-guy has been doing. We still need something to clean up image pull secret lists after secrets have been removed.

@davidcollom davidcollom deleted the allow-multiple-service-accounts branch November 12, 2020 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexellis alexellis alexellis left review comments

Assignees
No one assigned
Labels
new-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow for multiple service accounts to have ImagePullSecrets applied
2 participants
@davidcollom @alexellis