Skip to content

Allow register/degregister istioctl commands to work with .pem#5330

Closed
esnible wants to merge 1 commit intomasterfrom
issue5255
Closed

Allow register/degregister istioctl commands to work with .pem#5330
esnible wants to merge 1 commit intomasterfrom
issue5255

Conversation

@esnible
Copy link
Copy Markdown
Contributor

@esnible esnible commented May 1, 2018 •
edited
Loading

This PR adds the istioctl kube-inject fix to istioctl register and deregister.
See PR #5300

See Issue #5255

@esnible esnible requested a review from ayj May 1, 2018 17:06
@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented May 1, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: nmittler

Assign the PR to them by writing /assign @nmittler in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ayj
ayj reviewed May 1, 2018
View reviewed changes
istioctl/cmd/istioctl/register.go
portsList[i] = p
}
log.Infof("Registering for service '%s' ip '%s', ports list %v",
log.Debugf("Registering for service '%s' ip '%s', ports list %v",
Copy link
Copy Markdown
Contributor

@ayj ayj May 1, 2018

Choose a reason for hiding this comment

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

cc @ldemailly

Copy link
Copy Markdown
Member

@ldemailly ldemailly May 1, 2018

Choose a reason for hiding this comment

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

Leave as it was / why the change ?

ayj
ayj reviewed May 1, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@ayj ayj left a comment

Choose a reason for hiding this comment

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

createInterface() changes lgtm - thanks for fixing that.

Deferring to @ldemailly on use of debug vs. log for register/unregister logging.

@linsun
Copy link
Copy Markdown
Member

linsun commented May 1, 2018

@esnible note: we want this change in 0.8 first, think you can update to merge to 0.8 easily. You may also need to merge #5300 to 0.8 first too. cc @ayj

@istio-testing
Copy link
Copy Markdown
Collaborator

istio-testing commented May 1, 2018

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

Test name Commit Details Rerun command
prow/istio-unit-tests.sh 5bc26d4 link /test istio-unit-tests
prow/istio-pilot-e2e.sh 5bc26d4 link /test istio-pilot-e2e
prow/e2e-bookInfoTests-v1alpha3.sh 5bc26d4 link /test e2e-bookInfo-envoyv2-v1alpha3
Details

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.

@rshriram
Copy link
Copy Markdown
Member

rshriram commented May 1, 2018

I suggest tweaking this to generate service entries or remove these commands and change documentation where you point to the user to generate service entries, since this no longer the right way to register VM endpoints in the mesh. You need to use ServiceEntries and not kubernetes services or endpoints to register VMs. cc @costinm

@linsun
Copy link
Copy Markdown
Member

linsun commented May 1, 2018

+1 to what @rshriram said but could be a separate PR. @esnible let me know if you will pull in Jason's change so we can get the kube-inject cmd fixed in 0.8 with ibm cluoud.

ldemailly
ldemailly reviewed May 1, 2018
View reviewed changes
istioctl/cmd/istioctl/register.go
annotations = append(annotations, fmt.Sprintf("%s=%s", kube.KubeServiceAccountsOnVMAnnotation, svcAcctAnn))
}
log.Infof("%d labels (%v) and %d annotations (%v)",
log.Debugf("%d labels (%v) and %d annotations (%v)",
Copy link
Copy Markdown
Member

@ldemailly ldemailly May 1, 2018

Choose a reason for hiding this comment

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

Ditto

@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented May 1, 2018

If register/deregister are no longer used for VMs we should get rid of those commands, not fix them. I don't have a use case. I was fixing these commands while researching a fix I discovered these commands had the same flaw as kube-inject.

The PR #5300 was merged yesterday. I am no longer the On Call and have no power to merge it into 0.8.

@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented May 1, 2018

0.8 is where the normal process still takes place and PRs should go.

Mesh expension should still work

@ayj ayj mentioned this pull request May 1, 2018
Merged
@esnible
Copy link
Copy Markdown
Contributor Author

esnible commented May 3, 2018

Closing because there is no use case for these commands and no urgent need to remove them.

@esnible esnible closed this May 3, 2018
@esnible esnible deleted the issue5255 branch May 3, 2018 21:07
@ldemailly
Copy link
Copy Markdown
Member

ldemailly commented May 3, 2018

Mesh expension is the use case, if the commands got broken they need a fix

@linsun linsun mentioned this pull request May 11, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ayj ayj ayj left review comments

@ldemailly ldemailly ldemailly left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@esnible @istio-testing @linsun @rshriram @ldemailly @ayj @googlebot