-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Fleet] Present users with a Kubernetes manifest for deploying agent in fleet managed mode for Kubernetes #127703
[Fleet] Present users with a Kubernetes manifest for deploying agent in fleet managed mode for Kubernetes #127703
Conversation
Pinging @elastic/fleet (Team:Fleet) |
Just a heads up, the Fleet team has some in-progress work in this area that may result in conflicts: #80841 |
Thanks for the heads up! |
|
||
import { AdvancedAgentAuthenticationSettings } from './advanced_agent_authentication_settings'; | ||
import { SelectCreateAgentPolicy } from './agent_policy_select_create'; | ||
|
||
export const DownloadStep = (hasFleetServer: boolean) => { | ||
export const DownloadStep = (hasFleetServer: boolean, isK8s: string, enrollmentAPIKey: string) => { |
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.
you could make isK8s
and enrollmentAPIKey
optional parameters, to avoid passing ''
when not needed
It would be nice if you could add some unit test coverage on the changes. |
isK8s === 'IS_KUBERNETES' ? ( | ||
<FormattedMessage | ||
id="xpack.fleet.agentEnrollment.downloadDescriptionForK8s" | ||
defaultMessage="Copy or download the Kubernetes manifest inside the Kubernetes cluster. Check {FleetUrlVariable} and {FleetTokenVariable} in the Daemonset environment variables and apply the manifest." |
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.
I'm wondering if it could be misleading if user deployed Agent using ECK? I'm not sure what the "Kubernetes manifest" would be in that case ? (not sure I have the full picture, so my comment might not be relevant).
CC @Kushmaro
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.
There is a point in what you are saying. ECK process is different and uses different manifests https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-deploy-eck.html. But this could be an extra addition in the instructions. With this PR I am just implementing #92113
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.
IMHO we should promote both "regular" k8s manifests, however, we may want to promote ECK manifests (or related messaging) prior.
What I mean here - is that when a customer would like to "run something elastic with k8s" we should first let them know that ECK is the best way of doing so. regardless of where such a message exists.
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.
@Kushmaro Can you clarify what you meant by - > "both "regular" k8s manifests" and
"run something elastic with k8s"
If I understood @barkbay correctly, then the add->agent flyout should also generate agent configuration yaml and set of instructions to be followed in ECK user journey. The current solution assumes vanilla K8s setup, not associated with how the stack components are deployed.
@barkbay @Kushmaro are you able to create another issue in this repo for accounting for ECK user requirement?
CC @mlunadia
@MichaelKatsoulis we're working on a complete redesign of this flyout for 8.2. We'll have to change the steps and remove some of them. Can we find a way to coordinate on these PRs to make these changes easier? |
@criamico if the changes you are working on will affect that much the |
Yes I think it would be better as the redesign is going to modify the flyout in a substantial way. Let me find a time to schedule a call. |
After a discussion with @criamico, we have decided to freeze this until the restructuring of the flyout is finished. Then I will do my changes on top. |
@@ -93,6 +95,32 @@ export const AgentEnrollmentFlyout: React.FunctionComponent<Props> = ({ | |||
checkPolicyIsFleetServer(); | |||
}, [policyId]); | |||
|
|||
const [isK8s, setIsK8s] = useState<'IS_LOADING' | 'IS_KUBERNETES' | 'IS_NOT_KUBERNETES'>( |
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.
I think we can move this to it's own hook and use the same hook for standalone instructions
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.
Refactored hooks LGTM 🚀
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.
UI work as expected 🚀
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled in files
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @nchaulet |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
1 similar comment
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Hi Team
Build details: Hence marking this as QA:Validated. |
Summary
This PR updates agent addition instructions in case a policy contains Kubernetes integration.
It closes #92113
Checklist
Delete any items that are not applicable to this PR.
For maintainers