Skip to content

Stop using static tls data #22

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
Closed
trawler wants to merge 1 commit into from
Closed

Stop using static tls data #22

trawler wants to merge 1 commit into from

Conversation

@trawler
Copy link
Contributor

@trawler trawler commented Aug 17, 2018 •
edited
Loading

This PR removes the static tls secrets cluster-apiserver-secrets and uses instead the installer generated TLS certs: https://github.com/openshift/installer/blob/master/modules/bootkube/resources/manifests/cluster-apiserver-secret.yaml

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 17, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: trawler

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 17, 2018
@trawler trawler changed the title Stop using stating tls data Stop using static tls data Aug 17, 2018
@trawler trawler requested review from enxebre and ingvagabund August 17, 2018 15:15
@ingvagabund
Copy link
Member

ingvagabund commented Aug 17, 2018

We should still keep the manifests/clusterapi-apiserver-secret.yaml for testing purposes. Unless we want to generate it every time we run a CI test (which is not hard either). Otherwise, /approve.

@ingvagabund
Copy link
Member

ingvagabund commented Aug 17, 2018

@trawler can you attach the installer PR that generates the certificates? Just for reference.

@trawler
Copy link
Contributor Author

trawler commented Aug 17, 2018

@ingvagabund there you go :) openshift/installer#79

enxebre
enxebre reviewed Aug 17, 2018
View reviewed changes
manifests/clusterapi-apiservice.yaml
versionPriority: 10
caBundle: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUM5RENDQWR5Z0F3SUJBZ0lCQURBTkJna3Foa2lHOXcwQkFRc0ZBREFyTVNrd0p3WURWUVFERXlCamJIVnoKZEdWeVlYQnBMV05sY25ScFptbGpZWFJsTFdGMWRHaHZjbWwwZVRBZUZ3MHhPREEzTVRBd09ETTRORGhhRncweQpPREEzTURjd09ETTRORGhhTUNzeEtUQW5CZ05WQkFNVElHTnNkWE4wWlhKaGNHa3RZMlZ5ZEdsbWFXTmhkR1V0CllYVjBhRzl5YVhSNU1JSUJJakFOQmdrcWhraUc5dzBCQVFFRkFBT0NBUThBTUlJQkNnS0NBUUVBdVR3TGR6NlIKbTVxU20wNC9ES1Y3ZUV1ZnFrQmh0S01GZHM3MGswaUVVWjlKcWpNSWE4cFNkU2dMc3VWQXNwbkVZYmJxcTFUcAp1UVRHbnpoNnRQU2lsRnprOWVlOFUvL0dDQmU5NzhNeXl3emgycFB6WjBGaEZHMzFlaVNycjQrRlUxRFpPVGIwCkFucU9QQzU0QmpUOFZTci9aZm9OZkFoTm9hK1RxeEt4dzdDcDZMbmFLVmkzR3U1SlowSzFVK1Z2bGt6ajhWUGkKU256azMyVDJCZTZEQ0MwTUhYRXlRRXdoWFVrTTg5eTNHMTZtVG12Z0JiSHowU2F6Vjc4UE02b0lUdS9HcmgvLwp2RTVoYUdOcTF0RVY0TTRPMGR5K2k2d01rZU9vV3o1MjA5NjgvQ1VmMHJ3c2ZVMGwxWC9YK1RhTmkrMm4weEh1CkJqSWF5bEZaM3oxbVRRSURBUUFCb3lNd0lUQU9CZ05WSFE4QkFmOEVCQU1DQXFRd0R3WURWUjBUQVFIL0JBVXcKQXdFQi96QU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFoVFpMSmtOa0FibDE4b3JEc2ZjeFN5TE40MjREOVltLwpXWVo1d1RpNDdzUHdhcG8yMnpkMXF0N0dDa0MzckJrbkNMbnYvODdCWjcrWmhXR0JrYy8xMjZRUldwTUdoY283Ck4zRWhwNmk5OEo3NEY2UVkzRlhVRk9hNWUwNDdGUkZUOGZ2N2RLQjR6Nm45N05YWE9ZeDJIZ3d6R2ozNGliTGEKZUs3SkorTEUvL3BPT0h6bVF5UDZWdkU4ak91MDBXNW0xTGpsT0xsbzZ5MXpzajV5c2FvM3krUU90SjUvVUY1NQp3cWVWRW1uOVNxWTZEemp3ZWNoelU3OGFtN1I2end1ZzM1RWp0YTUvbVNvTGlBU0VtcEVTQ1hJUkhiK3BLSXNoCjZFRmNXbFAvcWNnbjVnZEk2aFBodlVwc1l5a2RMWHpQd1VKUVJLYnVPNWVGUGxBUzAzR2QzZz09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K
# TODO: generate this dynamically
insecureSkipTLSVerify: true
Copy link
Member

@enxebre enxebre Aug 17, 2018

Choose a reason for hiding this comment

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

why?

@enxebre
Copy link
Member

enxebre commented Aug 17, 2018

Yes, We need something static to test/develop

enxebre
enxebre reviewed Aug 17, 2018
View reviewed changes
manifests/clusterapi-apiserver.yaml
- name: cluster-apiserver-certs
secret:
secretName: cluster-apiserver-secrets
secretName: cluster-apiserver-certs
Copy link
Member

@enxebre enxebre Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Can you verify this consitently work? iirc there was some issue with previews name so vaguely this did not return empty as expected https://github.com/openshift/machine-api-operator/blob/master/vendor/github.com/coreos-inc/tectonic-operators/x-operator/pkg/xoperator/update.go#L114 so then this https://github.com/openshift/machine-api-operator/blob/master/vendor/github.com/coreos-inc/tectonic-operators/x-operator/pkg/xoperator/update.go#L125 and https://github.com/openshift/machine-api-operator/blob/master/vendor/github.com/coreos-inc/tectonic-operators/x-operator/pkg/xoperator/component.go#L194 fails to read

@paulfantom paulfantom removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2018
@paulfantom
Copy link
Contributor

paulfantom commented Aug 21, 2018

/test all

2 similar comments
@ingvagabund
Copy link
Member

ingvagabund commented Aug 22, 2018

/test all

@paulfantom
Copy link
Contributor

paulfantom commented Aug 23, 2018

/test all

ingvagabund added a commit to ingvagabund/machine-api-operator that referenced this pull request Aug 24, 2018
@ingvagabund
Merge pull request openshift#22 from frobware/master-machine-dev-conv…
83016ed 
…enience

Updates to examples/master-machine for dev convenience
@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 8, 2018

@trawler: PR needs rebase.

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2018
@enxebre enxebre closed this Sep 12, 2018
germanparente pushed a commit to germanparente/machine-api-operator that referenced this pull request Sep 23, 2025
@openshift-merge-robot
Merge pull request openshift#22 from odvarkadaniel/fix-machine-stuck-…
c1fce79 
…in-delete/add-state

OCPBUGS-3904: Register unknown instance groups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@enxebre enxebre enxebre left review comments

@ingvagabund ingvagabund Awaiting requested review from ingvagabund

Assignees

No one assigned

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@trawler @openshift-ci-robot @ingvagabund @enxebre @paulfantom @openshift-bot