Helm feature #54
Helm feature #54
Conversation
sophsoph321
force-pushed
the
helm
branch
5 times, most recently
from
530d414 to
17bc520
Compare
Tatsinnit
requested review from
Tatsinnit,
JunSun17,
arnaud-tincelin and
davidkydd
|
💡 Thank you so much for this new PR, I have a feeling that This PR over-rides the old PR: #51 please feel free to close the Old PR. Please let me know once this PR is ready for review? Thanks. |
|
@Tatsinnit so sorry for not getting back in advance! Yes, I have opened this PR so you and the periscope team can review it, it is no longer a draft. I will close PR #51 since this one has the latest changes. Thanks for your attention. |
There was a problem hiding this comment.
💡 Thank you Sophie, few things as a checklist to make sure you have covered existing behaviours are:
-
Make sure you locally run CI.
-
Tip to locally test changes via image reside here: https://github.com/Azure/aks-periscope#programming-guide
- Above will be a very important scenario to test, and feel free to reach out to us for anything.
-
If you find any new functionality could be easily extracted to UT (unit test) then feel enabled to do so. (We could make it part of the CI as well)
-
Once we have done all the above, we should have enough confidence to open PR for review.
Really appreciate it. 🙏
sophsoph321
force-pushed
the
helm
branch
3 times, most recently
from
31e5a04 to
ddd64fb
Compare
pkg/collector/helm_collector.go
Outdated
| return err | ||
| } | ||
|
|
||
| output, err := utils.RunCommandOnContainer("apk", "add", "curl", "openssl", "bash", "--no-cache") |
There was a problem hiding this comment.
💡 🐛 I plan to add PR to your fork tonight, but see I noticed something interesting:
:= is Short variable declaration, but if you see carefully, up until line 53 you don't do anything with the output var.
I can make small updates and send small PR to your fort by tonight. Thanks.
sophsoph321
force-pushed
the
helm
branch
2 times, most recently
from
d7df0c5 to
8a93271
Compare
pkg/collector/helm_collector.go
Outdated
| } | ||
|
|
||
| helmListFile := filepath.Join(rootPath, "helm_list") | ||
| output, err = utils.RunCommandOnContainer("helm", "list", "--all-namespaces") |
There was a problem hiding this comment.
🐛 More details here: #54 (comment)
There was a problem hiding this comment.
💡 Let's just dig a little deeper for this: I think adding secrets to the list might be ok, and non-impacting, as this all runs with users permission and user in use will run it and that might enable the Arc - helm scenarios as well.
There was a problem hiding this comment.
Lets add the secrets in resources and keep 1 yaml, I am not able to find out any issues with it, inc are we did we can always have a fallback idea anyways. Thank you. here is the small PR for it: sophsoph321#1 (Mainly to simplify all this)
There was a problem hiding this comment.
This looks like a very risky set of permissions to me? This creates a ClusterRole with list permissions for secrets across all namespaces in the cluster.
What is this needed for? Is there any way we can scope the permissions down to e.g. enable secret get to just that secret which is required?
e.g. number 1 risky permission here: https://www.cyberark.com/resources/threat-research-blog/securing-kubernetes-clusters-by-eliminating-risky-permissions and description of using get instead of list
https://darkbit.io/blog/the-power-of-kubernetes-rbac-list
There was a problem hiding this comment.
@davidkydd The reasoning comes from here: https://helm.sh/docs/topics/rbac/#example-grant-a-user-read-only-access-at-the-cluster-scope
If you go all the way at the bottom of that link above, it says in order to run "helm list --all-namespaces", the user needs cluster-scope read access, which requires having both view and secret-reader access. I discussed this internally with arc team, and we couldn't figure out a different way other than this to work around the issues Tats was having with running "helm list".
There was a problem hiding this comment.
not due to something experienced in the past , just jumped out as a red flag. Also note that while CLI deployment of periscope will be cleaned up once complete, non-CLI driven deployments aren't cleaned up automatically, and would potentially leave this dangling clusterRole.
Assumed you would be able to at least scope the secrets you need to read down so there wouldn't be need for full "list" permission on a clusterRole, but from the link @sophsoph321 posted it doesn't look like it. If this is already at least-privilege possible for the commands the helm collector needs to run then think thats probably fine.
As we will likely encounter with the CRD permissions - it seems we are forced to either:
- introduce additional complexity (Arc CLI has to update roleBinding after deploy), or
- violate least privilege by giving all flavours of deployments the full super-set of required permissions (AKS deployments of periscope will end up with CRD + secrets list permissions when it makes no use of them)
Again, a problem which could be avoided by using a different config / deployment / yaml file for each integration. If we aren't prepared to do that then I suggest violating least privilege and avoiding the need for additional complexity in the CLI.
There was a problem hiding this comment.
Yeah, initial thought process to be careful was exactly what I thought, but essentially the use case for this tool will not have any security impact to the behaviour.
User will always run this tool on their known cluster context. I would see it as Helm enablement requirement since this issue common with Helm command.
To get more idea/opinion, I reached out to @JunSun17 (Thank you for reply) - as long as we are not logging any secrets in an exported file, it should be good. I think the tool can access secrets and use it in the program to perform some logic, just make sure the secret values are not written out when exporting, or it will be exposed.
Further, @safeermohammed kindly agrees to stay involved and if there is any need be Arc will need to change/own for plan-b and we can together make move forward.
Side note: I would say, we can go ahead given we have captured all the opinion and have plan-b in case we find any concrete scenario. Also, if this at all is any security bug in Helm then it will be helm issue and in that case I would rather prefer to disable the whole helm command.
Thanks guys. 🙏
There was a problem hiding this comment.
Thanks @Tatsinnit - my concern was mostly driven by my own naivety re: k8s security :)
Believe this can be resolved if nothing else from Arc side
There was a problem hiding this comment.
@Tatsinnit thanks for summarizing this! I totally agree with you on the security side.
There was a problem hiding this comment.
As a quick note regarding "just make sure the secret values are not written out when exporting, or it will be exposed." Secrets will be exported out, but only those corresponding to helm releases, which is by design
pkg/collector/helm_collector.go
Outdated
| collector.AddToCollectorFiles(helmListFile) | ||
|
|
||
| helmHistoryFile := filepath.Join(rootPath, "helm_history") | ||
| output, err = utils.RunCommandOnContainer("helm", "history", "-n", "default", "azure-arc") |
There was a problem hiding this comment.
Rather than adding some packages on the Docker image and executing command lines from our code, could we use libraries instead ?
https://github.com/helm/helm/blob/0af54d0f5c0f2a108506733b64752039ee39d165/cmd/helm/chart_list.go#L33
https://github.com/helm/helm/blob/0af54d0f5c0f2a108506733b64752039ee39d165/cmd/helm/history.go#L54
That way we can have better error handling, smaller docker image and less external dependencies, which means less error at runtime.
Last point: it is best practice to create new variables rather than doing variables reuse (output, err = )
There was a problem hiding this comment.
Thank you @arnaud-tincelin, for taking the time to review my changes, these are very good suggestions.
Also tagging relevant people: @davidkydd @safeermohammed @Tatsinnit
Regarding the suggestion about utilizing helm library, one thing to keep in mind is that we, the arc team, are trying to get an MVP out by end of May (a.k.a. next week) which will include this new helm feature for periscope. Refactoring the helm classes so that it uses library functions seems to be a big change and the changes that I have so far have already been tested extensively.
After discussing offline with the periscope team, we had a suggestion for periscope team to make the collectors use library functions even for the kubectl commands in the code. So the helm collector can be taken care of as part of this refactoring.
For now, given the short amount of time we have left until the MVP needs to be released, my thinking is to freeze the code changes for the time being. But of course, I will do my best to fix the other smaller changes such as creating the new variables and combining the lines in the Dockerfile.
Please let me know your thoughts on this.
There was a problem hiding this comment.
Later refactoring to utilize helm + kubectl from libs rather than invoking executables sounds good to me
There was a problem hiding this comment.
+1, yes we should do it as part of refactor, I will add a workitem with more details by eod.
I will test any new changes sometime Monday our time or earlier. (Will kee you guys posted)
thanks guys 🙏
Thank you for noticing this @arnaud-tincelin , @sophsoph321 if you notice any old item in issue log, it seems you have a commit which references every old issue in this repo issue log. (Which is incorrect) For example go to the issue log and open any old item and you will see your commit referencing that workitem. Let’s fix this. like: Thanks, 🙏 |
# This is the 1st commit message: Revert "introduce arcmode" This reverts commit 5f4fed4. # This is the commit message #2: remove secrets # This is the commit message Azure#3: add print statement # This is the commit message Azure#4: update print statement # This is the commit message Azure#5: committed # This is the commit message Azure#6: committed # This is the commit message Azure#7: committed # This is the commit message Azure#8: committed # This is the commit message Azure#9: remove print statements # This is the commit message Azure#10: add helm collector # This is the commit message Azure#11: change helm command # This is the commit message Azure#12: add helm 3 installation # This is the commit message Azure#13: add curl command installation # This is the commit message Azure#14: change helm command # This is the commit message Azure#15: remove helm history # This is the commit message Azure#16: debug helm history # This is the commit message Azure#17: add repo command # This is the commit message Azure#18: change stable repo name # This is the commit message Azure#19: add write to file # This is the commit message Azure#20: add kured # This is the commit message Azure#21: change # This is the commit message Azure#22: changes # This is the commit message Azure#23: add default namespace # This is the commit message Azure#24: change # This is the commit message Azure#25: add integration test # This is the commit message Azure#26: changes # This is the commit message Azure#27: add helm test # This is the commit message Azure#28: change print statement to error # This is the commit message Azure#29: change # This is the commit message Azure#30: more changes # This is the commit message Azure#31: add go installation # This is the commit message Azure#32: fix unit test # This is the commit message Azure#33: iptables to Helm # This is the commit message Azure#34: add custom resource collector # This is the commit message Azure#35: add new exporter, diagnoser, collector # This is the commit message Azure#36: comment unused variables # This is the commit message Azure#37: debug exporter # This is the commit message Azure#38: filenames # This is the commit message Azure#39: test zip function # This is the commit message Azure#40: list files # This is the commit message Azure#41: fmt to log # This is the commit message Azure#42: delete lines # This is the commit message Azure#43: changed # This is the commit message Azure#44: get current directory # This is the commit message Azure#45: remove some print statements # This is the commit message Azure#46: test zip # This is the commit message Azure#47: changes # This is the commit message Azure#48: add windir check # This is the commit message Azure#49: minor fix # This is the commit message Azure#50: get hostname # This is the commit message Azure#51: add expose in dockerfile # This is the commit message Azure#52: add exec collector # This is the commit message Azure#53: mitigate exit code 126 # This is the commit message Azure#54: change curl url from example.com to dp endpoint # This is the commit message Azure#55: changes # This is the commit message Azure#56: uncomment exec # This is the commit message Azure#57: add new diagnoser # This is the commit message Azure#58: debugging # This is the commit message Azure#59: debug # This is the commit message Azure#60: debugging # This is the commit message Azure#61: remove print statements # This is the commit message Azure#62: remove print # This is the commit message Azure#63: add back crd print statement # This is the commit message Azure#64: change # This is the commit message Azure#65: change # This is the commit message Azure#66: update dataPoint name # This is the commit message Azure#67: modify forloop # This is the commit message Azure#68: add filename to datapoint # This is the commit message Azure#69: add back log prints # This is the commit message Azure#70: test # This is the commit message Azure#71: add fields to diagnostic signal # This is the commit message Azure#72: add config content to diagnoser # This is the commit message Azure#73: change format from yaml to json # This is the commit message Azure#74: add parameters for kubeobject config map # This is the commit message Azure#75: Revert "introduce arcmode" This reverts commit 5f4fed4. # This is the commit message Azure#76: fix helm collector style # This is the commit message Azure#77: revert changes that test arc customizations # This is the commit message Azure#78: fix merge conflicts # This is the commit message Azure#79: fix merge conflicts # This is the commit message Azure#80: Revert "Add v0.3 acr image for Private cluster fix. (Azure#22)" This reverts commit 49dd302. # This is the commit message Azure#81: fix merge conflicts # This is the commit message Azure#82: fix merge conflicts # This is the commit message Azure#83: add print statement # This is the commit message Azure#84: update print statement # This is the commit message Azure#85: committed # This is the commit message Azure#86: committed # This is the commit message Azure#87: committed # This is the commit message Azure#88: committed # This is the commit message Azure#89: remove print statements # This is the commit message Azure#90: fix merge conflicts # This is the commit message Azure#91: fix merge conflicts # This is the commit message Azure#92: fix merge conflicts # This is the commit message Azure#93: add repo command # This is the commit message Azure#94: change stable repo name # This is the commit message Azure#95: add write to file # This is the commit message Azure#96: add kured # This is the commit message Azure#97: change # This is the commit message Azure#98: changes # This is the commit message Azure#99: add default namespace # This is the commit message Azure#100: change # This is the commit message Azure#101: add integration test # This is the commit message Azure#102: changes # This is the commit message Azure#103: add helm test # This is the commit message Azure#104: change print statement to error # This is the commit message Azure#105: change # This is the commit message Azure#106: more changes # This is the commit message Azure#107: add go installation # This is the commit message Azure#108: fix unit test # This is the commit message Azure#109: add custom resource collector # This is the commit message Azure#110: fix merge conflicts # This is the commit message Azure#111: comment unused variables # This is the commit message Azure#112: debug exporter # This is the commit message Azure#113: filenames # This is the commit message Azure#114: test zip function # This is the commit message Azure#115: list files # This is the commit message Azure#116: fmt to log # This is the commit message Azure#117: delete lines # This is the commit message Azure#118: changed # This is the commit message Azure#119: get current directory # This is the commit message Azure#120: remove some print statements # This is the commit message Azure#121: test zip # This is the commit message Azure#122: changes # This is the commit message Azure#123: add windir check # This is the commit message Azure#124: minor fix # This is the commit message Azure#125: get hostname # This is the commit message Azure#126: add expose in dockerfile # This is the commit message Azure#127: add exec collector
@Tatsinnit @arnaud-tincelin. Thanks for bringing this up. I squashed a lot of my commits, so this PR should now have 2 commits instead of 18. About that commit which references all of the old issues, it comes from a different branch in my repo, so it won't affect this PR. It might have happened when I was trying to squash some commits in a different branch, I'm relatively new to the squash functionality for git commits and at one point, the commits doubled, which was not supposed to happen. Anyways, I'm trying to change the commit message right now so that it doesn't reference every issue, but am really struggling to get my updates in. Do you have any tips on how to fix this? Thanks. |
Thanks @sophsoph321 , ah, I think this commit which is appearing in all the issues is from this branch: https://github.com/sophsoph321/aks-periscope/commits/helm_class So not from your current branch, I would say if this “helm_class” is not in need you can clean or you can rebase that branch for whatever purpose it was created.
this is the specific commit in that branch:
|
Thanks @Tatsinnit. I have removed the helm_class branch from my repo since it is not needed. Unfortunately, the commit reference is still there, with a warning message saying that "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository." |
|
All good @sophsoph321 let's treat this as separate issue. I think the issue is that whatever your other branch has, it is not cleaned-up properly, hence even though that commit does't show up now, the rouge commit stays in that branch in your fork. Thanks. |
| name: clustertype-config | ||
| namespace: aks-periscope | ||
| data: | ||
| CLUSTER_TYPE: "managedCluster" # <custom flag> |
There was a problem hiding this comment.
A request: would it be possible to use a generic name here like FEATURE_FLAGS? Specifically because in PR #48, based on feedback from @Tatsinnit, we are looking to reuse your flag for adding in 2 new collectors (SMI and OSM) that are not based on cluster type.
There was a problem hiding this comment.
💡 Let's do this change in your PR, I think ARC folks have almost completed this, so as it looks like only thing you need is renaming this flag and we can do that as part of the OSM x SMI workitem.
cc: @safeermohammed - what do you think?
There was a problem hiding this comment.
Sounds good to me. We can make that change as a fast follow once this gets merged
cc: @shalier
There was a problem hiding this comment.
Thank you @sophsoph321 , these changes looks good, I am approving it and I need to cut a release notes etc, before we formally release it.
Next step for me will be cut a release notes, plan a release. Also, we might have to add small read-me to the main doc as well, but I could do that and get your thoughts.
We need to do squash merge so I will do that tonight one @arnaud-tincelin or @JunSun17 has taken on final eyeballing for this PR.
Thanks. 🙏
@Tatsinnit. This sounds good. About the documentation, the most compelling piece of documentation I can think of for now is updating the Appendix, so that the users know that they can customize values for the new configmap ClusterType. Would this new clusterType feature flag impact the az aks kollect cli as well? If yes, then then there should be an additional example in the User guide section of the ReadMe would also need to be updated. I will let you know if I think of other updates that need to be made for the documentation. |
Thank you @sohpsoph321 , I think that sounds like good idea, a doc with the name of Your work should not effect current functionality, as long as you have tested your changes against vanilla deployment, because the new flag is only used to trigger For read-me - we will add one liner as to what this new feature is. Idealistically we can add documentation as part of the release cut we will do. Thank you 🙏 |
This PR includes implementation for the following customizations for azure arc:
Description of the new HelmCollector class:
This HelmCollector class is needed as one of the arc customizations for periscope because as part of the arc extensions feature, the extensions are deployed as helm packages. Therefore, for troubleshooting purposes, the HelmCollector class will be running the commands "helm list --all-namespaces" and "helm history". The command "helm list --all-namespaces" will list all of the releases across all namespaces that are deployed or failed. The command "helm history" will list the historical revisions of a release and provides information such as the following: revision numbers, date and time of the revision, its status, the name of the release chart, the version of the app, and the description.