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

pkg/types/aws/machinepool: Drop IAM-role overrides #1154

Conversation

wking
Copy link
Member

@wking wking commented Jan 30, 2019

We're planning on dropping instance profiles in favor of the new credentials operator, because we want AWS access to have operator/pod/namespace granularity and not instance granularity. Many pods could be running on a given instance, and not all of them should have a given permission. While we're blocked from dropping these at the moment due to kubelet cloud-config+secrets, we can drop the user-facing knobs for this feature now. Then pivoting the internal approach once we get the kubelet sorted will be a non-breaking change.

CC @rajatchopra.

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 30, 2019
@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch from 3feb1d8 to 5b46586 Compare January 30, 2019 17:42
@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch from 5b46586 to dca6982 Compare January 30, 2019 21:51
@wking
Copy link
Member Author

wking commented Jan 30, 2019

I've pushed 5b46586 -> dca6982, fixing some broken installConfigVersion -> InstallConfigVersion references (I'd initially waffled over whether the constant should be public).

@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch 2 times, most recently from fa74b39 to 93df306 Compare January 30, 2019 22:08
@wking
Copy link
Member Author

wking commented Jan 30, 2019

image timed out awaiting headers (openshift/release#2505).

/test images
/test e2e-aws

@wking
Copy link
Member Author

wking commented Jan 31, 2019

/retest

@wking
Copy link
Member Author

wking commented Jan 31, 2019

images:

2019/01/31 00:21:46 Copied 0.27Mi of artifacts from release-latest to /logs/artifacts/release-latest
2019/01/31 00:21:46 error: unable to signal to artifacts container to terminate in pod release-latest, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2019/01/31 00:21:51 Ran for 7m8s
error: could not run steps: test "release-latest" failed: pod release-latest was already deleted

/retest

@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch from 93df306 to 5b005cb Compare February 1, 2019 06:26
@wking
Copy link
Member Author

wking commented Feb 1, 2019

I've pushed 93df306 -> 5b005cb to address:

level=fatal msg="failed to fetch Terraform Variables: failed to load asset \"Install Config\": invalid \"install-config.yaml\" file: apiVersion: Invalid value: \"v1beta1\": install-config version must be \"v1beta2\""

because CI continues to supply the v1beta1 install config (although it does not set the property I'm removing).

@wking
Copy link
Member Author

wking commented Feb 1, 2019

images:

2019/02/01 06:50:36 Container artifacts in pod release-latest completed successfully
2019/02/01 06:50:51 error: unable to signal to artifacts container to terminate in pod release-latest, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2019/02/01 06:50:51 error: unable to retrieve artifacts from pod release-latest: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")
2019/02/01 06:50:57 Ran for 23m50s

/retest

@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch from 5b005cb to 80b3bf4 Compare February 1, 2019 07:56
@openshift-ci-robot openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2019
We're planning on dropping instance profiles in favor of the new
credentials operator [1], because we want AWS access to have
operator/pod/namespace granularity and not instance granularity.  Many
pods could be running on a given instance, and not all of them should
have a given permission.  While we're blocked from dropping these at
the moment due to kubelet cloud-config+secrets [2], we can drop the
user-facing knobs for this feature now.  Then pivoting the internal
approach once we get the kubelet sorted will be a non-breaking change.

[1]: https://github.com/openshift/cloud-credential-operator
[2]: openshift#697 (comment)
@wking wking force-pushed the remove-user-facing-instance-profile-knobs branch from 80b3bf4 to 3b393da Compare February 1, 2019 08:52
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 1, 2019
@wking
Copy link
Member Author

wking commented Feb 1, 2019

images:

2019/02/01 09:19:46 Container artifacts in pod release-latest completed successfully
2019/02/01 09:19:57 error: unable to signal to artifacts container to terminate in pod release-latest, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2019/02/01 09:19:57 error: unable to retrieve artifacts from pod release-latest: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")
2019/02/01 09:20:03 Ran for 27m5s
error: could not run steps: test "release-latest" failed: pod release-latest was already deleted

Probably the CI master CPU pegging from last night.

/retest

@wking
Copy link
Member Author

wking commented Feb 1, 2019

e2e-aws:

level=info msg="Waiting up to 30m0s for the cluster to initialize..."
level=fatal msg="failed to initialize the cluster: timed out waiting for the condition"

/retest

@wking
Copy link
Member Author

wking commented Feb 2, 2019

All green :)

@crawford
Copy link
Contributor

crawford commented Feb 2, 2019

I consider this a bug fix.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crawford, wking

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

The pull request process is described here

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-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 8604f1f into openshift:master Feb 2, 2019
@wking
Copy link
Member Author

wking commented Feb 2, 2019

@dgoodwin, @vrutkovs: I think it's very unlikely that you were setting the role-name override, but you'll need to bump your generated install-config version string before we can remove this. Can you let me know when you have?

wking added a commit to wking/openshift-release that referenced this pull request Feb 2, 2019
To keep up with openshift/installer@3b393da
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154).  The change only affected AWS, and we
didn't set the removed property anyway.
@wking wking deleted the remove-user-facing-instance-profile-knobs branch February 4, 2019 18:21
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013) and openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013) and openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 14, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
csrwng pushed a commit to csrwng/hive that referenced this pull request Feb 19, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/hive that referenced this pull request Feb 21, 2019
Catching up with openshift/installer@dafc79f0 (Generate
Network.cluster config instead of NetworkConfig.networkoperator,
2019-01-15, openshift/installer#1013), openshift/installer@3b393da8
(pkg/types/aws/machinepool: Drop IAM-role overrides, 2019-01-30,
openshift/installer#1154), and openshift/installer@9ad20c35
(pkg/destroy/aws: Remove ClusterName consumer, 2019-01-31,
openshift/installer#1170).

The uint32 -> int32 cast is slightly dangerous, because it will
silently wrap overflowing values [1,2].  But I'll try and get the
installer updated to use unsigned types as well, and then we won't
have to worry about converting.

[1]: golang/go#19624
[2]: golang/go#30209
wking added a commit to wking/openshift-installer that referenced this pull request Feb 26, 2019
I'd kept this in 3b393da (pkg/types/aws/machinepool: Drop IAM-role
overrides, 2019-01-30, openshift#1154) to support CI.  But with
openshift/release@d31f601e (ci-operator/templates/openshift: Bump
install-config.yaml to v1beta2, 2019-02-02, openshift/release#2772)
and openshift/hive@ab7ee975 (*: Bump to install-config v0.12.0,
2019-02-14, openshift/hive#222) landed, we no longer need the
workaround.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants