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

Add jaeger port to values comment #8855

Merged
merged 1 commit into from Jul 13, 2022
Merged

Add jaeger port to values comment #8855

merged 1 commit into from Jul 13, 2022

Conversation

haswalt
Copy link
Contributor

@haswalt haswalt commented Jul 12, 2022

Fix linkerd-viz helm chart documentation for jaeger integration.

Adds miss port to jaeger url example in value.yaml. This port is required to allow the dashboard to proxy to the jaeger instance. This brings the example given in the values.yaml file in line with the web docs.

Fixes #8851

@haswalt haswalt requested a review from a team as a code owner July 12, 2022 14:52
mateiidavid
mateiidavid reviewed Jul 12, 2022
View reviewed changes
viz/charts/linkerd-viz/README.md Outdated
@@ -95,13 +95,13 @@ Kubernetes: `>=1.21.0-0`
| defaultRegistry | string | `"cr.l5d.io/linkerd"` | Docker registry for all viz components |
| defaultUID | int | `2103` | UID for all the viz components |
| enablePSP | bool | `false` | NodeAffinity section, See the [K8S documentation](https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#node-affinity) for more information nodeAffinity: -- Create Roles and RoleBindings to associate this extension's ServiceAccounts to the control plane PSP resource. This requires that `enabledPSP` is set to true on the control plane install. Note PSP has been deprecated since k8s v1.21 |
| enablePodAntiAffinity | bool | `false` | Enables Pod Anti Affinity logic to balance the placement of replicas across hosts and zones for High Availability. Enable this only when you have multiple replicas of components. |
| enablePodAntiAffinity | bool | `false` | |
Copy link
Member

Choose a reason for hiding this comment

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

Think the text for enablePodAntiAffinity was deleted by mistake. We should maybe add it back in and re-generate the README here 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was this removed in another commit on main? I only changed 1 line in the values.yaml and ran the doc script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

go.sum Outdated
@@ -863,8 +863,8 @@ github.com/prometheus/common v0.26.0/go.mod h1:M7rCNAaPfAosfx8veZJCuw84e35h3Cfd9
github.com/prometheus/common v0.28.0/go.mod h1:vu+V0TpY+O6vW9J44gczi3Ap/oXXR10b+M/gUGO4Hls=
github.com/prometheus/common v0.30.0/go.mod h1:vu+V0TpY+O6vW9J44gczi3Ap/oXXR10b+M/gUGO4Hls=
github.com/prometheus/common v0.32.1/go.mod h1:vu+V0TpY+O6vW9J44gczi3Ap/oXXR10b+M/gUGO4Hls=
github.com/prometheus/common v0.35.0 h1:Eyr+Pw2VymWejHqCugNaQXkAi6KayVNxaHeu6khmFBE=
github.com/prometheus/common v0.35.0/go.mod h1:phzohg0JFMnBEFGxTDbfu3QyL5GI8gTQJFhYO5B3mfA=
github.com/prometheus/common v0.36.0 h1:78hJTing+BLYLjhXE+Z2BubeEymH5Lr0/Mt8FKkxxYo=
Copy link
Member

Choose a reason for hiding this comment

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

Do we know where these dep bumps came from? Could we do soft reset in git and not commit go.mod and go.sum? Think something like git reset --soft HEAD^ might work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like those commits came from a commit by dependbot not by me...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@kleimkuhler
Copy link
Contributor

@haswalt as Matei already mentioned it looks like your branch is introducing an additional commit with some unnecessary changes. If you're unsure how that happened, I'd recommend pulling the latest main, resetting this branch to the latest commit (git reset --hard origin/main), and adding the change again.

Ultimately, the only change that should be introduced is the jaegerURL comment containing the necessary port value. After running bin/helm-docs, there should only be 2 files changes—each of those changes pertaining only to jaegerURL.

@haswalt
Add port to helm Values
ceba415 
Signed-off-by: Harry Walter <harry@bluebamboostudios.com>
@haswalt
Copy link
Contributor Author

haswalt commented Jul 13, 2022

@kleimkuhler @mateiidavid Looks like a bad rebase corrupted the HEAD commit from main. I've fixed and repushed

@mateiidavid
Copy link
Member

@haswalt yeahhh, thought something like that might've happened. Thanks for the quick fix and the PR! 🚀

kleimkuhler
kleimkuhler approved these changes Jul 13, 2022
View reviewed changes
Copy link
Contributor

@kleimkuhler kleimkuhler left a comment

Choose a reason for hiding this comment

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

Thanks @haswalt

@kleimkuhler kleimkuhler merged commit 6ed1d06 into linkerd:main Jul 13, 2022
olix0r added a commit that referenced this pull request Jul 13, 2022
@olix0r @adleong
@dependabot @haswalt
Policy controller suggestions (for #8795) (#8858)
26cc102 
* Policy controller suggestions

* core: Use `http` crate instead of `hyper`'s re-export. `http` is just
  the core types. `hyper` includes client/server infrastructure which
  isn't needed. We already pull in both so there's practically no
  functional difference.
* core: Rename `Hostname` to `HostMatch` to be consistent with API
  types.
* core: Rename `HttpRoute`, `HttpFilter`, etc to `Inbound*`. These types
  are specific to inbound policies. We wouldn't use the same types for
  outbound policies.
* core: Split individual filter types from the `InboundFilter` type so
  that the `InboundFilter` type doesn't hold all of the details for all
  of the filters.
* core: Make `HeaderMatch` hold `HeaderName` and `HeaderValue` so that
  we can rely on the validation from these libraries. Notably,
  `Headervalue` does not necessarily hold a string.
* core: Make `QueryParamMatch` an enum, since the `Value` type would
  only have that one use now.

* index: Rename `RouteBinding` to `InboundRouteBinding`, as it holds
  inbound-specific route configuration.
* index: Add a `InboundParentRef` type that describes a validated parent
  reference.
* index: Update `InboundRouteBinding::try_from` to validate parent
  references and fail reading routes that do not reference servers.

* grpc: Move general `http_route` conversions into a dedicated module
  (to simplify inbound coverters).

* Cleanup imports as much as possible, shortening module names with
  aliases where possible. Because we're frequently converting between
  different representations of the same types, it's helpful to reference
  the modules explicitly rather than relying on large sets of imports.
* Where possible, we destructure types to document that we are handling
  all fields on a type.
* Update deny.toml for git dependency

Signed-off-by: Oliver Gould <ver@buoyant.io>

* Upgrade to moment 2.29.4 (#8856)

Signed-off-by: Alex Leong <alex@buoyant.io>

* build(deps): bump google.golang.org/grpc from 1.47.0 to 1.48.0 (#8857)

Bumps [google.golang.org/grpc](https://github.com/grpc/grpc-go) from 1.47.0 to 1.48.0.
- [Release notes](https://github.com/grpc/grpc-go/releases)
- [Commits](grpc/grpc-go@v1.47.0...v1.48.0)

---
updated-dependencies:
- dependency-name: google.golang.org/grpc
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Add port to helm Values (#8855)

Fix `linkerd-viz` helm chart documentation for jaeger integration.

Adds miss port to jaeger url example in `value.yaml`. This port is required to
allow the dashboard to proxy to the jaeger instance. This brings the example
given in the `values.yaml` file in line with the web docs.

Closes #8851

Signed-off-by: Harry Walter <harry@bluebamboostudios.com>

* policy: Index authorization policies with no authentications (#8865)

In 1a0c1c3 we updated the admission controller to allow
`AuthorizationPolicy` resources with an empty
`requiredAuthenticationRefs`. But we did NOT update the indexer, so we
would allow these resources to be created but then fail to honor them in
the API.

To fix this:

1. The `AuthorizationPolicy` admission controller is updated to exercise
   the indexer's validation so that it is impossible to admit resources
   that will be discarded by the indexer;
2. An e2e test is added to exercise this configuration;
3. The indexer's validation is updated to accept resources with no
   authentications.

Signed-off-by: Oliver Gould <ver@buoyant.io>

* Simply ignore non-server parent refs when indexing

Signed-off-by: Oliver Gould <ver@buoyant.io>

Co-authored-by: Alex Leong <alex@buoyant.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Harry Walter <harry.walter@lqdinternet.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kleimkuhler kleimkuhler kleimkuhler approved these changes

@mateiidavid mateiidavid mateiidavid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Port required in helm values for linkerd-viz
3 participants
@haswalt @kleimkuhler @mateiidavid