-
Notifications
You must be signed in to change notification settings - Fork 148
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
add missing kube-api resources from managed agent manifest #381
Conversation
This pull request does not have a backport label. Could you fix it @oren-zohar? 🙏
NOTE: |
@blakerouse or @ChrsMark Can you take a look? |
I am going to have to defer to @ChrsMark, I do not know why it would need those permissions. Being that its related to that data-plane and not the control-plane (as Elastic Agent doesn't need that information for the the kubernetes dynamic input service). |
🌐 Coverage report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a chat with @tonymeehan and it seems that we can proceed with this one for now and follow-up on documenting in detail why these permissions are needed and what happens if someone disable them. I think the most suitable place for this would be https://www.elastic.co/guide/en/fleet/master/running-on-kubernetes-managed-by-fleet.html.
Another thing that we need to think of is how we treat changes in those manifests and more specifically if a change in those manifests can be considered a breaking change for users. My perspective is that so far we didn't treat those manifests the similar way we do with core codebase regarding breaking changes which means that we can remove/update specs/settings even if that breaks things.
@ChrsMark I opened an issue to track the documentation part. Merging is blocked for me so if you can please grant me merging rights or merge it yourself that would be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR would require some extra changes so as to be merged:
- I see that only the managed version is updated. Is this intentional? I would suggest ensuring same level of roles/permissions in both standalone and managed.
- Files in https://github.com/elastic/elastic-agent/tree/main/deploy/kubernetes/elastic-agent-standalone and https://github.com/elastic/elastic-agent/tree/main/deploy/kubernetes/elastic-agent-managed should be updated too properly. Actually changes should take place first in these files and then running
make all
at top level should update the full files too. This should had been catched by our CI here, since it was like this in Beats repo. @narph do you know why this is not happening here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. However we need to make sure that process of updating these files is robust by using make all
and that CI will ensure that no inconsistencies exist (cc: @narph )
version/docs/version.asciidoc
Outdated
@@ -1,4 +1,4 @@ | |||
:stack-version: 8.0.0 | |||
:stack-version: 8.2.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@narph it seems that this was needed so as to the make all
to generate the proper final manifests. Could you verify this please? Also we need to figure out why the CI didn't complain about the previous commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this pr #417 should update version and add check in CI
…use-orka * 'main' of github.com:elastic/elastic-agent: (23 commits) [Automation] Update go release version to 1.17.10 (elastic#432) [Automation] Update elastic stack version to 8.3.0-4149272f for testing (elastic#435) [Automation] Update elastic stack version to 8.3.0-19aba912 for testing (elastic#430) Add extra k8s resources in clusterRole (elastic#424) [Automation] Update elastic stack version to 8.3.0-8ee1196f for testing (elastic#422) [Automation] Update elastic stack version to 8.3.0-53513548 for testing (elastic#421) Add tags option during enroll/install (elastic#336) validate kubernetes templates in .CI (elastic#417) add missing kube-api resources from managed agent manifest (elastic#381) Create snyk-scan.yml (elastic#397) [Automation] Update elastic stack version to 8.3.0-d380914f for testing (elastic#414) [Automation] Update elastic stack version to 8.3.0-5c1ff35f for testing (elastic#413) [Automation] Update elastic stack version to 8.3.0-6ba9f710 for testing (elastic#410) [Automation] Update elastic stack version to 8.3.0-a1c5cfff for testing (elastic#406) [Automation] Update elastic stack version to 8.3.0-7f585873 for testing (elastic#401) [Automation] Update elastic stack version to 8.3.0-0b6ea9f2 for testing (elastic#399) ci: enable coverage (elastic#377) Remove last dependencies on beats repo (elastic#387) Remove dependency on libbeat (elastic#344) [Automation] Update elastic stack version to 8.3.0-cb2ce38c for testing (elastic#383) ...
What does this PR do?
In order for cloudbeat
kube-api
fetcher to work properly, we need to update the agent-managed manifest with permissions related to kube-api resources.Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Related issues