Skip to content

servingInfo is not required for sdn configuration#21551

Merged
smarterclayton merged 1 commit intoopenshift:masterfrom
smarterclayton:serving_info_optional
Nov 27, 2018
Merged

servingInfo is not required for sdn configuration#21551
smarterclayton merged 1 commit intoopenshift:masterfrom
smarterclayton:serving_info_optional

Conversation

@smarterclayton
Copy link
Copy Markdown
Contributor

@smarterclayton smarterclayton commented Nov 26, 2018 •
edited
Loading

The requirement is a legacy of our previously cautious approach to splitting config between SDN and kubelet. We don't need serving info for the SDN component at this point.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 26, 2018
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Nov 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2018
@sjenning
Copy link
Copy Markdown
Contributor

sjenning commented Nov 26, 2018

this code makes it seem that not setting cert-dir in the kubelet arguments is an option. is it a fake option? i.e. if you don't set cert-dir it fails validation elsewhere?

@sjenning sjenning mentioned this pull request Nov 26, 2018
Merged
@sjenning
Copy link
Copy Markdown
Contributor

sjenning commented Nov 26, 2018

Nevermind. I see now that the code is "if there is no cert-dir set, then the certificates are required and are validated".

I guess we could just set --cert-dir=/etc/kubernetes/pki as an argument at https://github.com/openshift/cluster-network-operator/blob/master/pkg/network/openshift_sdn.go#L200-L203 and remove the CertInfo from ServingInfo right?

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 26, 2018

You could, but if we're just going to use the SA (now I'm vaguely remembering that we backed off using the node's identity, and the code in 3.11/3.10 bears that out) we don't need cert-dir at all, and probably shouldn't fake it.

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 26, 2018

/retest

4 similar comments
@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 26, 2018

/retest

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 26, 2018

/retest

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 27, 2018

/retest

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 27, 2018

/retest

@smarterclayton smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2018
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 27, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 27, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 27, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 27, 2018

/retest

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented Nov 27, 2018

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 27, 2018

/retest

@sjenning
Copy link
Copy Markdown
Contributor

sjenning commented Nov 27, 2018

/test e2e-gcp

@smarterclayton smarterclayton merged commit 83ebbf2 into openshift:master Nov 27, 2018
@smarterclayton
Copy link
Copy Markdown
Contributor Author

smarterclayton commented Nov 27, 2018

here comes the hammer

@sjenning sjenning mentioned this pull request Nov 27, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericavonb ericavonb Awaiting requested review from ericavonb

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@smarterclayton @openshift-ci-robot @sjenning @openshift-bot