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.

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

feat(catalog): Add istio-stack #110

Merged
merged 32 commits into from
Apr 4, 2023
Merged

feat(catalog): Add istio-stack #110

merged 32 commits into from
Apr 4, 2023

Conversation

beiertu-mms
Copy link
Collaborator

See issue:

@beiertu-mms beiertu-mms force-pushed the feat/add-istio-stack branch 2 times, most recently from 1bd88e3 to 66f4699 Compare February 13, 2023 20:33
@beiertu-mms beiertu-mms marked this pull request as ready for review February 14, 2023 10:30
@beiertu-mms beiertu-mms force-pushed the feat/add-istio-stack branch 6 times, most recently from 46a53e6 to b199e28 Compare February 22, 2023 18:17
name: istio-apps-mtls
namespace: apps
spec:
host: "*.apps.svc.cluster.local"
Copy link
Owner

Choose a reason for hiding this comment

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

what is apps? What if the customer doesn't have an apps namespace? Imo this is user specific configuration whcih you can't foresee

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe it can be removed and a howto on the readme can be added for enabling this feature?

Copy link
Owner

@kharf kharf Mar 8, 2023

Choose a reason for hiding this comment

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

i think we shouldn't do it at all. This too opinionated. We just provide preconfiguration for istio based on where its running, but not how they should define their ingress/egress

kharf
kharf previously approved these changes Mar 8, 2023
Copy link
Owner

@kharf kharf left a comment

Choose a reason for hiding this comment

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

I think it looks fine. Im currently working on infrastructure tests, which we can use here as an admission control

@beiertu-mms beiertu-mms requested a review from kharf March 8, 2023 12:56
# istio-stack

This stack made it easier to setup a service mesh with [istio](https://istio.io/latest/).
It provides the setup for [istio-operator](https://github.com/stevehipwell/helm-charts/tree/master/charts/istio-operator) with sensible defaults
Copy link
Owner

Choose a reason for hiding this comment

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

sorry, didnt see it before. I think this link is not the correct anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

adjust this sentence

Copy link

@heubeck heubeck left a comment

Choose a reason for hiding this comment

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

haven't used the separate istio charts yet, just used the istio-operator.
but from the scope (install all base components without explicit configuration) it looks fine.
lets just try it with the next chance.

enabled: true
is_core: false
grafana:
url: "http://kube-prometheus-stack-grafana.monitoring.svc.cluster.local"
Copy link

Choose a reason for hiding this comment

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

Suggested change
url: "http://kube-prometheus-stack-grafana.monitoring.svc.cluster.local"
url: "http://prometheus-operator-grafana.monitoring.svc.cluster.local"

grafana:
url: "http://kube-prometheus-stack-grafana.monitoring.svc.cluster.local"
prometheus:
url: "http://kube-prometheus-stack-operator.monitoring.svc.cluster.local:9090"
Copy link

Choose a reason for hiding this comment

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

Suggested change
url: "http://kube-prometheus-stack-operator.monitoring.svc.cluster.local:9090"
url: "http://prometheus-operated.monitoring.svc.cluster.local:9090"

Copy link
Owner

Choose a reason for hiding this comment

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

for me kube-prometheus-stack-operator is the one, when kube-prometheus-stack is is in use. Currently we only have the kube-prometheus-stack in the catalog

custom_dashboards:
enabled: true
is_core: false
grafana:
Copy link

Choose a reason for hiding this comment

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

but probably extract the two urls in postBuild.subsitution to make it easy to override?
can one kustomization override the postBuild.subsitutions of another?

Copy link
Owner

Choose a reason for hiding this comment

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

good question, but you could also just patch the helmrelease via kustomize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

can one kustomization override the postBuild.subsitutions of another?

not sure, but probably not.
maybe we can take out these specific urls and let the user override them according to their setup?

Copy link

Choose a reason for hiding this comment

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

sure ;) postBuild.substitutions just would be simpler - but I also guess it won't work.

Copy link
Owner

Choose a reason for hiding this comment

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

if i want to overwrite something, I patch the HelmRelease

@kharf
Copy link
Owner

kharf commented Apr 4, 2023

is it finished=?

@beiertu-mms
Copy link
Collaborator Author

is it finished=?

hm, think so, waiting for the resolution of the conversation about the grafana/prometheus links.

@kharf
Copy link
Owner

kharf commented Apr 4, 2023

is it finished=?

hm, think so, waiting for the resolution of the conversation about the grafana/prometheus links.

For me its fine. People can override with kustomize patches

@beiertu-mms beiertu-mms merged commit bad5d54 into main Apr 4, 2023
@beiertu-mms beiertu-mms deleted the feat/add-istio-stack branch April 4, 2023 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants