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

Remove elasticsearch policy and port #6

Merged
merged 1 commit into from Jan 6, 2016
Merged

Conversation

lzap
Copy link
Member

@lzap lzap commented Mar 12, 2015

This patch is needed to be able to actually upgrade via yum, otherwise
elasticsearch_port_t causes issues during yum transaction.

This must be merged ideally in the same time as
theforeman/foreman-selinux#44 otherwise nightly will
not be upgradeable.

EDIT: This patch now removes the policy completely.

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

DO NOT MERGE upgrade paths do not work, I need to rewrite this totally.

@lzap lzap force-pushed the test branch 2 times, most recently from 7925eb2 to e2fdaf6 Compare March 19, 2015 12:37
@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

@domcleal how about this? I would follow the same startegy in foreman

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

The only question is now to effectively manage ports for the single port type. When we add docker or openstack, semanage port is clunky...

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

Maybe it's better to have it per service:

  • katello_elasticsearch_port_t
  • katello_docker_port_t
  • foreman_nova_port_t

@domcleal
Copy link

Yeah, I think you're right that with semanage it's easier to have separate types. Anyway, this style looks better and as long as this is released before the corresponding foreman-selinux change then it gets away from complex upgrade issues.

@domcleal
Copy link

Oh, simultaneously actually so we don't redefine the same port range as different names.

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

Ok I have renamed to katello_elasticsearch_port_t. Going to test this with updated foreman change.

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

@domcleal one question - if we merge this and build into nighlty and katello 2.2, how do we make sure that this is installed before the foreman-selinux update which relies on this?

@lzap
Copy link
Member Author

lzap commented Mar 19, 2015

I mean RC/nightly user can issue just yum upgrade and in this case foreman gets upgraded first (which will fail)

@domcleal
Copy link

Why might it fail? Maybe because the current loaded katello policy references the elasticsearch_port_t type that foreman-selinux removes? Maybe it needs more thought and a better solution then.

I don't think you can make this ever upgrade before foreman-selinux due to the direction of the dependency, unless you explicitly ask users to upgrade it first.

@lzap
Copy link
Member Author

lzap commented Mar 20, 2015

Ok how about to merge this, release into RC/nightly and then the refactoring in foreman merge later and only into nightly?

@domcleal
Copy link

How are you going to manage the port conflict?

@lzap
Copy link
Member Author

lzap commented Mar 26, 2015

@domcleal I think I found the solution:

  • Introduced katello_elasticsearch_port_t as a separate thing to prevent further conflicts
  • Added extra step in enable script - if elasticsearch_port_t is defined, move the port to the newly created katello_elasticsearch_port_t
  • Added optional_policy block to have this working with old elasticsearch_port_t so it works on 1.8 installation where the patch is not yet merged
  • We will merge this into nightly/1.8 katello and merge the other foreman-selinux patch only to nightly

echo "port -a -t elasticsearch_port_t -p tcp 9200-9300" > $TMP
# Previously elasticsearch_port_t was defined in Foreman - move it
/usr/sbin/semanage port -E | grep -qE '\s+elasticsearch_port_t\s+' && \
semanage port -m -t katello_elasticsearch_port_t -p tcp 9200-9300

Choose a reason for hiding this comment

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

Do you need to remove the ports from elasticsearch_port_t here? Maybe echo "port -d -t elasticsearch_port_t" >> $TMP (made up) or similar?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we actually move the port numbers from elasticsearch_port_t to katello_elasticsearch_port_t. Calling -d on elasticsearch_port_t would fail.

Note this is not part of the $TMP transaction as I was not sure if it works in one call. Wanted it outside of it.

Copy link

Choose a reason for hiding this comment

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

Sorry, what I don't get is that this code doesn't appear to move the ports, it just adds them to katello_*. What removes them from elasticsearch_port_t?

Copy link

Choose a reason for hiding this comment

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

lzap explained on IRC that semanage will apparently move the port definitions from elasticsearch_port_t to katello_elasticsearch_port_t when running this command. I'm surprised, but haven't tested it.

@domcleal
Copy link

@lzap good idea, I think that makes sense.

@domcleal
Copy link

domcleal commented Apr 2, 2015

I think a change will also be required to foreman-selinux sooner than 1.9. If this works and is active in a Katello 2.1 + Foreman 1.8.0 installation then will a subsequent upgrade of foreman-selinux to 1.8.1 cause foreman-selinux-enable to add elasticsearch_port_t again after this has renamed it?

@lzap lzap self-assigned this Apr 29, 2015
@lzap lzap changed the title Refs #9126 - moved Katello policy to a separate repo WIP rename elasticsearch port resource to prevent conflicts with base policy May 11, 2015
@ehelms
Copy link
Member

ehelms commented Jun 23, 2015

@lzap what's the status of this PR? Seems it has been sitting for a while

@lzap
Copy link
Member Author

lzap commented Jun 24, 2015

This is still WIP, trying to find a way how to do this. We see issues with upgrades.

@ehelms
Copy link
Member

ehelms commented Jul 9, 2015

@lzap OK, if this is no required for the next set of releases (e.g. 1.9/2.3) then I wouldn't worry to much about it. Katello 2.4 should see the removal of ES from your stack.

@lzap
Copy link
Member Author

lzap commented Jul 15, 2015

Sounds like a plan, unfortunately in 1.9 we will have duplicate elasticsearch policies.

@ehelms
Copy link
Member

ehelms commented Nov 18, 2015

We have removed elasticsearch from our deployments in nightly at this point -- can this be closed in light of?

@ehelms
Copy link
Member

ehelms commented Dec 14, 2015

@lzap ping

@lzap
Copy link
Member Author

lzap commented Jan 4, 2016

Ok can you confirm we no longer start ES or even connect to it in any way? If so, let's close this one and I can go ahead removing this port completely from the codebase.

@ehelms
Copy link
Member

ehelms commented Jan 5, 2016

I can confirm that.

@lzap lzap changed the title WIP rename elasticsearch port resource to prevent conflicts with base policy Remove elasticsearch policy and port Jan 6, 2016
@lzap
Copy link
Member Author

lzap commented Jan 6, 2016

REBASED. This patch now removes the policy completely.

@jlsherrill
Copy link
Contributor

ACK looks good to me. thanks @lzap !

jlsherrill added a commit that referenced this pull request Jan 6, 2016
Remove elasticsearch policy and port
@jlsherrill jlsherrill merged commit 055444e into theforeman:master Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants