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

Add APIService fake clientset support #3569

Merged
merged 1 commit into from
Oct 21, 2019
Merged

Add APIService fake clientset support #3569

merged 1 commit into from
Oct 21, 2019

Conversation

siggy
Copy link
Member

@siggy siggy commented Oct 14, 2019

The linkerd upgrade --from-manifests command supports reading the
manifest output via linkerd install. PR #3167 introduced a tap
APIService object into linkerd install, but the manifest-reading code
in fake.go was never updated to support this new object kind.

Update the fake clientset code to support APIService objects.

Fixes #3559

Signed-off-by: Andrew Seigner [email protected]

@siggy siggy added the area/cli label Oct 14, 2019
@siggy siggy requested a review from ihcsim October 14, 2019 04:01
@siggy siggy self-assigned this Oct 14, 2019
@siggy siggy force-pushed the siggy/fake-apiservice branch from f620028 to b1d464b Compare October 14, 2019 04:02
Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

This change looks good, but I do have some meta-questions about the larger context.

Do we have concerns about this approach being sustainable? We will need to add a fake client set for any API group in our install manifest, even if we never plan to read that object from the real or fake API.

An alternate approach would be to have the upgrade --from-manifest code parse the manifest directly rather than constructing a fake API. Has this already been considered?

Anyway, I'm sure this is fine. It just feels weird that we need to build fake client sets for apigroups that we never interact with.

Copy link
Member

@adleong adleong left a comment

Choose a reason for hiding this comment

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

Is the deps change required?

@siggy
Copy link
Member Author

siggy commented Oct 15, 2019

@adleong

Do we have concerns about this approach being sustainable? We will need to add a fake client set for any API group in our install manifest, even if we never plan to read that object from the real or fake API.

I'm not particularly concerned about the sustainability. Thus far we have special-cased CustomResourceDefinition and APIService.

An alternate approach would be to have the upgrade --from-manifest code parse the manifest directly rather than constructing a fake API. Has this already been considered?

When implementing #2697 I did consider this approach, but realizing we already had NewFakeClientSets in place for testing, re-using that code, which uses Kubernetes libraries to parse manifests, seemed a more sustainable approach.

Anyway, I'm sure this is fine. It just feels weird that we need to build fake client sets for apigroups that we never interact with.

I had two motivations for this branch:

1. Upgrading from a complete install manifest

Our install/check/upgrade/cli ux is extremely clear and often called out as a favorite feature by users. Without this change, the upgrade from manifest flow requires the user to understand which k8s objects are required for upgrade:

linkerd install > linkerd-install.yaml
cat linkerd-install.yaml | kubectl apply -f -
kubectl -n linkerd get \
  secret/linkerd-identity-issuer \
  configmap/linkerd-config \
  -oyaml > linkerd-manifests.yaml
cat linkerd-manifests.yaml | linkerd upgrade --from-manifests - | kubectl apply -f -

... this change allows for a more user friendly upgrade process:

linkerd install > linkerd-install.yaml
cat linkerd-install.yaml | kubectl apply -f -
cat linkerd-install.yaml | linkerd upgrade --from-manifests - | kubectl apply -f -

2. Testing completeness

The fake module in pkg/k8s is used heavily in our testing infrastructure. Anecdotally I find that when there is existing test infrastructure in an area I'm working in, I'm more likely to add more tests during development. If no testing infrastructure is present (for example, if NewFakeClientSets does not support APIService), the barrier to getting tests implemented is much higher, particularly for a small change.

@siggy
Copy link
Member Author

siggy commented Oct 15, 2019

@adleong

Is the deps change required?

I tried minimizing the changes to go.mod while still adding k8s.io/kube-aggregator, by varying the version of that dependency we were importing. This was the smallest change I was able to achieve.

@ihcsim
Copy link
Contributor

ihcsim commented Oct 16, 2019

@siggy Code change LGTM 👍! Mind fixing up the conflicts?

@siggy siggy force-pushed the siggy/fake-apiservice branch from b1d464b to c3a24d7 Compare October 16, 2019 22:59
The `linkerd upgrade --from-manifests` command supports reading the
manifest output via `linkerd install`. PR #3167 introduced a tap
APIService object into `linkerd install`, but the manifest-reading code
in fake.go was never updated to support this new object kind.

Update the fake clientset code to support APIService objects.

Fixes #3559

Signed-off-by: Andrew Seigner <[email protected]>
@siggy siggy force-pushed the siggy/fake-apiservice branch from c3a24d7 to f648a92 Compare October 21, 2019 18:11
@ihcsim ihcsim merged commit 0f9ea55 into master Oct 21, 2019
@siggy siggy deleted the siggy/fake-apiservice branch April 1, 2020 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade using --from-manifest option causes APIService error
3 participants