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

Allow verify-install to work if cluster has managedField enabled (default in K8s 1.18) #26308

Closed
wants to merge 2 commits into from

Conversation

esnible
Copy link
Contributor

@esnible esnible commented Aug 9, 2020

Resolves #25688

Istioctl couldn't parse the on-cluster IstioOperator, because of a time field inside a managedField. I have only seen this on K8s 1.18.)

Also:

  • Allow --manifests to work with -f. This is very important if we don't an in-cluster IOP to recreate a problem. The in-cluster IOP may have a path that doesn't exist on the machine doing the verification, e.g. /home/prow.
  • Relax deployment counting. Any deployment created by istioctl install is counted, not just core control plane.

@esnible esnible requested a review from a team as a code owner August 9, 2020 18:51
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 9, 2020
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 9, 2020
@esnible esnible added the release-notes-none Indicates a PR that does not require release notes. label Aug 9, 2020
@knight42
Copy link
Member

knight42 commented Aug 11, 2020

I grep-ed SetCreationTimestamp in the project, and find out some code might also need to be updated:

  1. if len(mapIOP) > 0 {
    un := &unstructured.Unstructured{Object: mapIOP}
    un.SetCreationTimestamp(meta_v1.Time{}) // UnmarshalIstioOperator chokes on these
    iopYAML = util.ToYAML(un)
    }
    iop := &v1alpha1.IstioOperator{}
    if err := util.UnmarshalWithJSONPB(iopYAML, iop, false); err != nil {
  2. // IstioOperator isn't part of pkg/config/schema/collections,
    // usual conversion not available. Convert unstructured to string
    // and ask operator code to check.
    un.SetCreationTimestamp(metav1.Time{}) // UnmarshalIstioOperator chokes on these
    by := util.ToYAML(un)
    iop, err := operator_istio.UnmarshalIstioOperator(by, false)
    if err != nil {
    return err
    }
  3. // IstioOperator isn't part of pkg/config/schema/collections,
    // usual conversion not available. Convert unstructured to string
    // and ask operator code to unmarshal.
    un.SetCreationTimestamp(meta_v1.Time{}) // UnmarshalIstioOperator chokes on these
    by := util.ToYAML(un)
    iop, err := operator_istio.UnmarshalIstioOperator(by, true)
    if err != nil {
    return nil, err
    }
  4. for _, un := range ul.Items {
    un.SetCreationTimestamp(meta_v1.Time{}) // UnmarshalIstioOperator chokes on these
    by := util.ToYAML(un.Object)
    iop, err := operator_istio.UnmarshalIstioOperator(by, true)
    if err != nil {
    return nil, err
    }

@esnible
Copy link
Contributor Author

esnible commented Aug 11, 2020

I grep-ed SetCreationTimestamp in the project, and find out some code might also need to be updated:

@knight42 The problem is that we use gogo proto code in a way that chokes on Kubernetes time. We don't even need to use protobuf code at all. We should be treating IOP like the other Istio CRs.

  • Previous attempts to make IOP act like the other CRs have failed. Many things need to move to istio/api.
  • I have shown @ostromart a replacement for UnmarshalIstioOperator() in common.go based on encoding/json and it looks like that will be the way forward for 1.8
  • We could try to create a common function for 1.7 for unmarshalling IOPs, but I don't think it could be tested in time for 1.7.0. This PR is supposed to be the MVP to get verify-install working on 1.18.

@knight42
Copy link
Member

a replacement for UnmarshalIstioOperator() in common.go based on encoding/json and it looks like that will be the way forward for 1.8

@esnible I don't know what the implementation is, maybe I am overcautious, but if you decide to leverage encoding/json, please be careful that * IstioOperatorSpec implements UnmarshalJSON method, and that implementation forbids unknown fields. See also #25247 (comment)

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 12, 2020
@mandarjog
Copy link
Contributor

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Sep 15, 2020
@esnible esnible added lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed do-not-merge/hold Block automatic merging of a PR. and removed lifecycle/staleproof Indicates a PR or issue has been deemed to be immune from becoming stale and/or automatically closed labels Oct 1, 2020
@esnible
Copy link
Contributor Author

esnible commented Oct 1, 2020

Adding a hold because another PR might have accomplished the same thing.

@istio-policy-bot istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 1, 2020
@aroundthecode
Copy link

Hi guys, any update on this? I'm trying to install 1.7.4 on k8 1.18.0 but I'm totally unable do deploy IstioOperator configuration?

Any workaround other than waiting for this fix?
Both kubectl apply nor istioctl install seems to work to me.

@aroundthecode
Copy link

@esnible sorry to bother you, but is there any chance to see this merged and released in 1.7.5? I don't know which "other PR" are you referring to solve this problem, but it's totally blocking any custom deploy on k8s 1.18.

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Nov 7, 2020
@istio-testing istio-testing removed needs-rebase Indicates a PR needs to be rebased before being merged size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2020
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 10, 2020

// gogoWorkaround prepares an unstructed for github.com/gogo/protobuf's Unmarshal(). That method cannot
// unmarshal Kubernetes time.Time.
func gogoWorkaround(un *unstructured.Unstructured) {
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to name it gogoWorkaround? The function doesn't do anything related to gogo.
If I recall correctly, @sdake was working on removing gogo related stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I call it that because it is a flaw in gogo's unmarshall, at least the last time I checked.

@istio-policy-bot istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Nov 10, 2020
@shamsher31
Copy link
Member

shamsher31 commented Nov 10, 2020

@esnible Does it make sense to refactor the same mentioned in #26308 (comment) ?

I think we should give it a try #24819 (comment)

@shamsher31
Copy link
Member

@esnible Is this PR will help resolve some of the issues mentioned in #28148?

@esnible
Copy link
Contributor Author

esnible commented Nov 11, 2020

/test integ-ipv6-k8s-tests_istio

@istio-testing
Copy link
Collaborator

@esnible: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
integ-ipv6-k8s-tests_istio 60d4be2 link /test integ-ipv6-k8s-tests_istio

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@esnible
Copy link
Contributor Author

esnible commented Nov 12, 2020

Closing because #28255 changed verify-install significantly.

If #28750 continues to fail with the changes in 28255 this work will be done in an all-new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/hold Block automatic merging of a PR. release-notes-none Indicates a PR that does not require release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[1.8] 'istioctl verify-install' fails on Kubernetes 1.18
8 participants